851253 - SimplePush: Only wake up registered applications using push-register

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

Broadcasting a system message has two drawbacks:

1) It could end up launching 10-20 push enabled apps at the same time, putting serious load on the system. Have a staggered wake up.

2) Don't start push enabled apps unless they actually have active registrations which will need to be refreshed. Apps that've never run or never acquired an endpoint, or no longer have active endpoints should not be started.

Comment 1

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

@Marcelino, input from Telefonica is welcome here. I'm going to fix this after 822712 lands, only if it is shown to be a problem

Comment 2

[Marcelino Veiga Tuimil [:sonmarce]]

@Nikhil I have added Telefonica push guys in CC, so they can give you some feedback, better than me :-)

Comment 3

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

Attachment: only push-register applications with registrations

Comment 4

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

Attachment: only push-register applications with registrations

Comment 5

[Justin Lebar (not reading bugmail)]

Comment on attachment 733618 [details] [diff] [review]
only push-register applications with registrations

To be clear, this patch does not appear to stagger the wakeup.  Which is fine.

Comment 6

[Justin Lebar (not reading bugmail)]

Oops, I had review comments!

r=me with some nits.

>+      this._db.getAllChannelIDs(function(records) {
>+        this._notifyAllAppsRegister(records);
>+        this._dropRegistrations()
>+          .then(finishHandshake.bind(this));

Nit: I know this is inconsisent with how you did it when the .then() was a
multiline clause, but here it would match our style if you moved the "." in
".then" to align with the "." in the line above.

>@@ -804,21 +796,41 @@ PushService.prototype = {
>       debug("Could not get channelID " + aChannelIDFromServer + " from DB");
>     }
> 
>     this._db.getByChannelID(aChannelID,
>                             compareRecordVersionAndNotify.bind(this),
>                             recoverNoSuchChannelID.bind(this));
>   },
> 
>-  _notifyAllAppsRegister: function() {
>+  // Fires a push-register system message to all applications that have
>+  // registrations.
>+  _notifyAllAppsRegister: function(records) {

Can you please explain in the comment what |records| is supposed to be?  Or
even better: Can we make this function take no arguments and return a promise?
So you'd do something like

  this._notifyAllAppsRegister()
      .then(this._dropRegistrations.bind(this))
      .then(finishHandshake.bind(this));

>     debug("notifyAllAppsRegister()");
>     let messenger = Cc["@mozilla.org/system-message-internal;1"]
>                       .getService(Ci.nsISystemMessagesInternal);
>-    messenger.broadcastMessage('push-register', {});
>+
>+    // Pages to be notified.
>+    // wakeupTable[manifestURL] -> [ pageURL ]
>+    var wakeupTable = {};
>+    for (var i = 0; i < records.length; i++) {
>+      var record = records[i];
>+      if (!wakeupTable[record.manifestURL])

Nit: It's equivalent in this case, but it would be more idiomatic to do |if
(record.manifestURL in wakeupTable)|.

Comment 7

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

Attachment: only push-register applications with registrations

Carrying forward r+ from jlebar and landing.

Convert notifyAllAppsRegister() to promises and fix nits.

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/01ffdd61fd94

Marking as [leave open] since part 1 of bug description hasn't been fixed.

Comment 9

[Justin Lebar (not reading bugmail)]

> Marking as [leave open] since part 1 of bug description hasn't been fixed.

Would you mind taking care of that in a separate bug?  Combining landings into multiple bugs often makes it really confusing to understand what's going on, particularly when uplifts are involved.

Comment 10

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

Filed bug 860041 for additional improvements.

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/01ffdd61fd94

Comment 12

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

Comment on attachment 735414 [details] [diff] [review]
only push-register applications with registrations

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 822712 Push Notifications

User impact if declined: 
Low impact while push has low usage. But if a user installs many applications with push notifications, they'll have a lot more background apps running when notifications arrive, unless this patch is applied.

Testing completed: 
Yes

Risk to taking this patch (and alternatives if risky): 
None as long as push is disabled.

String or UUID changes made by this patch:
None

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/b664c49fb8b7