[webkit-reviews] review requested: [Bug 42843] Interrupt all DB operations when a worker is shutting down : [Attachment 62497] patch

[bugzilla-daemon at webkit.org]

Dumitru Daniliuc <dumi at chromium.org> has asked	for review:
Bug 42843: Interrupt all DB operations when a worker is shutting down
https://bugs.webkit.org/show_bug.cgi?id=42843

Attachment 62497: patch
https://bugs.webkit.org/attachment.cgi?id=62497&action=review

------- Additional Comments from Dumitru Daniliuc <dumi at chromium.org>
(In reply to comment #6)
> (From update of attachment 62337 [details])
> http://wkrietveld.appspot.com/42843/diff/1/6
> File WebCore/storage/AbstractDatabase.cpp (right):
> 
> http://wkrietveld.appspot.com/42843/diff/1/6#newcode480
> WebCore/storage/AbstractDatabase.cpp:480: while
(!m_interruptedGuard.tryLock())
> i wonder if we can find a way to do this w/o looping like this

i don't think there's a way to do this without looping. i moved this code to
SQLiteDatabase though.

> http://wkrietveld.appspot.com/42843/diff/1/8
> File WebCore/storage/Database.cpp (right):
> 
> http://wkrietveld.appspot.com/42843/diff/1/8#newcode92
> WebCore/storage/Database.cpp:92: MutexLocker
interruptedMutexLock(DatabaseTracker::tracker().interruptedMutex());
> This lock can be held for a very long time. There are some actions below that
go ranging far and wide.
> 
> See DatabaseTracker::canEstablishDatabase and in particular this comment...
> 
>     // Drop all locks before calling out; we don't know what they'll do.
>     context->databaseExceededQuota(name);
> 
> And openAndVerifyVersion() requires a task to be performed on the db thread
while the current thread waits for it to complete (with this lock held). Is
this mutex ever held on the db thread?
> 

reverted the changes to Database and DatabaseSync.
DatabaseTracker::interruptAllDatabasesForContext() is the only new place where
this lock is being held, but only for a short period of time.

> http://wkrietveld.appspot.com/42843/diff/1/9#newcode73
> WebCore/storage/DatabaseSync.cpp:73: creationCallback->handleEvent(context,
database.get());
> For example, what if the creation callback being invoked here calls
openDatabaseSync().

no more locks held here.

> http://wkrietveld.appspot.com/42843/diff/1/10
> File WebCore/storage/DatabaseTracker.cpp (right):
> 
> http://wkrietveld.appspot.com/42843/diff/1/10#newcode262
> WebCore/storage/DatabaseTracker.cpp:262: 
> It looks like the loop is interrupt all dbs for a particular origin rather
than for a particular context.

i believe every ScriptExecutionContext has only one origin. see
ScriptExecutionContext::m_securityOrigin().

> http://wkrietveld.appspot.com/42843/diff/1/11
> File WebCore/storage/DatabaseTracker.h (right):
> 
> http://wkrietveld.appspot.com/42843/diff/1/11#newcode69
> WebCore/storage/DatabaseTracker.h:69: Mutex& interruptedMutex() { return
m_openDatabaseMapGuard; }
> oh... this is one of the existing locks, not a new one.

reverted this change; this method is no longer needed.

> http://wkrietveld.appspot.com/42843/diff/1/17
> File WebCore/workers/WorkerThread.cpp (right):
> 
> http://wkrietveld.appspot.com/42843/diff/1/17#newcode228
> WebCore/workers/WorkerThread.cpp:228:
DatabaseTracker::tracker().interruptAllDatabasesForContext(workerContext());
> workerContext may be NULL here

moved inside the if-block, after the script execution is forbidden.