Closed Bug 1058156 Opened 6 years ago Closed 6 years ago

Push functionality with ServiceWorkers

Categories

(Core :: DOM: Push Notifications, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1038811

People

(Reporter: tysmith, Unassigned)

References

Details

Including the exposure of pushRegistrationManager on ServiceWorkerRegistrations and exposing the onpush event handler
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();
+}
+
Flags: needinfo?(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++"?
Flags: needinfo?(bzbarsky)
Depends on: 1038811
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 :)
Flags: needinfo?(nsm.nikhil)
Ah, wait, I see, the generated code does the wrapping.  That makes sense.
Flags: needinfo?(nsm.nikhil)
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?
Flags: needinfo?(tylsmith)
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(tylsmith)
I never tried with mWindow on my machine. You could just have had a bad build or something then.
Okay, well since I can't reproduce that error with mWindow, I guess doing it without the manual wrap should be fine.
> 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.
Flags: needinfo?(bzbarsky)
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1038811
You need to log in before you can comment on or make changes to this bug.