Bug 68099 – [V8] Null v8::Context crash in V8DOMWindowShell::namedItemAdded()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

Original report at http://code.google.com/p/chromium/issues/detail?id=96073.

For reasons that aren't clear, V8DOMWindowShell::namedItemAdded() is calling initContextIfNeeded(), but no v8::Context is getting initialized.  We then crash when we try to enter the null context.

We don't no why this is happening, so to start with we're going to see if it works to just exit early on null context. In addition, I'm going to include a bunch of INC_STATS logging to see if we can get some more data on what exactly is going wrong.

[Nate Chapin]

Created attachment 107381 [details]
patch

[WebKit Review Bot]

Comment on attachment 107381 [details]
patch

Attachment 107381 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9658674

[Nate Chapin]

Created attachment 107409 [details]
patch - fixed inverted logic and attempted cr-mac build fix

[Adam Barth]

Comment on attachment 107409 [details]
patch - fixed inverted logic and attempted cr-mac build fix

View in context: https://bugs.webkit.org/attachment.cgi?id=107409&action=review

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:574
> +        // FIXME: Temporary diagnostics as to why V8 sometimes crashes with a null context below.

Technically we should either skip the FIXME or re-word this to explain when we should remove this code.

[WebKit Review Bot]

Comment on attachment 107409 [details]
patch - fixed inverted logic and attempted cr-mac build fix

Clearing flags on attachment: 107409

Committed r95166: <http://trac.webkit.org/changeset/95166>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nate Chapin]

This got auto-closed by review bot....reopening and new logging patch incoming.

[Nate Chapin]

Created attachment 109180 [details]
Log where exactly the context initialization failed, use histograms

[WebKit Review Bot]

Comment on attachment 109180 [details]
Log where exactly the context initialization failed, use histograms

Clearing flags on attachment: 109180

Committed r96349: <http://trac.webkit.org/changeset/96349>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nate Chapin]

(In reply to comment #10)
> All reviewed patches have been landed.  Closing bug.

Reopening what reviewbot closed (again).

[Nate Chapin]

Created attachment 109678 [details]
Remove logging and always check the result of initContextIfNeeded()

[WebKit Review Bot]

Comment on attachment 109678 [details]
Remove logging and always check the result of initContextIfNeeded()

Attachment 109678 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9942474

New failing tests:
css1/basic/comments.html
css1/basic/grouping.html
canvas/philip/tests/2d.canvas.readonly.html
http/tests/appcache/access-via-redirect.php
http/tests/appcache/auth.html
animations/animation-css-rule-types.html
css1/basic/contextual_selectors.html
animations/animation-direction.html
http/tests/appcache/cyrillic-uri.html
css1/basic/containment.html
http/tests/appcache/credential-url.html
http/tests/appcache/crash-when-navigating-away-then-back.html
animations/animation-add-events-in-handler.html
animations/animation-controller-drt-api.html
canvas/philip/tests/2d.canvas.reference.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html
css1/basic/class_as_selector.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
animations/animation-direction-normal.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html

[WebKit Review Bot]

Created attachment 109689 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Adam Barth]

Comment on attachment 109678 [details]
Remove logging and always check the result of initContextIfNeeded()

This looks good.  I'm not sure why the bot is complaining about all those test failures.

[Nate Chapin]

Comment on attachment 109678 [details]
Remove logging and always check the result of initContextIfNeeded()

Yeah, it's really weird.  I'm just gonna cq+ it and see if it happens again, since this isn't happening locally for me.

[WebKit Review Bot]

Comment on attachment 109678 [details]
Remove logging and always check the result of initContextIfNeeded()

Rejecting attachment 109678 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
arRect+fillRect.basic.html = CRASH
  css1/basic/class_as_selector.html = CRASH
  css1/basic/comments.html = CRASH
  css1/basic/containment.html = CRASH
  css1/basic/contextual_selectors.html = CRASH
  css1/basic/grouping.html = CRASH
  http/tests/appcache/access-via-redirect.php = CRASH
  http/tests/appcache/auth.html = CRASH
  http/tests/appcache/crash-when-navigating-away-then-back.html = CRASH
  http/tests/appcache/credential-url.html = CRASH
  http/tests/appcache/cyrillic-uri.html = CRASH



Full output: http://queues.webkit.org/results/9954218

[WebKit Review Bot]

Created attachment 109708 [details]
Archive of layout-test-results from ec2-cq-03

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Nate Chapin]

Created attachment 109819 [details]
missed a case in CodeGeneratorV8.pm

The case in CodeGeneratorV8.pm apparently depended on initContextIfNeeded() returning false if a context already existed. Just checking for a context before calling initContextIfNeeded() should be sufficient in that case.

[Adam Barth]

Comment on attachment 109819 [details]
missed a case in CodeGeneratorV8.pm

This patch is causing the cr-linux EWS to spin.

[Nate Chapin]

Created attachment 110413 [details]
Fix my mistakes in CodeGeneratorV8.pm

Instead of only calling initContextIfNeeded() if it was actually needed in CodeGeneratorV8.pm, I just called it a different way. Silly me.

[Adam Barth]

Comment on attachment 110413 [details]
Fix my mistakes in CodeGeneratorV8.pm

View in context: https://bugs.webkit.org/attachment.cgi?id=110413&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2711
> -            if (proxy->windowShell()->initContextIfNeeded()) {
> +            if (proxy->windowShell()->context().IsEmpty() && proxy->windowShell()->initContextIfNeeded()) {

Do you need to run-bindings-tests?

[Nate Chapin]

Comment on attachment 110413 [details]
Fix my mistakes in CodeGeneratorV8.pm

Looks like run-bindings-tests still passes.

[WebKit Review Bot]

Comment on attachment 110413 [details]
Fix my mistakes in CodeGeneratorV8.pm

Clearing flags on attachment: 110413

Committed r97280: <http://trac.webkit.org/changeset/97280>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.