Bug 1183954 - Fix Notification.data structured cloning on workers. r=robertbindar,mccr8
authorNikhil Marathe <nsm.nikhil@gmail.com>
Thu, 30 Jul 2015 12:44:14 -0700
changeset 287437 02652b4367625e2771366b18bf2e95753c240e26
parent 287436 a48677d48570e2e6158f4d0f4ef012466320fbfb
child 287438 b080520b4a151b8ee6beda1adc71af2c8626f138
push id5067
push userraliiev@mozilla.com
push dateMon, 21 Sep 2015 14:04:52 +0000
treeherdermozilla-beta@14221ffe5b2f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrobertbindar, mccr8
bugs1183954
milestone42.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1183954 - Fix Notification.data structured cloning on workers. r=robertbindar,mccr8 Rather than store a non-thread-safe refcounted nsIStructuredCloneContainer, store the base64 representation. Caches a jsval the first time an attempt to access data is made from content script.
dom/base/nsStructuredCloneContainer.cpp
dom/interfaces/base/nsIStructuredCloneContainer.idl
dom/notification/Notification.cpp
dom/notification/Notification.h
dom/workers/test/notification_worker.js
dom/workers/test/serviceworkers/notificationclick.html
dom/workers/test/serviceworkers/notificationclick.js
dom/workers/test/serviceworkers/test_notificationclick.html
--- a/dom/base/nsStructuredCloneContainer.cpp
+++ b/dom/base/nsStructuredCloneContainer.cpp
@@ -90,35 +90,47 @@ nsStructuredCloneContainer::InitFromBase
   NS_ENSURE_STATE(mData);
   memcpy(mData, binaryData.get(), binaryData.Length());
 
   mSize = binaryData.Length();
   mVersion = aFormatVersion;
   return NS_OK;
 }
 
