Bug 68099 - [V8] Null v8::Context crash in V8DOMWindowShell::namedItemAdded()
Summary: [V8] Null v8::Context crash in V8DOMWindowShell::namedItemAdded()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-14 11:55 PDT by Nate Chapin
Modified: 2011-10-12 11:45 PDT (History)
3 users (show)

See Also:


Attachments
patch (1.90 KB, patch)
2011-09-14 13:19 PDT, Nate Chapin
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
patch - fixed inverted logic and attempted cr-mac build fix (2.06 KB, patch)
2011-09-14 15:34 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Log where exactly the context initialization failed, use histograms (5.53 KB, patch)
2011-09-29 11:41 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Remove logging and always check the result of initContextIfNeeded() (8.00 KB, patch)
2011-10-04 13:53 PDT, Nate Chapin
abarth: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (32.55 KB, application/zip)
2011-10-04 14:24 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from ec2-cq-03 (32.55 KB, application/zip)
2011-10-04 15:56 PDT, WebKit Review Bot
no flags Details
missed a case in CodeGeneratorV8.pm (8.78 KB, patch)
2011-10-05 10:33 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Fix my mistakes in CodeGeneratorV8.pm (8.80 KB, patch)
2011-10-10 15:14 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2011-09-14 11:55:43 PDT
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.
Comment 1 Nate Chapin 2011-09-14 13:19:35 PDT
Created attachment 107381 [details]
patch
Comment 2 WebKit Review Bot 2011-09-14 13:42:22 PDT
Comment on attachment 107381 [details]
patch

Attachment 107381 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9658674
Comment 3 Nate Chapin 2011-09-14 15:34:52 PDT
Created attachment 107409 [details]
patch - fixed inverted logic and attempted cr-mac build fix
Comment 4 Adam Barth 2011-09-14 16:33:04 PDT
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.
Comment 5 WebKit Review Bot 2011-09-14 22:11:58 PDT
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>
Comment 6 WebKit Review Bot 2011-09-14 22:12:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Nate Chapin 2011-09-29 11:40:01 PDT
This got auto-closed by review bot....reopening and new logging patch incoming.
Comment 8 Nate Chapin 2011-09-29 11:41:07 PDT
Created attachment 109180 [details]
Log where exactly the context initialization failed, use histograms
Comment 9 WebKit Review Bot 2011-09-29 12:45:09 PDT
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>
Comment 10 WebKit Review Bot 2011-09-29 12:45:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Nate Chapin 2011-09-29 12:47:14 PDT
(In reply to comment #10)
> All reviewed patches have been landed.  Closing bug.

Reopening what reviewbot closed (again).
Comment 12 Nate Chapin 2011-10-04 13:53:46 PDT
Created attachment 109678 [details]
Remove logging and always check the result of initContextIfNeeded()
Comment 13 WebKit Review Bot 2011-10-04 14:23:58 PDT
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
Comment 14 WebKit Review Bot 2011-10-04 14:24:01 PDT
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
Comment 15 Adam Barth 2011-10-04 15:30:47 PDT
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.
Comment 16 Nate Chapin 2011-10-04 15:34:26 PDT
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.
Comment 17 WebKit Review Bot 2011-10-04 15:56:08 PDT
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
Comment 18 WebKit Review Bot 2011-10-04 15:56:11 PDT
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
Comment 19 Nate Chapin 2011-10-05 10:33:08 PDT
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.
Comment 20 Adam Barth 2011-10-09 14:38:36 PDT
Comment on attachment 109819 [details]
missed a case in CodeGeneratorV8.pm

This patch is causing the cr-linux EWS to spin.
Comment 21 Nate Chapin 2011-10-10 15:14:53 PDT
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.
Comment 22 Adam Barth 2011-10-12 09:36:12 PDT
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?
Comment 23 Nate Chapin 2011-10-12 09:41:44 PDT
Comment on attachment 110413 [details]
Fix my mistakes in CodeGeneratorV8.pm

Looks like run-bindings-tests still passes.
Comment 24 WebKit Review Bot 2011-10-12 11:44:55 PDT
Comment on attachment 110413 [details]
Fix my mistakes in CodeGeneratorV8.pm

Clearing flags on attachment: 110413

Committed r97280: <http://trac.webkit.org/changeset/97280>
Comment 25 WebKit Review Bot 2011-10-12 11:45:01 PDT
All reviewed patches have been landed.  Closing bug.