1003432 - Exposed CustomEvent in Worker

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Arkadiusz Michalski (Spirit)]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140314220517

Steps to reproduce:

In accordance with:

http://dom.spec.whatwg.org/#interface-customevent
https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#idl-def-CustomEvent

CustomEvent interface should be exposed on Worker. Now only Event and EventTarget interface have correct behaviour.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

We should implement CustomEvent using webidl codegen and then exposing it in workers would be trivial.
Currently the implementation uses old codegen, so it is main-thread only.

One issue with using webidl codegen is that it would require C++ users to then use JSAPI, and not
Variant.

Comment 2

[u279076]

Marking as NEW based on comment 1.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

FWIW, CustomEvent doesn't use any codegen anymore. The main issue exposing CustomEvent in workers is the use of nsIVariant.
We could probably just store JS::Heap<JS::Value> and for C++ users in the main thread do the conversion to and from nsIVariant.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Need to then also trace the JS::Value member variable

Comment 5

[Jocelyn Liu [:jocelyn] [:joliu]]

I would like to try on this bug.

Comment 6

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [WIP] Bug 1003432: Expose CustomEvent in Worker.

This is just my first WIP patch without testing.

- s/nsCOMPtr<nsIVariant>/JS::Heap<JS::Value> for |CustomEvent::mDetail|.
- convert between JS::Value and nsIVariant in functions for C++ callers.

TODO: add a worker test case

Comment 7

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1003432: Expose CustomEvent in Worker.

- Add Hold/DropJSObjects
- Add a mochitest for CustomEvent in Worker.

Comment 8

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8730151 [details] [diff] [review]
Bug 1003432: Expose CustomEvent in Worker.

Hi Olli,

Could you help to review my patch for exposing CustomEvent in Worker?

Thanks,
Jocelyn

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8730151 [details] [diff] [review]
Bug 1003432: Expose CustomEvent in Worker.

