Bug 71229 – V8MessageEvent::dataAccessorGetter does not return a reference to its caller

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dave Michael]

V8MessageEvent::dataAccessorGetter does not return a reference to its caller

[Dave Michael]

Created attachment 113069 [details]
Patch

[Adam Barth]

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 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]

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]

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]

> (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]

Created attachment 113085 [details]
Patch

[Kentaro Hara]

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]

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 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]

Created attachment 113122 [details]
Patch

[Adam Barth]

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 on attachment 113122 [details]
Patch

Clearing flags on attachment: 113122

Committed r98934: <http://trac.webkit.org/changeset/98934>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.