message-event-constructor.html crashes and fails

• Component: DOM
• Status: RESOLVED FIXED

[Kentaro Hara]

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.

[Kentaro Hara]

Created attachment 108982 [details]
Patch

[Hajime Morrita]

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?

[Kentaro Hara]

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

[Hajime Morrita]

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

[Kentaro Hara]

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

[Kentaro Hara]

Created attachment 110488 [details]
Patch

[Kentaro Hara]

Created attachment 111249 [details]
Patch

[WebKit Review Bot]

Comment on attachment 111249 [details]
Patch

Clearing flags on attachment: 111249

Committed r97697: <http://trac.webkit.org/changeset/97697>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

Reverted r97697 for reason:

some tests are broken

Committed r97725: <http://trac.webkit.org/changeset/97725>

[Kentaro Hara]

Created attachment 111586 [details]
Patch

[Kentaro Hara]

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?

[Hajime Morrita]

Comment on attachment 111586 [details]
Patch

let's retry.

[WebKit Review Bot]

Comment on attachment 111586 [details]
Patch

Clearing flags on attachment: 111586

Committed r97939: <http://trac.webkit.org/changeset/97939>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.