Bug 107050 – IndexedDB: Possible null ScriptExecutionContext passed to callbacks during frame destruction

• Component: WebCore Misc.
• Status: RESOLVED INVALID

[Joshua Bell]

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.

[Joshua Bell]

Created attachment 183029 [details]
Patch

[Tony Chang]

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]

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]

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 on attachment 183029 [details]
Patch

Seems OK to try, but please explain more in the ChangeLog.

[Joshua Bell]

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]

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

Created attachment 183039 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 183039 [details]
Patch for landing

Clearing flags on attachment: 183039

Committed r139929: <http://trac.webkit.org/changeset/139929>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Joshua Bell]

Reports from users that the crash still occurs. After verifying, will likely back this out.

[WebKit Review Bot]

Re-opened since this is blocked by bug 107141

[Joshua Bell]

Rolled this out. Closing as INVALID since this wasn't the proximal cause of the crashes we're seeing.