Bug 1280106 - Don't send push events to service workers without a principal. r=dragana
authorKit Cambridge <kcambridge@mozilla.com>
Mon, 27 Jun 2016 08:22:59 -0700
changeset 302737 87c50f8365f82badd5f04ea7a223a52ce6278852
parent 302736 a21989d264ee66319e274891a12c390482dd9345
child 302738 e1223a519bdf8c624ab1c5a18ea58b2ce40d3beb
push id78851
push userkcambridge@mozilla.com
push dateMon, 27 Jun 2016 18:30:05 +0000
treeherdermozilla-inbound@e1223a519bdf [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdragana
bugs1280106
milestone50.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 1280106 - Don't send push events to service workers without a principal. r=dragana MozReview-Commit-ID: JwTU633hENl
dom/push/PushDB.jsm
dom/push/PushNotifier.cpp
dom/push/test/xpcshell/test_observer_remoting.js
dom/push/test/xpcshell/test_quota_observer.js
--- a/dom/push/PushDB.jsm
+++ b/dom/push/PushDB.jsm
@@ -120,20 +120,20 @@ this.PushDB.prototype = {
    */
   delete: function(aKeyID) {
     console.debug("delete()");
 
     return new Promise((resolve, reject) =>
       this.newTxn(
         "readwrite",
         this._dbStoreName,
-        function txnCb(aTxn, aStore) {
+        (aTxn, aStore) => {
           console.debug("delete: Removing record", aKeyID);
           aStore.get(aKeyID).onsuccess = event => {
-            aTxn.result = event.target.result;
+            aTxn.result = this.toPushRecord(event.target.result);
             aStore.delete(aKeyID);
           };
         },
         resolve,
         reject
       )
     );
   },
--- a/dom/push/PushNotifier.cpp
+++ b/dom/push/PushNotifier.cpp
@@ -42,55 +42,60 @@ NS_IMPL_CYCLE_COLLECTING_ADDREF(PushNoti
 NS_IMPL_CYCLE_COLLECTING_RELEASE(PushNotifier)
 
 NS_IMETHODIMP
 PushNotifier::NotifyPushWithData(const nsACString& aScope,
                                  nsIPrincipal* aPrincipal,
                                  const nsAString& aMessageId,
                                  uint32_t aDataLen, uint8_t* aData)
 {
+  NS_ENSURE_ARG(aPrincipal);
   nsTArray<uint8_t> data;
   if (!data.SetCapacity(aDataLen, fallible)) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
   if (!data.InsertElementsAt(0, aData, aDataLen, fallible)) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
   PushMessageDispatcher dispatcher(aScope, aPrincipal, aMessageId, Some(data));
   return Dispatch(dispatcher);
 }
 
 NS_IMETHODIMP
 PushNotifier::NotifyPush(const nsACString& aScope, nsIPrincipal* aPrincipal,
                          const nsAString& aMessageId)
 {
+  NS_ENSURE_ARG(aPrincipal);
   PushMessageDispatcher dispatcher(aScope, aPrincipal, aMessageId, Nothing());
   return Dispatch(dispatcher);
 }
 
 NS_IMETHODIMP
 PushNotifier::NotifySubscriptionChange(const nsACString& aScope,
                                        nsIPrincipal* aPrincipal)
 {
+  NS_ENSURE_ARG(aPrincipal);
   PushSubscriptionChangeDispatcher dispatcher(aScope, aPrincipal);
   return Dispatch(dispatcher);
 }
 
 NS_IMETHODIMP
 PushNotifier::NotifySubscriptionModified(const nsACString& aScope,
                                          nsIPrincipal* aPrincipal)
 {
+  NS_ENSURE_ARG(aPrincipal);
   PushSubscriptionModifiedDispatcher dispatcher(aScope, aPrincipal);
   return Dispatch(dispatcher);
 }
 
 NS_IMETHODIMP
 PushNotifier::NotifyError(const nsACString& aScope, nsIPrincipal* aPrincipal,
                           const nsAString& aMessage, uint32_t aFlags)
 {
+  NS_ENSURE_ARG(aPrincipal);
   PushErrorDispatcher dispatcher(aScope, aPrincipal, aMessage, aFlags);
   return Dispatch(dispatcher);
 }
 
 nsresult
 PushNotifier::Dispatch(PushDispatcher& aDispatcher)
 {
   if (XRE_IsParentProcess()) {
@@ -271,16 +276,19 @@ PushDispatcher::NotifyObserversAndWorker
 {
   Unused << NS_WARN_IF(NS_FAILED(NotifyObservers()));
   return NotifyWorkers();
 }
 
 bool
 PushDispatcher::ShouldNotifyWorkers()
 {
+  if (NS_WARN_IF(!mPrincipal)) {
+    return false;
+  }
   // System subscriptions use observer notifications instead of service worker
   // events. The `testing.notifyWorkers` pref disables worker events for
   // non-system subscriptions.
   return !nsContentUtils::IsSystemPrincipal(mPrincipal) &&
          Preferences::GetBool("dom.push.testing.notifyWorkers", true);
 }
 
 nsresult
--- a/dom/push/test/xpcshell/test_observer_remoting.js
+++ b/dom/push/test/xpcshell/test_observer_remoting.js
@@ -6,74 +6,95 @@ const pushNotifier = Cc['@mozilla.org/pu
 add_task(function* test_observer_remoting() {
   if (isParent) {
     yield testInParent();
   } else {
     yield testInChild();
   }
 });
 
+const childTests = [{
+  text: 'Hello from child!',
+  principal: Services.scriptSecurityManager.getSystemPrincipal(),
+}];
+
+const parentTests = [{
+  text: 'Hello from parent!',
+  principal: Services.scriptSecurityManager.getSystemPrincipal(),
+}];
+
 function* testInParent() {
   // Register observers for notifications from the child, then run the test in
   // the child and wait for the notifications.
-  let promiseNotifications = waitForNotifierObservers('Hello from child!');
+  let promiseNotifications = childTests.reduce(
+    (p, test) => p.then(_ => waitForNotifierObservers(test, /* shouldNotify = */ false)),
+    Promise.resolve()
+  );
   let promiseFinished = run_test_in_child('./test_observer_remoting.js');
   yield promiseNotifications;
 
   // Wait until the child is listening for notifications from the parent.
   yield do_await_remote_message('push_test_observer_remoting_child_ready');
 
   // Fire an observer notification in the parent that should be forwarded to
   // the child.
-  yield waitForNotifierObservers('Hello from parent!', true);
+  yield parentTests.reduce(
+    (p, test) => p.then(_ => waitForNotifierObservers(test, /* shouldNotify = */ true)),
+    Promise.resolve()
+  );
 
   // Wait for the child to exit.
   yield promiseFinished;
 }
 
 function* testInChild() {
   // Fire an observer notification in the child that should be forwarded to
   // the parent.
-  yield waitForNotifierObservers('Hello from child!', true);
+  yield childTests.reduce(
+    (p, test) => p.then(_ => waitForNotifierObservers(test, /* shouldNotify = */ true)),
+    Promise.resolve()
+  );
 
   // Register observers for notifications from the parent, let the parent know
   // we're ready, and wait for the notifications.
-  let promiseNotifierObservers = waitForNotifierObservers('Hello from parent!');
+  let promiseNotifierObservers = parentTests.reduce(
+    (p, test) => p.then(_ => waitForNotifierObservers(test, /* shouldNotify = */ false)),
+    Promise.resolve()
+  );
   do_send_remote_message('push_test_observer_remoting_child_ready');
   yield promiseNotifierObservers;
 }
 
-function* waitForNotifierObservers(expectedText, shouldNotify = false) {
+var waitForNotifierObservers = Task.async(function* ({ text, principal }, shouldNotify = false) {
   let notifyPromise = promiseObserverNotification(
     PushServiceComponent.pushTopic);
   let subChangePromise = promiseObserverNotification(
     PushServiceComponent.subscriptionChangeTopic);
   let subModifiedPromise = promiseObserverNotification(
     PushServiceComponent.subscriptionModifiedTopic);
 
   let scope = 'chrome://test-scope';
-  let principal = Services.scriptSecurityManager.getSystemPrincipal();
-  let data = new TextEncoder('utf-8').encode(expectedText);
+  let data = new TextEncoder('utf-8').encode(text);
 
   if (shouldNotify) {
     pushNotifier.notifyPushWithData(scope, principal, '', data.length, data);
     pushNotifier.notifySubscriptionChange(scope, principal);
     pushNotifier.notifySubscriptionModified(scope, principal);
   }
 
   let {
     data: notifyScope,
     subject: notifySubject,
   } = yield notifyPromise;
   equal(notifyScope, scope,
     'Should fire push notifications with the correct scope');
   let message = notifySubject.QueryInterface(Ci.nsIPushMessage);
   equal(message.principal, principal,
     'Should include the principal in the push message');
-  strictEqual(message.data.text(), expectedText, 'Should include data');
+  strictEqual(message.data.text(), text, 'Should include data');
 
   let {
     data: subChangeScope,
     subject: subChangePrincipal,
   } = yield subChangePromise;
   equal(subChangeScope, scope,
     'Should fire subscription change notifications with the correct scope');
   equal(subChangePrincipal, principal,
@@ -82,9 +103,9 @@ function* waitForNotifierObservers(expec
   let {
     data: subModifiedScope,
     subject: subModifiedPrincipal,
   } = yield subModifiedPromise;
   equal(subModifiedScope, scope,
     'Should fire subscription modified notifications with the correct scope');
   equal(subModifiedPrincipal, principal,
     'Should pass the principal as the subject of a modified notification');
-}
+});
--- a/dom/push/test/xpcshell/test_quota_observer.js
+++ b/dom/push/test/xpcshell/test_quota_observer.js
@@ -105,27 +105,39 @@ add_task(function* test_expiration_histo
   strictEqual(expiredRecord.quota, 0, 'Expired record not updated');
 
   let notifiedScopes = [];
   subChangePromise = promiseObserverNotification(PushServiceComponent.subscriptionChangeTopic, (subject, data) => {
     notifiedScopes.push(data);
     return notifiedScopes.length == 2;
   });
 
-  // Add an expired registration that we'll revive later.
+  // Add an expired registration that we'll revive later using the idle
+  // observer.
   yield putRecord('ALLOW_ACTION', {
     channelID: 'eb33fc90-c883-4267-b5cb-613969e8e349',
     pushEndpoint: 'https://example.org/push/2',
     scope: 'https://example.com/auctions',
     pushCount: 0,
     lastPush: 0,
     version: null,
     originAttributes: '',
     quota: 0,
   });
+  // ...And an expired registration that we'll revive on fetch.
+  yield putRecord('ALLOW_ACTION', {
+    channelID: '6b2d13fe-d848-4c5f-bdda-e9fc89727dca',
+    pushEndpoint: 'https://example.org/push/4',
+    scope: 'https://example.net/sales',
+    pushCount: 0,
+    lastPush: 0,
+    version: null,
+    originAttributes: '',
+    quota: 0,
+  });
 
   // Now visit the site...
   yield PlacesTestUtils.addVisits({
     uri: 'https://example.com/another-page',
     title: 'Infrequently-visited page',
     visitDate: Date.now() * 1000,
     transition: Ci.nsINavHistoryService.TRANSITION_LINK
   });
@@ -138,9 +150,34 @@ add_task(function* test_expiration_histo
     'https://example.com/deals'
   ], 'Wrong scopes for subscription changes');
 
   let aRecord = yield db.getByKeyID('379c0668-8323-44d2-a315-4ee83f1a9ee9');
   ok(!aRecord, 'Should drop expired record');
 
   let bRecord = yield db.getByKeyID('eb33fc90-c883-4267-b5cb-613969e8e349');
   ok(!bRecord, 'Should drop evicted record');
+
+  // Simulate a visit to a site with an expired registration, then fetch the
+  // record. This should drop the expired record and fire an observer
+  // notification.
+  yield PlacesTestUtils.addVisits({
+    uri: 'https://example.net/sales',
+    title: 'Firefox plushies, 99% off',
+    visitDate: Date.now() * 1000,
+    transition: Ci.nsINavHistoryService.TRANSITION_LINK
+  });
+  subChangePromise = promiseObserverNotification(PushServiceComponent.subscriptionChangeTopic, (subject, data) => {
+    if (data == 'https://example.net/sales') {
+      ok(subject.isCodebasePrincipal,
+        'Should pass subscription principal as the subject');
+      return true;
+    }
+  });
+  let record = yield PushService.registration({
+    scope: 'https://example.net/sales',
+    originAttributes: '',
+  });
+  ok(!record, 'Should not return evicted record');
+  ok(!(yield db.getByKeyID('6b2d13fe-d848-4c5f-bdda-e9fc89727dca')),
+    'Should drop evicted record on fetch');
+  yield subChangePromise;
 });