+nsresult
+nsStructuredCloneContainer::DeserializeToJsval(JSContext* aCx,
+                                               JS::MutableHandle<JS::Value> aValue)
+{
+  aValue.setNull();
+  JS::Rooted<JS::Value> jsStateObj(aCx);
+  bool hasTransferable = false;
+  bool success = JS_ReadStructuredClone(aCx, mData, mSize, mVersion,
+                                        &jsStateObj, nullptr, nullptr) &&
+                 JS_StructuredCloneHasTransferables(mData, mSize,
+                                                    &hasTransferable);
+  // We want to be sure that mData doesn't contain transferable objects
+  MOZ_ASSERT(!hasTransferable);
+  NS_ENSURE_STATE(success && !hasTransferable);
+
+  aValue.set(jsStateObj);
+  return NS_OK;
+}
 
 nsresult
 nsStructuredCloneContainer::DeserializeToVariant(JSContext *aCx,
                                                  nsIVariant **aData)
 {
   NS_ENSURE_STATE(mData);
   NS_ENSURE_ARG_POINTER(aData);
   *aData = nullptr;
 
   // Deserialize to a JS::Value.
   JS::Rooted<JS::Value> jsStateObj(aCx);
-  bool hasTransferable = false;
-  bool success = JS_ReadStructuredClone(aCx, mData, mSize, mVersion,
-                                        &jsStateObj, nullptr, nullptr) &&
-                 JS_StructuredCloneHasTransferables(mData, mSize,
-                                                    &hasTransferable);
-  // We want to be sure that mData doesn't contain transferable objects
-  MOZ_ASSERT(!hasTransferable);
-  NS_ENSURE_STATE(success && !hasTransferable);
+  nsresult rv = DeserializeToJsval(aCx, &jsStateObj);
+  NS_ENSURE_SUCCESS(rv, rv);
 
   // Now wrap the JS::Value as an nsIVariant.
   nsCOMPtr<nsIVariant> varStateObj;
   nsCOMPtr<nsIXPConnect> xpconnect = do_GetService(nsIXPConnect::GetCID());
   NS_ENSURE_STATE(xpconnect);
   xpconnect->JSValToVariant(aCx, jsStateObj, getter_AddRefs(varStateObj));
   NS_ENSURE_STATE(varStateObj);
 
--- a/dom/interfaces/base/nsIStructuredCloneContainer.idl
+++ b/dom/interfaces/base/nsIStructuredCloneContainer.idl
@@ -22,17 +22,17 @@ interface nsIDocument;
  * initFromJSVal or initFromBase64.  It's an error to initialize an
  * nsIStructuredCloneContainer more than once.
  *
  * Once you've initialized the container, you can get a copy of the object it
  * stores by calling deserializeToVariant.  You can also get a base-64-encoded
  * string containing a copy of the container's serialized data, using
  * getDataAsBase64.
  */
-[scriptable, uuid(63eeafec-63f5-42c3-aea9-5c04678784e7)]
+[scriptable, uuid(c664aae7-0d67-4155-a2dd-a3861778626f)]
 interface nsIStructuredCloneContainer : nsISupports
 {
   /**
    * Initialize this structured clone container so it contains a clone of the
    * given jsval.
    */
   [noscript, implicit_jscontext]
   void initFromJSVal(in jsval aData);
@@ -41,18 +41,26 @@ interface nsIStructuredCloneContainer : 
    * Initialize this structured clone container from a base-64-encoded byte
    * stream, stored in aData.  aFormatVersion should be the version of the
    * structured clone algorithm which was used to generate aData.
    */
   [implicit_jscontext]
   void initFromBase64(in AString aData,in unsigned long aFormatVersion);
 
   /**
+   * Deserializes this structured clone container returning it as a jsval.
+   * Can be called on main and worker threads.
+   */
+  [implicit_jscontext]
+  jsval deserializeToJsval();
+
+  /**
    * Deserialize the object this container holds, returning it wrapped as
    * an nsIVariant.
+   * Main thread only!
    */
   [implicit_jscontext]
   nsIVariant deserializeToVariant();
 
   /**
    * Get this structured clone container's data as a base-64-encoded string.
    */
   AString getDataAsBase64();
--- a/dom/notification/Notification.cpp
+++ b/dom/notification/Notification.cpp
@@ -691,18 +691,18 @@ Notification::IsGetEnabled(JSContext* aC
 Notification::Notification(nsIGlobalObject* aGlobal, const nsAString& aID,
                            const nsAString& aTitle, const nsAString& aBody,
                            NotificationDirection aDir, const nsAString& aLang,
                            const nsAString& aTag, const nsAString& aIconUrl,
                            const NotificationBehavior& aBehavior)
   : DOMEventTargetHelper(),
     mWorkerPrivate(nullptr), mObserver(nullptr),
     mID(aID), mTitle(aTitle), mBody(aBody), mDir(aDir), mLang(aLang),
-    mTag(aTag), mIconUrl(aIconUrl), mBehavior(aBehavior), mIsClosed(false),
-    mIsStored(false), mTaskCount(0)
+    mTag(aTag), mIconUrl(aIconUrl), mBehavior(aBehavior), mData(JS::NullValue()),
+    mIsClosed(false), mIsStored(false), mTaskCount(0)
 {
   if (NS_IsMainThread()) {
     // We can only call this on the main thread because
     // Event::SetEventType() called down the call chain when dispatching events
     // using DOMEventTargetHelper::DispatchTrustedEvent() will assume the event
     // is a main thread event if it has a valid owner. It will then attempt to
     // fetch the atom for the event name which asserts main thread only.
     BindToOwner(aGlobal);
@@ -822,38 +822,31 @@ Notification::PersistNotification()
   MOZ_ASSERT(NS_SUCCEEDED(rv));
 
   nsString id;
   GetID(id);
 
   nsString alertName;
   GetAlertName(alertName);
 
-  nsString dataString;
-  nsCOMPtr<nsIStructuredCloneContainer> scContainer;
-  scContainer = GetDataCloneContainer();
-  if (scContainer) {
-    scContainer->GetDataAsBase64(dataString);
-  }
-
   nsAutoString behavior;
   if (!mBehavior.ToJSON(behavior)) {
     return NS_ERROR_FAILURE;
   }
 
   rv = notificationStorage->Put(origin,
                                 id,
                                 mTitle,
                                 DirectionToString(mDir),
                                 mLang,
                                 mBody,
                                 mTag,
                                 mIconUrl,
                                 alertName,
-                                dataString,
+                                mDataAsBase64,
                                 behavior,
                                 mScope);
 
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   SetStoredState(true);
@@ -908,32 +901,35 @@ Notification::CreateInternal(nsIGlobalOb
                                                          aOptions.mTag,
                                                          aOptions.mIcon,
                                                          aOptions.mMozbehavior);
   return notification.forget();
 }
 
 Notification::~Notification()
 {
+  mData.setUndefined();
+  mozilla::DropJSObjects(this);
   AssertIsOnTargetThread();
   MOZ_ASSERT(!mFeature);
   MOZ_ASSERT(!mTempRef);
 }
 
 NS_IMPL_CYCLE_COLLECTION_CLASS(Notification)
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(Notification, DOMEventTargetHelper)
-  NS_IMPL_CYCLE_COLLECTION_UNLINK(mData)
-  NS_IMPL_CYCLE_COLLECTION_UNLINK(mDataObjectContainer)
+  tmp->mData.setUndefined();
 NS_IMPL_CYCLE_COLLECTION_UNLINK_END
 
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(Notification, DOMEventTargetHelper)
-  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mData)
-  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mDataObjectContainer)
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
 
+NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(Notification, DOMEventTargetHelper)
+  NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK(mData);
+NS_IMPL_CYCLE_COLLECTION_TRACE_END
+
 NS_IMPL_ADDREF_INHERITED(Notification, DOMEventTargetHelper)
 NS_IMPL_RELEASE_INHERITED(Notification, DOMEventTargetHelper)
 
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(Notification)
 NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper)
 
 nsIPrincipal*
 Notification::GetPrincipal()
@@ -1389,23 +1385,16 @@ Notification::ShowInternal()
     // This observer does not care about the Notification. It will be released
     // at the end of this function.
     //
     // The observer is wholly owned by the alerts service.
     observer = new ServiceWorkerNotificationObserver(mScope, GetPrincipal(), mID);
   }
   MOZ_ASSERT(observer);
 
