Bug 68978

Summary: message-event-constructor.html crashes and fails
Product: WebKit Reporter: Kentaro Hara <haraken>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, cmarcelo, dominicc, japhet, morrita, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Failed test cases
none
Patch
none
Patch
none
Patch
none
Patch none

Kentaro Hara
Reported 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.
Attachments
Failed test cases (9.93 KB, text/html)
2011-09-28 00:09 PDT, Kentaro Hara
no flags
Patch (23.71 KB, patch)
2011-09-28 01:17 PDT, Kentaro Hara
no flags
Patch (24.62 KB, patch)
2011-10-11 02:14 PDT, Kentaro Hara
no flags
Patch (23.21 KB, patch)
2011-10-17 05:31 PDT, Kentaro Hara
no flags
Patch (25.76 KB, patch)
2011-10-19 03:25 PDT, Kentaro Hara
no flags
Kentaro Hara
Comment 1 2011-09-28 01:17:43 PDT
Hajime Morrita
Comment 2 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?
Kentaro Hara
Comment 3 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).
Hajime Morrita
Comment 4 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.
Kentaro Hara
Comment 5 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.
Kentaro Hara
Comment 6 2011-10-11 02:14:24 PDT
Kentaro Hara
Comment 7 2011-10-17 05:31:52 PDT
WebKit Review Bot
Comment 8 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>
WebKit Review Bot
Comment 9 2011-10-17 18:49:20 PDT
All reviewed patches have been landed. Closing bug.
Kentaro Hara
Comment 10 2011-10-18 00:19:37 PDT
Reverted r97697 for reason: some tests are broken Committed r97725: <http://trac.webkit.org/changeset/97725>
Kentaro Hara
Comment 11 2011-10-19 03:25:25 PDT
Kentaro Hara
Comment 12 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?
Hajime Morrita
Comment 13 2011-10-19 21:54:12 PDT
Comment on attachment 111586 [details] Patch let's retry.
WebKit Review Bot
Comment 14 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>
WebKit Review Bot
Comment 15 2011-10-19 23:01:57 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.