RESOLVED INVALID 107050
IndexedDB: Possible null ScriptExecutionContext passed to callbacks during frame destruction 
https://bugs.webkit.org/show_bug.cgi?id=107050
Summary IndexedDB: Possible null ScriptExecutionContext passed to callbacks during fr...
Joshua Bell
Reported 2013-01-16 13:34:32 PST
Trying to track down a null-dereference crash involving Workers and IndexedDB, one possibility is that the generated binding code gets a null context from WorkerScriptController::controllerForContext() and passes this into DOM callbacks (including IDB). It seems unlikely, but we'd like to confirm or refute the hypothesis.
Attachments
Patch (1.89 KB, patch)
2013-01-16 13:38 PST, Joshua Bell 		no flags
DetailsFormatted DiffDiff
Patch for landing (2.18 KB, patch)
2013-01-16 14:55 PST, Joshua Bell 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Joshua Bell
Comment 1 2013-01-16 13:38:08 PST
Created attachment 183029 [details] Patch
Tony Chang
Comment 2 2013-01-16 14:09:24 PST
Comment on attachment 183029 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183029&action=review > Source/WebCore/ChangeLog:7 > + Temporary code to defend against null contexts. Will either refute a hypothesis, or we'll > + need to make a more systemic fix elsewhere. Either way it will be removed in a few days. How will we decide whether it worked or not? Is this something we're seeing in the wild or something that cluster fuzz caught? I'm a bit nervous about this even if it fixes the crash because it might be hiding some other bug. We need a repro case to be certain. Someone who knows more about frame destruction (abarth?) can probably comment whether this ought to be needed or not.
Joshua Bell
Comment 3 2013-01-16 14:20:22 PST
Sorry, should have linked to crbug.com/168503 for context. We've been unable to repro locally, but some users can do so reliably with a simple repro case. We're still working on trying to repro it ourselves using virtual machines, etc.
Tony Chang
Comment 4 2013-01-16 14:27:50 PST
I see, you might want to mention that WorkerScriptController::controllerForContext() does a similar check. Does the change that added the check in WorkerScriptController::controllerForContext() include a test for this case? Maybe that will provide some hints? I would also mention in the changelog that we're unable to repro and we're watching crash reports from users.
Tony Chang
Comment 5 2013-01-16 14:28:40 PST
Comment on attachment 183029 [details] Patch Seems OK to try, but please explain more in the ChangeLog.
Joshua Bell
Comment 6 2013-01-16 14:43:18 PST
Thanks, tony@ - great ideas. For reference: https://bugs.webkit.org/show_bug.cgi?id=36565 - doesn't look like new tests were added.
Tony Chang
Comment 7 2013-01-16 14:50:49 PST
(In reply to comment #6) > Thanks, tony@ - great ideas. > > For reference: > > https://bugs.webkit.org/show_bug.cgi?id=36565 - doesn't look like new tests were added. That was a code move. The original patch that added the lines is https://bugs.webkit.org/show_bug.cgi?id=28004 , which also doesn't have a test :(
Joshua Bell
Comment 8 2013-01-16 14:55:42 PST
Created attachment 183039 [details] Patch for landing
WebKit Review Bot
Comment 9 2013-01-16 15:12:32 PST
Comment on attachment 183039 [details] Patch for landing Clearing flags on attachment: 183039 Committed r139929: <http://trac.webkit.org/changeset/139929>
WebKit Review Bot
Comment 10 2013-01-16 15:12:36 PST
All reviewed patches have been landed. Closing bug.
Joshua Bell
Comment 11 2013-01-17 10:10:52 PST
Reports from users that the crash still occurs. After verifying, will likely back this out.
WebKit Review Bot
Comment 12 2013-01-17 10:26:34 PST
Re-opened since this is blocked by bug 107141
Joshua Bell
Comment 13 2013-01-17 10:30:40 PST
Rolled this out. Closing as INVALID since this wasn't the proximal cause of the crashes we're seeing.
Note You need to log in before you can comment on or make changes to this bug.