-  // mDataObjectContainer might be uninitialized here because the notification
-  // was constructed with an undefined data property.
-  nsString dataStr;
-  if (mDataObjectContainer) {
-    mDataObjectContainer->GetDataAsBase64(dataStr);
-  }
-
 #ifdef MOZ_B2G
   nsCOMPtr<nsIAppNotificationService> appNotifier =
     do_GetService("@mozilla.org/system-alerts-service;1");
   if (appNotifier) {
     uint32_t appId = nsIScriptSecurityManager::UNKNOWN_APP_ID;
     if (mWorkerPrivate) {
       appId = mWorkerPrivate->GetPrincipal()->GetAppId();
     } else {
@@ -1423,17 +1412,17 @@ Notification::ShowInternal()
         AppNotificationServiceOptions ops;
         ops.mTextClickable = true;
         ops.mManifestURL = manifestUrl;
         GetAlertName(ops.mId);
         ops.mDbId = mID;
         ops.mDir = DirectionToString(mDir);
         ops.mLang = mLang;
         ops.mTag = mTag;
-        ops.mData = dataStr;
+        ops.mData = mDataAsBase64;
         ops.mMozbehavior = mBehavior;
         ops.mMozbehavior.mSoundFile = soundUrl;
 
         if (!ToJSValue(cx, ops, &val)) {
           NS_WARNING("Converting dict to object failed!");
           return;
         }
 
@@ -1466,17 +1455,17 @@ Notification::ShowInternal()
     inPrivateBrowsing = loadContext && loadContext->UsePrivateBrowsing();
   }
 
   nsAutoString alertName;
   GetAlertName(alertName);
   alertService->ShowAlertNotification(iconUrl, mTitle, mBody, true,
                                       uniqueCookie, observer, alertName,
                                       DirectionToString(mDir), mLang,
-                                      dataStr, GetPrincipal(),
+                                      mDataAsBase64, GetPrincipal(),
                                       inPrivateBrowsing);
 }
 
 /* static */ bool
 Notification::RequestPermissionEnabledForScope(JSContext* aCx, JSObject* /* unused */)
 {
   // requestPermission() is not allowed on workers. The calling page should ask
   // for permission on the worker's behalf. This is to prevent 'which window
@@ -1972,62 +1961,83 @@ Notification::GetOrigin(nsIPrincipal* aP
       do_GetService("@mozilla.org/AppsService;1", &rv);
     NS_ENSURE_SUCCESS(rv, rv);
     appsService->GetManifestURLByLocalId(appId, aOrigin);
   }
 
   return NS_OK;
 }
 
-nsIStructuredCloneContainer* Notification::GetDataCloneContainer()
-{
-  return mDataObjectContainer;
-}
-
 void
 Notification::GetData(JSContext* aCx,
                       JS::MutableHandle<JS::Value> aRetval)
 {
-  if (!mData && mDataObjectContainer) {
+  if (mData.isNull() && !mDataAsBase64.IsEmpty()) {
     nsresult rv;
-    rv = mDataObjectContainer->DeserializeToVariant(aCx, getter_AddRefs(mData));
+    auto container = new nsStructuredCloneContainer();
+    rv = container->InitFromBase64(mDataAsBase64, JS_STRUCTURED_CLONE_VERSION,
+                                   aCx);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       aRetval.setNull();
       return;
     }
+
+    JS::Rooted<JS::Value> data(aCx);
+    rv = container->DeserializeToJsval(aCx, &data);
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      aRetval.setNull();
+      return;
+    }
+
+    if (data.isGCThing()) {
+      mozilla::HoldJSObjects(this);
+    }
+    mData = data;
   }
