Bug 90832 - IndexedDB: ASSERT hit calling open from callback in Worker
Summary: IndexedDB: ASSERT hit calling open from callback in Worker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Major
Assignee: Charles Wei
URL:
Keywords:
Depends on: 88338
Blocks: 90310
  Show dependency treegraph
 
Reported: 2012-07-09 16:06 PDT by Joshua Bell
Modified: 2012-07-12 08:38 PDT (History)
8 users (show)

See Also:


Attachments
Layout Test repro - HTML file (273 bytes, text/html)
2012-07-09 16:06 PDT, Joshua Bell
no flags Details
Layout Test repro - JS file (637 bytes, application/javascript)
2012-07-09 16:07 PDT, Joshua Bell
no flags Details
Patch (21.80 KB, patch)
2012-07-10 14:12 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2012-07-09 16:06:56 PDT
Created attachment 151342 [details]
Layout Test repro - HTML file

View in context: https://bugs.webkit.org/attachment.cgi?id=150330&action=review

> Source/WebCore/workers/WorkerThread.cpp:198
> +    return m_startupData->m_groupSettings.get();

m_startupData is cleared after the initial script evaluation but before the event loop is executed. This means that any calls to IDBFactory methods that access this (e.g. open) that occur in a callback will dereference null here.

I'm hitting this trying to rebase 90310 on top of this patch; it doesn't look like tests with multiple open() calls from workers were tested against this patch.

...

To repro, drop the attachments into LayoutTests/storage/indexeddb/open-twice-workers.html and LayoutTests/storage/indexeddb/resources/open-twice.js - it should crash (hitting assert).
Comment 1 Joshua Bell 2012-07-09 16:07:42 PDT
Created attachment 151343 [details]
Layout Test repro - JS file
Comment 2 Joshua Bell 2012-07-09 16:09:01 PDT
> To repro, drop the attachments into LayoutTests/storage/indexeddb/open-twice-workers.html and LayoutTests/storage/indexeddb/resources/open-twice.js - it should crash (hitting assert).

By which I mean: save the attachments out as those two files, and execute via new-run-webkit-tests (etc). (No dragging-and-dropping involved.)
Comment 3 Joshua Bell 2012-07-10 14:12:10 PDT
Created attachment 151524 [details]
Patch
Comment 4 Joshua Bell 2012-07-10 14:15:09 PDT
I took a stab at fixing this.

An alternate (and much shorter) fix would be to just move the m_startupData.clear() line after runEventLoop() inside WorkerThread::workerThread() but that seems to violate the spirit of the surrounding code. 

Opinions?
Comment 5 David Grogan 2012-07-10 18:29:10 PDT
Comment on attachment 151524 [details]
Patch

LGTM

This fix is more involved than your alternative but this one feels safer.
Comment 6 Charles Wei 2012-07-11 02:11:13 PDT
Comment on attachment 151524 [details]
Patch

You cached the GroupSettings in the WorkerThread as a data member instead of the StartupData,  that looks good. you might also be able keep the API WorkerThread::groupSettings(),  which gets the GroupSetting from m_groupSettings instead of from startupData().  This way,  you don't need to change so many things in WorkerContext, SharedWorkerContext,  IDBFactory.
Comment 7 Joshua Bell 2012-07-11 10:27:10 PDT
(In reply to comment #6)
> (From update of attachment 151524 [details])
> You cached the GroupSettings in the WorkerThread as a data member instead of the StartupData,  that looks good. you might also be able keep the API WorkerThread::groupSettings(),  which gets the GroupSetting from m_groupSettings instead of from startupData().  This way,  you don't need to change so many things in WorkerContext, SharedWorkerContext,  IDBFactory.

I pass the GroupSettings all the way through to the context which caches it. It could be cached on the thread instead which would reduce the size of the patch, but other properties (e.g. URL, userAgent, etc) are cached on the context not the thread so it seemed appropriate to follow the same pattern.

abarth@, jochen@ or anyone else - opinions? r?
Comment 8 Joshua Bell 2012-07-11 15:18:02 PDT
Adding haraken@ who may also have opinions and has reviewed worker code in the past.
Comment 9 Charles Wei 2012-07-11 20:29:36 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 151524 [details] [details])
> > You cached the GroupSettings in the WorkerThread as a data member instead of the StartupData,  that looks good. you might also be able keep the API WorkerThread::groupSettings(),  which gets the GroupSetting from m_groupSettings instead of from startupData().  This way,  you don't need to change so many things in WorkerContext, SharedWorkerContext,  IDBFactory.
> 
> I pass the GroupSettings all the way through to the context which caches it. It could be cached on the thread instead which would reduce the size of the patch, but other properties (e.g. URL, userAgent, etc) are cached on the context not the thread so it seemed appropriate to follow the same pattern.
> 
> abarth@, jochen@ or anyone else - opinions? r?

Ok, Sounds reasonable.
Comment 10 Kentaro Hara 2012-07-11 23:24:21 PDT
Comment on attachment 151524 [details]
Patch

LGTM
Comment 11 WebKit Review Bot 2012-07-12 08:38:33 PDT
Comment on attachment 151524 [details]
Patch

Clearing flags on attachment: 151524

Committed r122463: <http://trac.webkit.org/changeset/122463>
Comment 12 WebKit Review Bot 2012-07-12 08:38:38 PDT
All reviewed patches have been landed.  Closing bug.