1058156 - Push functionality with ServiceWorkers

Status: RESOLVED DUPLICATE of bug 1038811

(Core :: DOM: Push Subscriptions, defect)

Description

[Tyler Smith [:tysmith]]

Including the exposure of pushRegistrationManager on ServiceWorkerRegistrations and exposing the onpush event handler

Comment 1

[Tyler Smith [:tysmith]]

PushManager, which is implemented in JS, was originally exposed on Navigator by specifying NavigatorProperty in webidl.  Now we're exposing it on SWRegistrations (which are C++) but there doesn't seem to be an overly elegant way of exposing JS stuff on C++ stuff.  Nikhil came up with this as a solution but wanted feedback from you.  

diff --git a/dom/webidl/ServiceWorkerRegistration.webidl b/dom/webidl/ServiceWorkerRegistration.webidl
index d383f71..b4e1299 100644
--- a/dom/webidl/ServiceWorkerRegistration.webidl
+++ b/dom/webidl/ServiceWorkerRegistration.webidl
@@ -23,5 +23,6 @@ interface ServiceWorkerRegistration : EventTarget {
   // event
   attribute EventHandler onupdatefound;
 
-  readonly attribute PushManager pushRegistrationManager;
-};
\ No newline at end of file
+  [Throws]
+  readonly attribute object pushRegistrationManager;
+};
diff --git a/dom/workers/ServiceWorkerRegistration.cpp b/dom/workers/ServiceWorkerRegistration.cpp
index 7561706..a88b4c9 100644
--- a/dom/workers/ServiceWorkerRegistration.cpp
+++ b/dom/workers/ServiceWorkerRegistration.cpp
@@ -247,5 +247,50 @@ ServiceWorkerRegistration::Observe(nsISupports* aSubject, const char* aTopic,
 
   return NS_OK;
 }
+
+void
+ServiceWorkerRegistration::GetPushRegistrationManager(JSContext* aCx, JS::MutableHandle<JSObject*> aOut, ErrorResult& aRv)
+{
+  nsRefPtr<PushManager> pm = PushRegistrationManager(aRv);
+  if (aRv.Failed()) {
+    return;
+  }
+  JS::Rooted<JS::Value> v(aCx);
+  if (!WrapNewBindingObject(aCx, pm, &v)) {
+    //XXX Assertion disabled for now, see bug 991271.
+    MOZ_ASSERT(true || JS_IsExceptionPending(aCx));
+    //XXXnsm: What do we do here? ThrowMethodFailedWithDetails?
+    return;
+  }
+  aOut.set(&v.toObject());
+}
+
+already_AddRefed<PushManager>
+ServiceWorkerRegistration::PushRegistrationManager(ErrorResult& aRv)
+{
+  if(mPushRegistrationManager == nullptr){
+    MOZ_ASSERT(mWindow);
+    //nsCOMPtr<nsIGlobalObject> globalObject = do_QueryInterface(mWindow);
+    nsCOMPtr<nsIGlobalObject> globalObject(GetEntryGlobal());
+    AutoJSAPI jsapi;
+    jsapi.Init(globalObject);
+    JSContext* cx = jsapi.cx();
+
+    JS::RootedObject globalJs(cx, globalObject->GetGlobalJSObject());
+    GlobalObject global(cx, globalJs);
+
+    JS::Rooted<JSObject*> jsImplObj(cx);
+    nsCOMPtr<nsPIDOMWindow> window =
+    ConstructJSImplementation(cx, "@mozilla.org/push/PushManager;1", global, &jsImplObj, aRv);
+    if (aRv.Failed()) {
+      return nullptr;
+    }
+    // Build the C++ implementation.
+    mPushRegistrationManager = new PushManager(jsImplObj, window);
+  }
+  nsRefPtr<PushManager> ret = mPushRegistrationManager;
+  return ret.forget();
+}
+

Comment 2

[Boris Zbarsky [:bzbarsky]]

The IDL should just return a PushManager, no?  You should not need to do WrapNewBindingObject manually!

Past that, did you read the mail from me to mccr8 you were cced on, dated 2014-08-21, subject "Re: manipulating JS-implemented WebIDL from C++"?

Comment 3

[Tyler Smith [:tysmith]]

It seems to me like the IDL eventually wants a JS representation of PushManager, and since we can only invoke the C++ constructor, it makes sense to have to wrap it manually?  

I'm not super clear on this though.  Maybe Nikhil can shed some light.  

And yeah, I read the email :)

Comment 4

[Tyler Smith [:tysmith]]

Ah, wait, I see, the generated code does the wrapping.  That makes sense.

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

So the problem with using GetEntryGlobal() was that under certain situations (from what I saw that day, having devtools open), the global would be a SandboxPrivate and not a window. In that case, ConstructJSImplementation() would fail, and the auto generated code would attempt to wrap a nullptr, resulting in a crash.

So I added the manual binding so I can check the ErrorResult before I call that. That said, this problem may not occur if mWindow is used. Tyler can you try that?

Comment 6

[Tyler Smith [:tysmith]]

Running this 

  already_AddRefed<PushManager>
  PushRegistrationManager(){
    if(mPushRegistrationManager == nullptr){
      MOZ_ASSERT(mWindow);
      nsCOMPtr<nsIGlobalObject> globalObject = do_QueryInterface(mWindow);
      // nsCOMPtr<nsIGlobalObject> globalObject(GetEntryGlobal());
      // AutoJSAPI jsapi;
      // if (NS_WARN_IF(!jsapi.Init(globalObject))) {
      //   return nullptr;
      // }
      // JSContext* cx = jsapi.cx();
      AutoJSContext cx;

      JS::RootedObject globalJs(cx, globalObject->GetGlobalJSObject());
      GlobalObject global(cx, globalJs);
      ErrorResult aRv;

      JS::Rooted<JSObject*> jsImplObj(cx);
      nsCOMPtr<nsPIDOMWindow> window =
      ConstructJSImplementation(cx, "@mozilla.org/push/PushManager;1", global, &jsImplObj, aRv);
      if (aRv.Failed()) {
        return nullptr;
      }
      // Build the C++ implementation.
      mPushRegistrationManager = new PushManager(jsImplObj, window);
    }
    return mPushRegistrationManager.forget();
  }

Seems to work, but I thought we had found on Friday that it didn't?

This version doesn't have [Throws] for pushRegistrationManager.

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I never tried with mWindow on my machine. You could just have had a bad build or something then.

Comment 8

[Tyler Smith [:tysmith]]

Okay, well since I can't reproduce that error with mWindow, I guess doing it without the manual wrap should be fine.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> So the problem with using GetEntryGlobal()

This code should NOT be using GetEntryGlobal.  That's almost always the wrong thing to use; it should only be used if a spec explicitly tells you to use it.

What you presumably want to use is the global of the ServiceWorkerRegistration or better yet the global of the Realm of the called function (which is much like the current compartment, but not quite in the Xray case).  Unfortunately, we have no sane way of getting the current-Realm global right now.  :(

> So I added the manual binding so I can check the ErrorResult before I call that.

Shouldn't be needed, as long as you propagate back the failing aRv.