Closed Bug 1003432 Opened 10 years ago Closed 8 years ago

Exposed CustomEvent in Worker

Categories

(Core :: DOM: Events, defect)

Product:
Core
Component:
DOM: Events
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox48 --- fixed

People

(Reporter: crimsteam, Assigned: yrliou, Mentored)

Details

(Keywords: dev-doc-complete, Whiteboard: [tw-dom] btpp-active)

Attachments

(1 file, 5 obsolete files)

Bug 1003432(v5): Expose CustomEvent in Worker. r=smaug
11.10 KB, patch
Details | Diff | Splinter Review 
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.
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.
Marking as NEW based on comment 1.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 31 Branch → Trunk
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.
Need to then also trace the JS::Value member variable
I would like to try on this bug.
Assignee: nobody → joliu
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
Whiteboard: [tw-dom] → [tw-dom] btpp-active
- Add Hold/DropJSObjects
- Add a mochitest for CustomEvent in Worker.
Attachment #8729453 - Attachment is obsolete: true
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
Attachment #8730151 - Attachment description: [WIPv2]Bug 1003432: Expose CustomEvent in Worker. → Bug 1003432: Expose CustomEvent in Worker.
Attachment #8730151 - Flags: review?(bugs)
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.
Attachment #8730151 - Flags: review?(bugs) → review-
- address review comments
Attachment #8730151 - Attachment is obsolete: true
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
Attachment #8731042 - Flags: review?(bugs)
Attachment #8731042 - Flags: review?(bugs) → review+
(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
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().
Hmm, but that caller is passing non-null variant. Not sure then. But that null check is needed anyhow, I think.
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.
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.
(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?
test_serviceworker_interfaces.js to be precise. Looks like yes.
I wasn't aware of that test.
- 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?
Attachment #8731042 - Attachment is obsolete: true
- add AutoJSAPI in |GetDetail|.

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

Sorry, I missed the GetDetail bit in the previous patch.
Attachment #8732051 - Attachment is obsolete: true
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+
Attachment #8732089 - Flags: review+
(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?
Just add a test somewhere. Don't care too much where :)
Thanks a lot for all the help here, Kyle and smaug. :)
https://hg.mozilla.org/mozilla-central/rev/9873db170eaa
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: