Bug 71229

Created attachment 113069 [details] Patch

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?

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.

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.

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.

> (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.)

Created attachment 113085 [details] Patch

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)

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!

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?

Created attachment 113122 [details] Patch

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.

Comment on attachment 113122 [details] Patch Clearing flags on attachment: 113122 Committed r98934: <http://trac.webkit.org/changeset/98934>

All reviewed patches have been landed. Closing bug.