Bug 68978 - message-event-constructor.html crashes and fails
Summary: message-event-constructor.html crashes and fails
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-28 00:09 PDT by Kentaro Hara
Modified: 2011-10-19 23:01 PDT (History)
8 users (show)

See Also:


Attachments
Failed test cases (9.93 KB, text/html)
2011-09-28 00:09 PDT, Kentaro Hara
no flags Details
Patch (23.71 KB, patch)
2011-09-28 01:17 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (24.62 KB, patch)
2011-10-11 02:14 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (23.21 KB, patch)
2011-10-17 05:31 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (25.76 KB, patch)
2011-10-19 03:25 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2011-09-28 00:09:24 PDT
Created attachment 108975 [details]
Failed test cases

I added message-event-constructor.html in the bug 68883, but some tests are crashing or failing. This bug is similar to the bug 68345.

- The following test cases are failing, as you can see in the attached file. The IDL type of MessageEvent.data should be 'any' (http://www.whatwg.org/specs/web-apps/current-work/#messageevent), and the IDL spec of 'any' (http://www.w3.org/TR/WebIDL/#es-any) says that the following test cases should pass.

    shouldBe("new MessageEvent('eventType', { data: test_object }).data", "test_object");
    shouldBe("new MessageEvent('eventType', { bubbles: true, cancelable: true, data: test_object, origin: 'wonderful', lastEventId: 'excellent', source: window, ports: [channel.port1, channel.port2, channel.port2] }).data", "test_object");

- When we pass a DOM object (i.e. an unserializable object) as follows, then DRT crashes. This crash happens only in DRT.

    shouldBe("new MessageEvent('eventType', { data: document }).data", "document");

The reason for these failures and crash is that MessageEvent.data is implemented just as SerializedScriptValue, and thus it cannot handle ScriptValue.
Comment 1 Kentaro Hara 2011-09-28 01:17:43 PDT
Created attachment 108982 [details]
Patch
Comment 2 Hajime Morrita 2011-10-02 23:06:29 PDT
Comment on attachment 108982 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108982&action=review

> Source/WebCore/dom/MessageEvent.h:128
>      RefPtr<SerializedScriptValue> m_dataAsSerializedScriptValue;

Do we need this even after introducing m_dataAsScriptValue?
Comment 3 Kentaro Hara 2011-10-02 23:19:19 PDT
Comment on attachment 108982 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108982&action=review

>> Source/WebCore/dom/MessageEvent.h:128
>>      RefPtr<SerializedScriptValue> m_dataAsSerializedScriptValue;
> 
> Do we need this even after introducing m_dataAsScriptValue?

Yes. We need to keep the following factory method, which stores MessageEvent.data into m_dataAsSerializedScriptValue.

static PassRefPtr<MessageEvent> create(PassOwnPtr<MessagePortArray> ports, PassRefPtr<SerializedScriptValue> data\
, const String& origin = "", const String& lastEventId = "", PassRefPtr<DOMWindow> source = 0);

This API can be called from the context that has SerializedScriptValue but cannot convert it into ScriptValue since the context does not have ExecState. For example, WebCore/workers/WorkerMessagingProxy.cpp and WebKit/chromium/src/WebWorkerImpl.cpp are using this API. In this case, we convert the SerializedScriptValue into ScriptValue when MessageEvent.data is called (At this point, we have ExecState).
Comment 4 Hajime Morrita 2011-10-05 22:31:13 PDT
> static PassRefPtr<MessageEvent> create(PassOwnPtr<MessagePortArray> ports, PassRefPtr<SerializedScriptValue> data\
> , const String& origin = "", const String& lastEventId = "", PassRefPtr<DOMWindow> source = 0);
> 
> This API can be called from the context that has SerializedScriptValue but cannot convert it into ScriptValue since the context does not have ExecState. For example, WebCore/workers/WorkerMessagingProxy.cpp and WebKit/chromium/src/WebWorkerImpl.cpp are using this API. In this case, we convert the SerializedScriptValue into ScriptValue when MessageEvent.data is called (At this point, we have ExecState).
I got it.
So it looks we should have a test that create and send an MessageEvent with non-serializable object data
to worker thread, which enforces a tricky serialization.
Comment 5 Kentaro Hara 2011-10-11 02:13:51 PDT
(In reply to comment #4)
> > static PassRefPtr<MessageEvent> create(PassOwnPtr<MessagePortArray> ports, PassRefPtr<SerializedScriptValue> data\
> > , const String& origin = "", const String& lastEventId = "", PassRefPtr<DOMWindow> source = 0);
> > 
> > This API can be called from the context that has SerializedScriptValue but cannot convert it into ScriptValue since the context does not have ExecState. For example, WebCore/workers/WorkerMessagingProxy.cpp and WebKit/chromium/src/WebWorkerImpl.cpp are using this API. In this case, we convert the SerializedScriptValue into ScriptValue when MessageEvent.data is called (At this point, we have ExecState).
> I got it.
> So it looks we should have a test that create and send an MessageEvent with non-serializable object data
> to worker thread, which enforces a tricky serialization.

I guess that it is impossible to write test cases where we send non-serializable data to a worker thread by postMessage() and then the worker thread constructs MessageEvent with SerializedScriptValue. This is because if we pass non-serializable data to postMessage(), postMessage() tries to serialize the non-serializable data, resulting in TypeError in browser or a crash in DRT. In other words, we cannot send non-serializable data to the worker thread by postMessage() in the first place, which makes impossible to write the test cases we want. 

Instead, I added ASSERT() to MessageEvent.dataXXXX() in order to guarantee that MessageEvent.dataXXXX() is not called for wrong type of data.
Comment 6 Kentaro Hara 2011-10-11 02:14:24 PDT
Created attachment 110488 [details]
Patch
Comment 7 Kentaro Hara 2011-10-17 05:31:52 PDT
Created attachment 111249 [details]
Patch
Comment 8 WebKit Review Bot 2011-10-17 18:49:15 PDT
Comment on attachment 111249 [details]
Patch

Clearing flags on attachment: 111249

Committed r97697: <http://trac.webkit.org/changeset/97697>
Comment 9 WebKit Review Bot 2011-10-17 18:49:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Kentaro Hara 2011-10-18 00:19:37 PDT
Reverted r97697 for reason:

some tests are broken

Committed r97725: <http://trac.webkit.org/changeset/97725>
Comment 11 Kentaro Hara 2011-10-19 03:25:25 PDT
Created attachment 111586 [details]
Patch
Comment 12 Kentaro Hara 2011-10-19 03:27:11 PDT
I am sorry for the reversion.

The reason for failures is that the change for JSMessageEvent::handleInitMessageEvent() in JSMessageEventCustom.cpp and V8MessageEvent::initMessageEventCallback() in V8MessageEventCustom.cpp was not enough. 

- I fixed these two methods.
- I removed a 'doTransfer' parameter from JSMessageEvent::handleInitMessageEvent(), since the 'doTransfer' is no longer used.

I confirmed that the failing tests (fast/dom/message-port-deleted-by-accessor.html, fast/events/init-events.html, fast/eventsource/eventsource-attribute-listeners.html) pass both on JSC and on V8. Would you please take a look?
Comment 13 Hajime Morrita 2011-10-19 21:54:12 PDT
Comment on attachment 111586 [details]
Patch

let's retry.
Comment 14 WebKit Review Bot 2011-10-19 23:01:51 PDT
Comment on attachment 111586 [details]
Patch

Clearing flags on attachment: 111586

Committed r97939: <http://trac.webkit.org/changeset/97939>
Comment 15 WebKit Review Bot 2011-10-19 23:01:57 PDT
All reviewed patches have been landed.  Closing bug.