>@@ -65,10 +74,18 @@ CustomEvent::InitCustomEvent(const nsAString& aType,
>                              bool aCanBubble,
>                              bool aCancelable,
>                              nsIVariant* aDetail)
> {
>+  AutoJSContext cx;
>+  JS::Rooted<JS::Value> detail(cx);
>+
>+  if (NS_WARN_IF(!VariantToJsval(cx, aDetail, &detail))) {
Exception handling is rather annoying, but looks like VariantToJsval may actually
put an exception to cx, so I think we should clear it here.
JS_ClearPendingException(aCx);

>+  mDetail = detail;
>   Event::InitEvent(aType, aCanBubble, aCancelable);
>-  mDetail = aDetail;

Nit, I would set mDetail to some value after Event::InitEvent. Just to have more consistent behavior where
base class is initialized first

>@@ -78,37 +95,31 @@ CustomEvent::InitCustomEvent(JSContext* aCx,
>                              bool aCancelable,
>                              JS::Handle<JS::Value> aDetail,
>                              ErrorResult& aRv)
...
>-  InitCustomEvent(aType, aCanBubble, aCancelable, detail);
>+  mDetail = aDetail;
>+  Event::InitEvent(aType, aCanBubble, aCancelable);
ditto


> CustomEvent::GetDetail(nsIVariant** aDetail)
> {
>-  NS_IF_ADDREF(*aDetail = mDetail);
>-  return NS_OK;
>+  AutoJSContext cx;
>+  JS::Rooted<JS::Value> detail(cx);
>+  nsIXPConnect* xpc = nsContentUtils::XPConnect();
>+
>+  if (NS_WARN_IF(!xpc || !ToJSValue(cx, mDetail, &detail))) {
>+    return NS_ERROR_FAILURE;
>+  }
what is the ToJSValue call about. mDetail is already JS::Value so why the call?
You may need to assign mDetail to detail, but I don't understand the ToJSValue call

>+addEventListener("foobar",
>+  function(evt) {
>+    postMessage(
>+      {
>+        type: evt.type,
>+        bubbles: evt.bubbles,
>+        cancelable: evt.cancelable,
>+        detail: evt.detail
>+      });
>+  }, true);
>+
>+addEventListener("message",
>+  function(evt) {
>+    var e = new CustomEvent("foobar");
>+    e.initCustomEvent("foobar", true, true, "test");
>+    dispatchEvent(e);
>+  }, true);
please test also CustomEvent ctor's dictionary, I mean the 2nd parameter to the constructor. In that case initCustomEvent shouldn't be used.
But anyhow, both initialization cases should be tested.

You need to also add CustomEvent to http://mxr.mozilla.org/mozilla-central/source/dom/workers/test/test_worker_interfaces.js

So, just small fixes, but I could take a look at another version of the patch.

Comment 10

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1003432(v2): Expose CustomEvent in Worker.

- address review comments

Comment 11

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8731042 [details] [diff] [review]
Bug 1003432(v2): Expose CustomEvent in Worker.

Hi Olli,

Thanks for your comments, I have addressed them in this patch.
Could you help to review it again?

Thanks,
Jocelyn

Comment 12

[Jocelyn Liu [:jocelyn] [:joliu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=20603a5d23e7

Comment 13

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #12)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=20603a5d23e7

I can see tests failed when calling |VariantToJsval| in |InitCustomEvent| for c++ callers.
Still trying to figure out the cause.
Below is the stack for running dom/base/test/test_bug590812.html.

Program received signal SIGSEGV, Segmentation fault.
xpc::CompartmentPrivate::Get (object=0x0)
    at /home/jocelyn/mozilla/gecko-dev/js/xpconnect/src/xpcprivate.h:3704
3704            JSCompartment* compartment = js::GetObjectCompartment(object);
(gdb) bt
#0  xpc::CompartmentPrivate::Get (object=0x0)
    at /home/jocelyn/mozilla/gecko-dev/js/xpconnect/src/xpcprivate.h:3704
#1  0x00007fffe839d2f7 in ObjectScope (obj=<optimized out>)
    at /home/jocelyn/mozilla/gecko-dev/js/xpconnect/src/xpcprivate.h:3843
#2  XPCConvert::NativeInterface2JSObject (d=..., dest=dest@entry=0x0, aHelper=..., 
    iid=iid@entry=0x7fffffffb3d0, Interface=Interface@entry=0x0, 
    allowNativeWrapper=allowNativeWrapper@entry=true, pErr=0x7fffffffb574)
    at /home/jocelyn/mozilla/gecko-dev/js/xpconnect/src/XPCConvert.cpp:771
#3  0x00007fffe83a057c in XPCConvert::NativeData2JS (d=..., s=s@entry=0x7fffffffb3b8, type=..., 
    iid=iid@entry=0x7fffffffb3d0, pErr=pErr@entry=0x7fffffffb574)
    at /home/jocelyn/mozilla/gecko-dev/js/xpconnect/src/XPCConvert.cpp:344
#4  0x00007fffe83be5a0 in XPCVariant::VariantDataToJS (variant=variant@entry=0x7fffb8eedb00, 
    pErr=pErr@entry=0x7fffffffb574, pJSVal=...)
    at /home/jocelyn/mozilla/gecko-dev/js/xpconnect/src/XPCVariant.cpp:547
#5  0x00007fffe917b843 in mozilla::dom::VariantToJsval (aCx=0x7fffe12d0c00, 
    aVariant=aVariant@entry=0x7fffb8eedb00, aRetval=..., aRetval@entry=...)
    at /home/jocelyn/mozilla/gecko-dev/dom/bindings/BindingUtils.cpp:1029
#6  0x00007fffe928a914 in mozilla::dom::CustomEvent::InitCustomEvent (this=0x7fffb9b77a20, 
    aType=..., aCanBubble=aCanBubble@entry=false, aCancelable=aCancelable@entry=false, 
    aDetail=aDetail@entry=0x7fffb8eedb00)
    at /home/jocelyn/mozilla/gecko-dev/dom/events/CustomEvent.cpp:81
#7  0x00007fffe98a4a1c in nsXMLPrettyPrinter::PrettyPrint (this=0x7fffbaff6310, aDocument=
    0x7fffb9bad000, aDidPrettyPrint=aDidPrettyPrint@entry=0x7fffffffb87f)
    at /home/jocelyn/mozilla/gecko-dev/dom/xml/nsXMLPrettyPrinter.cpp:170

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

That looks like null pointer error.
Does the CustomEvent::InitCustomEvent for C++ callers need to have a null check for aDetail before NS_WARN_IF(!VariantToJsval(cx, aDetail, &detail)) and if aDetail is null, explicitly
set detail to JS::NullValue().

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Hmm, but that caller is passing non-null variant. Not sure then. But that null check is needed anyhow, I think.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCConvert.cpp#771 is definitely buggy.
It explicitly uses JS::CurrentGlobalOrNull and then  ObjectScope which isn't null-safe.

But why we're getting null here. bholley could help here.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

It is possible that before VariantToJsval call one needs to have
AutoJSAPI jsapi;
NS_ENSURE_STATE(jsapi.Init(GetParentObject()));
so that xpconnect can find a global object to play with.

Comment 18

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Olli Pettay [:smaug] from comment #17)
> It is possible that before VariantToJsval call one needs to have
> AutoJSAPI jsapi;
> NS_ENSURE_STATE(jsapi.Init(GetParentObject()));
> so that xpconnect can find a global object to play with.

