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).
Created attachment 151343 [details] Layout Test repro - JS file
> 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.)
Created attachment 151524 [details] Patch
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 on attachment 151524 [details] Patch LGTM This fix is more involved than your alternative but this one feels safer.
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.
(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?
Adding haraken@ who may also have opinions and has reviewed worker code in the past.
(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 on attachment 151524 [details] Patch LGTM
Comment on attachment 151524 [details] Patch Clearing flags on attachment: 151524 Committed r122463: <http://trac.webkit.org/changeset/122463>
All reviewed patches have been landed. Closing bug.