Bug 90832

Summary: IndexedDB: ASSERT hit calling open from callback in Worker
Product: WebKit Reporter: Joshua Bell <jsbell>
Component: WebCore Misc.Assignee: Charles Wei <charles.wei>
Status: RESOLVED FIXED    
Severity: Major CC: abarth, alecflett, charles.wei, dgrogan, haraken, jochen, levin, webkit.review.bot
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 88338    
Bug Blocks: 90310    
Attachments:
Description Flags
Layout Test repro - HTML file
none
Layout Test repro - JS file
none
Patch none

Joshua Bell
Reported 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).
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
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Joshua Bell
Comment 1 2012-07-09 16:07:42 PDT
Created attachment 151343 [details] Layout Test repro - JS file
Joshua Bell
Comment 2 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.)
Joshua Bell
Comment 3 2012-07-10 14:12:10 PDT
Created attachment 151524 [details] Patch
Joshua Bell
Comment 4 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?
David Grogan
Comment 5 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.
Charles Wei
Comment 6 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.
Joshua Bell
Comment 7 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?
Joshua Bell
Comment 8 2012-07-11 15:18:02 PDT
Adding haraken@ who may also have opinions and has reviewed worker code in the past.
Charles Wei
Comment 9 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.
Kentaro Hara
Comment 10 2012-07-11 23:24:21 PDT
Comment on attachment 151524 [details] Patch LGTM
WebKit Review Bot
Comment 11 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>
WebKit Review Bot
Comment 12 2012-07-12 08:38:38 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.