I think you were right.
test_bug590812.html can be passed by doing it.
Thanks a lot for pointing it out. :)
I will submit another try run to see how it goes.

BTW, should we add CustomEvent into dom/workers/test/serviceworkers/test_serviceworker_interface.js also?

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

test_serviceworker_interfaces.js to be precise. Looks like yes.
I wasn't aware of that test.

Comment 20

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1003432(v3): Expose CustomEvent in Worker. r=smaug

- add AutoJSAPI bit and null checking for aDetail in |InitCustomEvent|
- add CustomEvent into test_serviceworker_interfaces.js

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c57326a389b5

Olly, would you like to review my change again or I could land it directly after try passed?

Comment 21

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1003432(v4): Expose CustomEvent in Worker. r=smaug

- add AutoJSAPI in |GetDetail|.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4da2b388f2f9

Sorry, I missed the GetDetail bit in the previous patch.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8732089 [details] [diff] [review]
Bug 1003432(v4): Expose CustomEvent in Worker. r=smaug

 CustomEvent::CustomEvent(mozilla::dom::EventTarget* aOwner,
                          nsPresContext* aPresContext,
                          mozilla::WidgetEvent* aEvent)
-: Event(aOwner, aPresContext, aEvent)
+  : Event(aOwner, aPresContext, aEvent)
+  , mDetail(JS::UndefinedValue())
I realized this is a change to behavior. Initialize mDetail to NullValue(), not UndefinedValue.
Testcase for this is 
var e = document.createEvent("CustomEvent"); e.initEvent("foo", true, true); e.detail == null;



> NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(CustomEvent)
>@@ -65,10 +74,23 @@ CustomEvent::InitCustomEvent(const nsAString& aType,
>                              bool aCanBubble,
>                              bool aCancelable,
>                              nsIVariant* aDetail)
> {
>+  AutoJSAPI jsapi;
>+  NS_ENSURE_STATE(jsapi.Init(GetParentObject()));
>+  JSContext* cx = jsapi.cx();
>+  JS::Rooted<JS::Value> detail(cx);
>+
>+  if (!aDetail) {
>+    detail = JS::UndefinedValue();
not undefined, but null.

> CustomEvent::GetDetail(nsIVariant** aDetail)
> {
>-  NS_IF_ADDREF(*aDetail = mDetail);
>-  return NS_OK;
>+  AutoJSAPI jsapi;
>+  NS_ENSURE_STATE(jsapi.Init(GetParentObject()));
>+  JSContext* cx = jsapi.cx();
>+  JS::Rooted<JS::Value> detail(cx, mDetail);
>+  nsIXPConnect* xpc = nsContentUtils::XPConnect();
>+
>+  if (NS_WARN_IF(!xpc)) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  return xpc->JSToVariant(cx, detail, aDetail);
> }
I think in this method we could check if mDetail is null and if so, return null as aDetail.
That would be consistent with the current CustomEvent which stores nsIVariant, returns null if null was used when initializing the event.


Those fixed, r+

Comment 23

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Olli Pettay [:smaug] from comment #22)
> Comment on attachment 8732089 [details] [diff] [review]
> Bug 1003432(v4): Expose CustomEvent in Worker. r=smaug
> 
>  CustomEvent::CustomEvent(mozilla::dom::EventTarget* aOwner,
>                           nsPresContext* aPresContext,
>                           mozilla::WidgetEvent* aEvent)
> -: Event(aOwner, aPresContext, aEvent)
> +  : Event(aOwner, aPresContext, aEvent)
> +  , mDetail(JS::UndefinedValue())
> I realized this is a change to behavior. Initialize mDetail to NullValue(),
> not UndefinedValue.
> Testcase for this is 
> var e = document.createEvent("CustomEvent"); e.initEvent("foo", true, true);
> e.detail == null;
> 
Thanks for pointing out, should we add this test case into test_bug427537.html?
Is it needed to be tested also in test_bug1003432.html which is for worker case?

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Just add a test somewhere. Don't care too much where :)

Comment 25

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1003432(v5): Expose CustomEvent in Worker. r=smaug

Address comment 22.

Try looks fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcaa61fba65a

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9873db170eaa

Comment 27

[Jocelyn Liu [:jocelyn] [:joliu]]

Thanks a lot for all the help here, Kyle and smaug. :)

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9873db170eaa

Comment 29

[Chris Mills [:cmills]]

I've indicated that CustomEvent is now available to web workers in the following places:

https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent
https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent/CustomEvent
https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Functions_and_classes_available_to_workers

I've also added a note about it to the relevant developer release notes

https://developer.mozilla.org/en-US/Firefox/Releases/48#Others