Bug 107050

Summary: IndexedDB: Possible null ScriptExecutionContext passed to callbacks during frame destruction
Product: WebKit Reporter: Joshua Bell <jsbell>
Component: WebCore Misc.Assignee: Joshua Bell <jsbell>
Status: RESOLVED INVALID    
Severity: Normal CC: alecflett, dgrogan, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 36565, 107141    
Bug Blocks: 107146    
Attachments:
Description Flags
Patch
none
Patch for landing none

Description Joshua Bell 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.
Comment 1 Joshua Bell 2013-01-16 13:38:08 PST
Created attachment 183029 [details]
Patch
Comment 2 Tony Chang 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.
Comment 3 Joshua Bell 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.
Comment 4 Tony Chang 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.
Comment 5 Tony Chang 2013-01-16 14:28:40 PST
Comment on attachment 183029 [details]
Patch

Seems OK to try, but please explain more in the ChangeLog.
Comment 6 Joshua Bell 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.
Comment 7 Tony Chang 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 :(
Comment 8 Joshua Bell 2013-01-16 14:55:42 PST
Created attachment 183039 [details]
Patch for landing
Comment 9 WebKit Review Bot 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>
Comment 10 WebKit Review Bot 2013-01-16 15:12:36 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Joshua Bell 2013-01-17 10:10:52 PST
Reports from users that the crash still occurs. After verifying, will likely back this out.
Comment 12 WebKit Review Bot 2013-01-17 10:26:34 PST
Re-opened since this is blocked by bug 107141
Comment 13 Joshua Bell 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.