RESOLVED FIXED 68099
[V8] Null v8::Context crash in V8DOMWindowShell::namedItemAdded()
https://bugs.webkit.org/show_bug.cgi?id=68099
Summary [V8] Null v8::Context crash in V8DOMWindowShell::namedItemAdded()
Nate Chapin
Reported 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.
Attachments
patch (1.90 KB, patch)
2011-09-14 13:19 PDT, Nate Chapin
webkit.review.bot: commit-queue-
patch - fixed inverted logic and attempted cr-mac build fix (2.06 KB, patch)
2011-09-14 15:34 PDT, Nate Chapin
no flags
Log where exactly the context initialization failed, use histograms (5.53 KB, patch)
2011-09-29 11:41 PDT, Nate Chapin
no flags
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-
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
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
missed a case in CodeGeneratorV8.pm (8.78 KB, patch)
2011-10-05 10:33 PDT, Nate Chapin
no flags
Fix my mistakes in CodeGeneratorV8.pm (8.80 KB, patch)
2011-10-10 15:14 PDT, Nate Chapin
no flags
Nate Chapin
Comment 1 2011-09-14 13:19:35 PDT
WebKit Review Bot
Comment 2 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
Nate Chapin
Comment 3 2011-09-14 15:34:52 PDT
Created attachment 107409 [details] patch - fixed inverted logic and attempted cr-mac build fix
Adam Barth
Comment 4 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.
WebKit Review Bot
Comment 5 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>
WebKit Review Bot
Comment 6 2011-09-14 22:12:02 PDT
All reviewed patches have been landed. Closing bug.
Nate Chapin
Comment 7 2011-09-29 11:40:01 PDT
This got auto-closed by review bot....reopening and new logging patch incoming.
Nate Chapin
Comment 8 2011-09-29 11:41:07 PDT
Created attachment 109180 [details] Log where exactly the context initialization failed, use histograms
WebKit Review Bot
Comment 9 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>
WebKit Review Bot
Comment 10 2011-09-29 12:45:14 PDT
All reviewed patches have been landed. Closing bug.
Nate Chapin
Comment 11 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).
Nate Chapin
Comment 12 2011-10-04 13:53:46 PDT
Created attachment 109678 [details] Remove logging and always check the result of initContextIfNeeded()
WebKit Review Bot
Comment 13 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
WebKit Review Bot
Comment 14 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
Adam Barth
Comment 15 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.
Nate Chapin
Comment 16 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.
WebKit Review Bot
Comment 17 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
WebKit Review Bot
Comment 18 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
Nate Chapin
Comment 19 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.
Adam Barth
Comment 20 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.
Nate Chapin
Comment 21 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.
Adam Barth
Comment 22 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?
Nate Chapin
Comment 23 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.
WebKit Review Bot
Comment 24 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>
WebKit Review Bot
Comment 25 2011-10-12 11:45:01 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.