WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
68978
message-event-constructor.html crashes and fails
https://bugs.webkit.org/show_bug.cgi?id=68978
Summary
message-event-constructor.html crashes and fails
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
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Kentaro Hara
Comment 1
2011-09-28 01:17:43 PDT
Created
attachment 108982
[details]
Patch
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
Created
attachment 110488
[details]
Patch
Kentaro Hara
Comment 7
2011-10-17 05:31:52 PDT
Created
attachment 111249
[details]
Patch
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
Created
attachment 111586
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug