diff --git a/dom/push/PushDB.jsm b/dom/push/PushDB.jsm index c24201c3d8c7..f3caa01b6a48 100644 --- a/dom/push/PushDB.jsm +++ b/dom/push/PushDB.jsm @@ -125,10 +125,10 @@ this.PushDB.prototype = { 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); }; }, diff --git a/dom/push/PushNotifier.cpp b/dom/push/PushNotifier.cpp index 6e16d73cb6ae..c9e67f74c218 100644 --- a/dom/push/PushNotifier.cpp +++ b/dom/push/PushNotifier.cpp @@ -47,6 +47,7 @@ PushNotifier::NotifyPushWithData(const nsACString& aScope, const nsAString& aMessageId, uint32_t aDataLen, uint8_t* aData) { + NS_ENSURE_ARG(aPrincipal); nsTArray data; if (!data.SetCapacity(aDataLen, fallible)) { return NS_ERROR_OUT_OF_MEMORY; @@ -62,6 +63,7 @@ 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); } @@ -70,6 +72,7 @@ NS_IMETHODIMP PushNotifier::NotifySubscriptionChange(const nsACString& aScope, nsIPrincipal* aPrincipal) { + NS_ENSURE_ARG(aPrincipal); PushSubscriptionChangeDispatcher dispatcher(aScope, aPrincipal); return Dispatch(dispatcher); } @@ -78,6 +81,7 @@ NS_IMETHODIMP PushNotifier::NotifySubscriptionModified(const nsACString& aScope, nsIPrincipal* aPrincipal) { + NS_ENSURE_ARG(aPrincipal); PushSubscriptionModifiedDispatcher dispatcher(aScope, aPrincipal); return Dispatch(dispatcher); } @@ -86,6 +90,7 @@ 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); } @@ -276,6 +281,9 @@ PushDispatcher::NotifyObserversAndWorkers() 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. diff --git a/dom/push/test/xpcshell/test_observer_remoting.js b/dom/push/test/xpcshell/test_observer_remoting.js index 3c06bfe76c56..80903bed37d3 100644 --- a/dom/push/test/xpcshell/test_observer_remoting.js +++ b/dom/push/test/xpcshell/test_observer_remoting.js @@ -11,10 +11,23 @@ add_task(function* test_observer_remoting() { } }); +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; @@ -23,7 +36,10 @@ function* testInParent() { // 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; @@ -32,16 +48,22 @@ function* testInParent() { 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( @@ -50,8 +72,7 @@ function* waitForNotifierObservers(expectedText, shouldNotify = false) { 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); @@ -68,7 +89,7 @@ function* waitForNotifierObservers(expectedText, shouldNotify = false) { 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, @@ -87,4 +108,4 @@ function* waitForNotifierObservers(expectedText, shouldNotify = false) { 'Should fire subscription modified notifications with the correct scope'); equal(subModifiedPrincipal, principal, 'Should pass the principal as the subject of a modified notification'); -} +}); diff --git a/dom/push/test/xpcshell/test_quota_observer.js b/dom/push/test/xpcshell/test_quota_observer.js index 1842cbf3bfaf..9401a5c869a2 100644 --- a/dom/push/test/xpcshell/test_quota_observer.js +++ b/dom/push/test/xpcshell/test_quota_observer.js @@ -110,7 +110,8 @@ add_task(function* test_expiration_history_observer() { 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', @@ -121,6 +122,17 @@ add_task(function* test_expiration_history_observer() { 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({ @@ -143,4 +155,29 @@ add_task(function* test_expiration_history_observer() { 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; });