Bug 71229

Summary: V8MessageEvent::dataAccessorGetter does not return a reference to its caller
Product: WebKit Reporter: Dave Michael <dmichael>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, haraken, japhet, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Test that shows MessageEvent's data property getting corrupted.
none
Patch
none
Patch none

Description Dave Michael 2011-10-31 11:08:18 PDT
V8MessageEvent::dataAccessorGetter does not return a reference to its caller
Comment 1 Dave Michael 2011-10-31 11:08:46 PDT
Created attachment 113069 [details]
Patch
Comment 2 Adam Barth 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?
Comment 3 Adam Barth 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.
Comment 4 Dave Michael 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.
Comment 5 Dave Michael 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.
Comment 6 Adam Barth 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.)
Comment 7 Dave Michael 2011-10-31 14:04:06 PDT
Created attachment 113085 [details]
Patch
Comment 8 Kentaro Hara 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)
Comment 9 Dave Michael 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!
Comment 10 Adam Barth 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?
Comment 11 Dave Michael 2011-10-31 19:29:18 PDT
Created attachment 113122 [details]
Patch
Comment 12 Adam Barth 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.
Comment 13 WebKit Review Bot 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>
Comment 14 WebKit Review Bot 2011-10-31 20:38:10 PDT
All reviewed patches have been landed.  Closing bug.