IndexedDB: ASSERT hit calling open from callback in Worker

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joshua Bell]

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).

[Joshua Bell]

Created attachment 151343 [details]
Layout Test repro - JS file

[Joshua Bell]

> 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.)

[Joshua Bell]

Created attachment 151524 [details]
Patch

[Joshua Bell]

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?

[David Grogan]

Comment on attachment 151524 [details]
Patch

LGTM

This fix is more involved than your alternative but this one feels safer.

[Charles Wei]

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.

[Joshua Bell]

(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?

[Joshua Bell]

Adding haraken@ who may also have opinions and has reviewed worker code in the past.

[Charles Wei]

(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.

[Kentaro Hara]

Comment on attachment 151524 [details]
Patch

LGTM

[WebKit Review Bot]

Comment on attachment 151524 [details]
Patch

Clearing flags on attachment: 151524

Committed r122463: <http://trac.webkit.org/changeset/122463>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.