RESOLVED FIXED 71229
V8MessageEvent::dataAccessorGetter does not return a reference to its caller
https://bugs.webkit.org/show_bug.cgi?id=71229
Summary V8MessageEvent::dataAccessorGetter does not return a reference to its caller
Dave Michael
Reported 2011-10-31 11:08:18 PDT
V8MessageEvent::dataAccessorGetter does not return a reference to its caller
Attachments
Patch (1.42 KB, patch)
2011-10-31 11:08 PDT, Dave Michael
no flags
Test that shows MessageEvent's data property getting corrupted. (1.74 KB, text/html)
2011-10-31 13:06 PDT, Dave Michael
no flags
Patch (4.22 KB, patch)
2011-10-31 14:04 PDT, Dave Michael
no flags
Patch (4.53 KB, patch)
2011-10-31 19:29 PDT, Dave Michael
no flags
Dave Michael
Comment 1 2011-10-31 11:08:46 PDT
Adam Barth
Comment 2 2011-10-31 11:53:03 PDT
Comment on attachment 113069 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113069&action=review > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) I can haz test?
Adam Barth
Comment 3 2011-10-31 11:56:49 PDT
Comment on attachment 113069 [details] Patch This either needs a test or an explanation why a test is impossible. I this case, it seems like a test should be possible.
Dave Michael
Comment 4 2011-10-31 12:19:44 PDT
I have a manual test that demonstrates the problem when using pepper postmessage, which executes script via WebBindings to create & dispatch a MessageEvent. Without this patch, the passed string gets garbled about 1 in ~3000-6000 times. With this patch it succeeds. I haven't been able to reproduce it elsewhere. I can add that test to the bug so anybody who can build a trusted pepper plugin can reproduce, but I'd much rather have a decent automated test in WebKit. I'm still a total n00b in WebKit/V8 land. A couple of questions: (1) Does this look like I'm on the right track? (2) Do you have a suggestion as to how to test this? I'm looking at LayoutTests/fast/events right now, but it would seem easier to test directly on top of the C++, but I'm not sure where to put such a test.
Dave Michael
Comment 5 2011-10-31 13:06:10 PDT
Created attachment 113075 [details] Test that shows MessageEvent's data property getting corrupted. Open the javascript console if you want to see the mismatched strings. Open the page. The renderer will usually crash. What this test does is create 10000 events in sequence, each with a unique and long string for its data contents. Each message is dispatched. We check that the received string matches the one we sent. For me, about 1 in 1500 times, the string does not match. The renderer also seems to crash pretty reliably. I'll try to massage this in to a layout test so it can be checked in.
Adam Barth
Comment 6 2011-10-31 13:11:47 PDT
> (1) Does this look like I'm on the right track? Yes, the patch looks fine. (Sorry, distracted at TPAC and can't answer your testing questions.)
Dave Michael
Comment 7 2011-10-31 14:04:06 PDT
Kentaro Hara
Comment 8 2011-10-31 15:53:35 PDT
Thank you very much for fixing the regression. The change looks good to me. By the way, what makes the test result non-deterministic? The timing of GC? If so, isn't it possible to make the test more deterministic by triggering GC after each MessageEvent? (c.f. How to trigger GC: http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/LayoutTests/fast/dom/resources/script-element-gc.js&exact_package=chromium&q=%22gc()%22%20layouttests&type=cs)
Dave Michael
Comment 9 2011-10-31 16:05:19 PDT
I tried that, and it didn't seem to make it fail any more consistently for me. I can try it again, but if it looks good enough now, we really need it committed to make the NaCl tests start passing again. Thanks guys for looking!
Adam Barth
Comment 10 2011-10-31 16:36:36 PDT
Comment on attachment 113085 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113085&action=review > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) This line will prevent the patch from being landed. You have a test now, so referencing it is probably the thing to do. > LayoutTests/fast/events/dispatch-message-string-data.html:6 > +<script> I would have expected a call to layoutTestController.dumpAsText() and layoutTestController.waitUntilDone(), etc. These are usually guarded by checking if (window.layoutTestController) . Maybe there here and I don't see them?
Dave Michael
Comment 11 2011-10-31 19:29:18 PDT
Adam Barth
Comment 12 2011-10-31 19:31:39 PDT
Comment on attachment 113122 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113122&action=review > Source/WebCore/ChangeLog:8 > + V8MessageEvent::dataAccessorGetter does not return a reference to its caller > + https://bugs.webkit.org/show_bug.cgi?id=71229 > + > + Reviewed by Adam Barth. > + > + Test: fast/events/dispatch-message-string-data.html Generally we prefer more of an explanation of why you're making this change. Often this duplicates information in the bug comments, but helps folks who stumble upon this commit later.
WebKit Review Bot
Comment 13 2011-10-31 20:38:05 PDT
Comment on attachment 113122 [details] Patch Clearing flags on attachment: 113122 Committed r98934: <http://trac.webkit.org/changeset/98934>
WebKit Review Bot
Comment 14 2011-10-31 20:38:10 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.