-  if (!mData) {
+  if (mData.isNull()) {
     aRetval.setNull();
     return;
   }
-  VariantToJsval(aCx, mData, aRetval);
+
+  JS::ExposeValueToActiveJS(mData);
+  aRetval.set(mData);
 }
 
 void
 Notification::InitFromJSVal(JSContext* aCx, JS::Handle<JS::Value> aData,
                             ErrorResult& aRv)
 {
-  if (mDataObjectContainer || aData.isNull()) {
+  if (!mDataAsBase64.IsEmpty() || aData.isNull()) {
     return;
   }
-  mDataObjectContainer = new nsStructuredCloneContainer();
-  aRv = mDataObjectContainer->InitFromJSVal(aData, aCx);
+  auto dataObjectContainer = new nsStructuredCloneContainer();
+  aRv = dataObjectContainer->InitFromJSVal(aData, aCx);
+  if (NS_WARN_IF(aRv.Failed())) {
+    return;
+  }
+
+  dataObjectContainer->GetDataAsBase64(mDataAsBase64);
 }
 
 void Notification::InitFromBase64(JSContext* aCx, const nsAString& aData,
                                   ErrorResult& aRv)
 {
-  if (mDataObjectContainer || aData.IsEmpty()) {
+  if (!mDataAsBase64.IsEmpty() || aData.IsEmpty()) {
     return;
   }
 
+  // To and fro to ensure it is valid base64.
   auto container = new nsStructuredCloneContainer();
   aRv = container->InitFromBase64(aData, JS_STRUCTURED_CLONE_VERSION,
                                   aCx);
-  mDataObjectContainer = container;
+  if (NS_WARN_IF(aRv.Failed())) {
+    return;
+  }
+
+  container->GetDataAsBase64(mDataAsBase64);
 }
 
 bool
 Notification::AddRefObject()
 {
   AssertIsOnTargetThread();
   MOZ_ASSERT_IF(mWorkerPrivate && !mFeature, mTaskCount == 0);
   MOZ_ASSERT_IF(mWorkerPrivate && mFeature, mTaskCount > 0);
--- a/dom/notification/Notification.h
+++ b/dom/notification/Notification.h
@@ -12,17 +12,16 @@
 #include "mozilla/dom/NotificationBinding.h"
 #include "mozilla/dom/workers/bindings/WorkerFeature.h"
 
 #include "nsIObserver.h"
 
 #include "nsCycleCollectionParticipant.h"
 
 class nsIPrincipal;
-class nsIStructuredCloneContainer;
 class nsIVariant;
 
 namespace mozilla {
 namespace dom {
 
 class NotificationRef;
 class WorkerNotificationObserver;
 class Promise;
@@ -121,17 +120,17 @@ class Notification : public DOMEventTarg
 
 public:
   IMPL_EVENT_HANDLER(click)
   IMPL_EVENT_HANDLER(show)
   IMPL_EVENT_HANDLER(error)
   IMPL_EVENT_HANDLER(close)
 
   NS_DECL_ISUPPORTS_INHERITED
-  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(Notification, DOMEventTargetHelper)
+  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(Notification, DOMEventTargetHelper)
 
   static bool PrefEnabled(JSContext* aCx, JSObject* aObj);
   // Returns if Notification.get() is allowed for the current global.
   static bool IsGetEnabled(JSContext* aCx, JSObject* aObj);
 
   static already_AddRefed<Notification> Constructor(const GlobalObject& aGlobal,
                                                     const nsAString& aTitle,
                                                     const NotificationOptions& aOption,
@@ -200,18 +199,16 @@ public:
     mIsStored = val;
   }
 
   bool IsStored()
   {
     return mIsStored;
   }
 
-  nsIStructuredCloneContainer* GetDataCloneContainer();
-
   static bool RequestPermissionEnabledForScope(JSContext* aCx, JSObject* /* unused */);
 
   static void RequestPermission(const GlobalObject& aGlobal,
                                 const Optional<OwningNonNull<NotificationPermissionCallback> >& aCallback,
                                 ErrorResult& aRv);
 
   static NotificationPermission GetPermission(const GlobalObject& aGlobal,
                                               ErrorResult& aRv);
@@ -354,21 +351,21 @@ protected:
 
   const nsString mID;
   const nsString mTitle;
   const nsString mBody;
   const NotificationDirection mDir;
   const nsString mLang;
   const nsString mTag;
   const nsString mIconUrl;
-  nsCOMPtr<nsIStructuredCloneContainer> mDataObjectContainer;
+  nsString mDataAsBase64;
   const NotificationBehavior mBehavior;
 
   // It's null until GetData is first called
-  nsCOMPtr<nsIVariant> mData;
+  JS::Heap<JS::Value> mData;
 
   nsString mAlertName;
   nsString mScope;
 
   // Main thread only.
   bool mIsClosed;
 
   // We need to make a distinction between the notification being closed i.e.
--- a/dom/workers/test/notification_worker.js
+++ b/dom/workers/test/notification_worker.js
@@ -15,32 +15,36 @@ if (self.Notification) {
     },
 
     function (done) {
       var options = {
         dir: "auto",
         lang: "",
         body: "This is a notification body",
         tag: "sometag",
-        icon: "icon.png"
+        icon: "icon.png",
+        data: ["a complex object that should be", { "structured": "cloned" }],
+        mozbehavior: { vibrationPattern: [30, 200, 30] },
       };
       var notification = new Notification("This is a title", options);
 
       ok(notification !== undefined, "Notification exists");
       is(notification.onclick, null, "onclick() should be null");
       is(notification.onshow, null, "onshow() should be null");
       is(notification.onerror, null, "onerror() should be null");
       is(notification.onclose, null, "onclose() should be null");
       is(typeof notification.close, "function", "close() should exist");
 
       is(notification.dir, options.dir, "auto should get set");
       is(notification.lang, options.lang, "lang should get set");
       is(notification.body, options.body, "body should get set");
       is(notification.tag, options.tag, "tag should get set");
       is(notification.icon, options.icon, "icon should get set");
+      is(notification.data[0],  "a complex object that should be", "data item 0 should be a matching string");
+      is(notification.data[1]["structured"], "cloned", "data item 1 should be a matching object literal");
 
       // store notification in test context
       this.notification = notification;
 
       notification.onshow = function () {
         ok(true, "onshow handler should be called");
         done();
       };
--- a/dom/workers/test/serviceworkers/notificationclick.html
+++ b/dom/workers/test/serviceworkers/notificationclick.html
@@ -8,20 +8,20 @@
   <title>Bug 1114554 - controlled page</title>
 <script class="testbody" type="text/javascript">
   var testWindow = parent;
   if (opener) {
     testWindow = opener;
   }
 
   navigator.serviceWorker.ready.then(function(swr) {
-    swr.showNotification("Hi there. The ServiceWorker should receive a click event for this.");
+    swr.showNotification("Hi there. The ServiceWorker should receive a click event for this.", { data: { complex: ["jsval", 5] }});
   });
 
   navigator.serviceWorker.onmessage = function(msg) {
-    testWindow.callback();
+    testWindow.callback(msg.data.result);
   };
 </script>
 
 </head>
 <body>
 </body>
 </html>
--- a/dom/workers/test/serviceworkers/notificationclick.js
+++ b/dom/workers/test/serviceworkers/notificationclick.js
@@ -4,12 +4,16 @@
 onnotificationclick = function(e) {
   self.clients.matchAll().then(function(clients) {
     if (clients.length === 0) {
       dump("********************* CLIENTS LIST EMPTY! Test will timeout! ***********************\n");
       return;
     }
 
     clients.forEach(function(client) {
-      client.postMessage("done");
+      client.postMessage({ result: e.notification.data &&
+                                   e.notification.data['complex'] &&
+                                   e.notification.data['complex'][0] == "jsval" &&
+                                   e.notification.data['complex'][1] == 5 });
+
     });
   });
 }
--- a/dom/workers/test/serviceworkers/test_notificationclick.html
+++ b/dom/workers/test/serviceworkers/test_notificationclick.html
@@ -18,21 +18,21 @@ https://bugzilla.mozilla.org/show_bug.cg
 <pre id="test">
 </pre>
 <script type="text/javascript">
   SimpleTest.requestFlakyTimeout("Mock alert service dispatches show and click events.");
 
   function testFrame(src) {
     var iframe = document.createElement("iframe");
     iframe.src = src;
-    window.callback = function() {
+    window.callback = function(result) {
       window.callback = null;
       document.body.removeChild(iframe);
       iframe = null;
-      ok(true, "Got notificationclick event.");
+      ok(result, "Got notificationclick event with correct data.");
       MockServices.unregister();
       SimpleTest.finish();
     };
     document.body.appendChild(iframe);
   }
 
   function runTest() {
     MockServices.register();