856440 - Push Notification startup regression

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Doug Turner (:dougt)]

Regression: Firefox - Trace Malloc Allocs - CentOS (x86_64) release 5 (Final) - 2.66% increase
----------------------------------------------------------------------------------------------
    Previous: avg 540981.533 stddev 3628.872 of 30 runs up to revision 8aeabe064932
    New     : avg 555370.600 stddev 1510.698 of 5 runs since revision bfced2ecc0cf
    Change  : +14389.067 (2.66% / z=3.965)
    Graph   : http://mzl.la/10de8Fu

Changeset range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8aeabe064932&tochange=bfced2ecc0cf

Changesets:
  * http://hg.mozilla.org/mozilla-central/rev/e93a4da26856
    : Doug Turner <[e-mail]> - Bug 822712 - SimplePush - Interface. r=dougt, sr=sicking
    : http://bugzilla.mozilla.org/show_bug.cgi?id=822712

  * http://hg.mozilla.org/mozilla-central/rev/2aaf82b852e7
    : Nikhil Marathe <[e-mail]> - Bug 822712 - SimplePush - Implementation. r=dougt, jst, jlebar
    : http://bugzilla.mozilla.org/show_bug.cgi?id=822712

  * http://hg.mozilla.org/mozilla-central/rev/bfced2ecc0cf
    : Doug Turner <[e-mail]> - Bug 822712 - SimplePush - UDP Wakeup feature. r=jst, jlebar
    : http://bugzilla.mozilla.org/show_bug.cgi?id=822712

Bugs:
  * http://bugzilla.mozilla.org/show_bug.cgi?id=822712 - Simple Push notifications for B2G


-----------------

Regression: Firefox-Non-PGO - Tp5 No Network Row Major MozAfterPaint (Non-Main Startup File IO Bytes) - Win7 - 38% increase
---------------------------------------------------------------------------------------------------------------------------
    Previous: avg 560341.633 stddev 99685.221 of 30 runs up to revision 8aeabe064932
    New     : avg 773273.800 stddev 19911.764 of 5 runs since revision bfced2ecc0cf
    Change  : +212932.167 (38% / z=2.136)
    Graph   : http://mzl.la/10dgSms

Changeset range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8aeabe064932&tochange=bfced2ecc0cf

Changesets:
  * http://hg.mozilla.org/mozilla-central/rev/e93a4da26856
    : Doug Turner <[e-mail]> - Bug 822712 - SimplePush - Interface. r=dougt, sr=sicking
    : http://bugzilla.mozilla.org/show_bug.cgi?id=822712

  * http://hg.mozilla.org/mozilla-central/rev/2aaf82b852e7
    : Nikhil Marathe <[e-mail]> - Bug 822712 - SimplePush - Implementation. r=dougt, jst, jlebar
    : http://bugzilla.mozilla.org/show_bug.cgi?id=822712

  * http://hg.mozilla.org/mozilla-central/rev/bfced2ecc0cf
    : Doug Turner <[e-mail]> - Bug 822712 - SimplePush - UDP Wakeup feature. r=jst, jlebar
    : http://bugzilla.mozilla.org/show_bug.cgi?id=822712

Bugs:
  * http://bugzilla.mozilla.org/show_bug.cgi?id=822712 - Simple Push notifications for B2G


-----------------


Regression: Firefox - Trace Malloc Allocs - WINNT 5.2 - 2.69% increase
----------------------------------------------------------------------
    Previous: avg 452586.467 stddev 2782.522 of 30 runs up to revision 8aeabe064932
    New     : avg 464763.800 stddev 993.787 of 5 runs since revision bfced2ecc0cf
    Change  : +12177.333 (2.69% / z=4.376)
    Graph   : http://mzl.la/10dk1Cz

Changeset range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8aeabe064932&tochange=bfced2ecc0cf

Changesets:
  * http://hg.mozilla.org/mozilla-central/rev/e93a4da26856
    : Doug Turner <[e-mail]> - Bug 822712 - SimplePush - Interface. r=dougt, sr=sicking
    : http://bugzilla.mozilla.org/show_bug.cgi?id=822712

  * http://hg.mozilla.org/mozilla-central/rev/2aaf82b852e7
    : Nikhil Marathe <[e-mail]> - Bug 822712 - SimplePush - Implementation. r=dougt, jst, jlebar
    : http://bugzilla.mozilla.org/show_bug.cgi?id=822712

  * http://hg.mozilla.org/mozilla-central/rev/bfced2ecc0cf
    : Doug Turner <[e-mail]> - Bug 822712 - SimplePush - UDP Wakeup feature. r=jst, jlebar
    : http://bugzilla.mozilla.org/show_bug.cgi?id=822712

Bugs:
  * http://bugzilla.mozilla.org/show_bug.cgi?id=822712 - Simple Push notifications for B2G



Fix (ideally immediately), or backout.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Tracemalloc Allocs is just a count of the calls to malloc, which isn't a particularly useful performance metric.

That said I'm kind of surprised that this code runs on Windows at all ...

Comment 2

[Doug Turner (:dougt)]

Okay -- let's ignore Tracemalloc allocs for now. Yes, it could be pref'ed off until we have a desktop UI..

The worry is:

Regression: Firefox-Non-PGO - Tp5 No Network Row Major MozAfterPaint (Non-Main Startup File IO Bytes) - Win7 - 38% increase


In the startup, we are basically looking at indexedDB.  This probably should be delayed so we aren't in the startup path.  Other similar components wait until browser-ui-startup-complete for this.  What do you think Kyle?

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Waiting for startup to complete sounds reasonable, though I'd be very surprised that this delays startup by 37% if all it's doing is kicking off some indexedDB calls.

Comment 4

[Doug Turner (:dougt)]

I added timing and nothing stands out:

-*- PushService.js: 1364756000823 final-ui-startup start
-*- PushService.js: init()
-*- PushService.js: 1364756000824 new PushDB start
-*- PushService.js: PushDB()
-*- PushService.js: 1364756000825 new PushDB start
-*- PushService.js: 1364756000825 ppmm access start
-*- PushService.js: 1364756000826 ppmm access end
-*- PushService.js: 1364756000826 getallchannelids start
-*- PushService.js: getAllChannelIDs()
-*- PushService.js: 1364756000827 getallchannelids end
-*- PushService.js: 1364756000828 final-ui-startup end

Comment 5

[Doug Turner (:dougt)]

Attachment: disable pref

Comment 6

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

Comment on attachment 731687 [details] [diff] [review]
disable pref

Review of attachment 731687 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/push/src/PushService.js
@@ +280,5 @@
>  
>    observe: function observe(aSubject, aTopic, aData) {
>      switch (aTopic) {
>        case "app-startup":
> +        if (this._prefs.get("enabled") == false) {

When there is no existing pref, _prefs.get() will return undefined unless a default value is passed. So this fails on FF Nightly and PushService still starts.

How about:

  if (!this._prefs.get("enabled"))

@@ +281,5 @@
>    observe: function observe(aSubject, aTopic, aData) {
>      switch (aTopic) {
>        case "app-startup":
> +        if (this._prefs.get("enabled") == false) {
> +            return;

Nit: Indent is 2 spaces.

Comment 7

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

Attachment: disable pref

Updated patch which adds a (false by default) pref to all platforms and checks for pref in navigator.push as well.

Comment 8

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

Comment on attachment 732115 [details] [diff] [review]
disable pref

Review of attachment 732115 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f1a8ad8bf0c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7f1a8ad8bf0c

Comment 11

[Doug Turner (:dougt)]

My testing clearly wasn't correct.  It looks like this test:

   if (!this._prefs.get("enabled"))

during app startup was always false no matter what value the preference was.  What follows is a patch to move away from the getter on PushService and just to make a const.  This is what other services do.

Comment 12

[Doug Turner (:dougt)]

Attachment: patch v.2 follow up

Comment 13

[Doug Turner (:dougt)]

Attachment: patch v.2 follow up

Comment 14

[Kartikaya Gupta (email:[e-mail])]

(In reply to Doug Turner (:dougt) from comment #11)
> My testing clearly wasn't correct.  It looks like this test:
> 
>    if (!this._prefs.get("enabled"))
> 
> during app startup was always false no matter what value the preference was.

This happens surprisingly often - I had filed bug 814517 for this issue a while back. Perhaps a system-wide check for this sort of thing would be a good idea.

Comment 15

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

Comment on attachment 735011 [details] [diff] [review]
patch v.2 follow up

Review of attachment 735011 [details] [diff] [review]:
-----------------------------------------------------------------

Landing changed patch with whitespace nits fixed and sandbox bits removed.

::: dom/push/src/PushService.js
@@ +19,5 @@
>  Cu.import("resource://gre/modules/Timer.jsm");
>  Cu.import("resource://gre/modules/services-common/preferences.js");
>  Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");
>  
>  Cu.import("resource://gre/modules/identity/Sandbox.jsm");

looks like you're patching over the sandbox patch which hasn't landed yet.

@@ +317,5 @@
>  
>    observe: function observe(aSubject, aTopic, aData) {
>      switch (aTopic) {
>        case "app-startup":
> +        

whitespace

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e13ccfc9b811

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e13ccfc9b811

Comment 18

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

Attachment: Combined fix for b2g18.

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
User impact if declined: 
None

Testing completed: 
Yes

Risk to taking this patch (and alternatives if risky):
Very low. Simply uses an alternative preferences object and disables push by default on all platforms except B2G.
 
String or UUID changes made by this patch:
None

Comment 19

[Doug Turner (:dougt)]

Comment on attachment 741137 [details] [diff] [review]
Combined fix for b2g18.

Review of attachment 741137 [details] [diff] [review]:
-----------------------------------------------------------------

r+ and a+.  this is a required fix for b2g18 for tef+

::: dom/push/src/Push.js
@@ +39,5 @@
>    init: function(aWindow) {
>      debug("init()");
>  
> +    if (!Services.prefs.getBoolPref("services.push.enabled"))
> +      return null;

isn't the style in this file:

if () {
  stmt;
}

::: dom/push/src/PushService.js
@@ +281,5 @@
>    observe: function observe(aSubject, aTopic, aData) {
>      switch (aTopic) {
>        case "app-startup":
> +
> +        if (!prefs.get("enabled"))

I think we need to move this into final-ui-startup.

Comment 20

[Doug Turner (:dougt)]

just kidding about tef+.  I meant leo+.

Comment 21

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

(In reply to Doug Turner (:dougt) from comment #19)
> Comment on attachment 741137 [details] [diff] [review]
> Combined fix for b2g18.
> 
> Review of attachment 741137 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ and a+.  this is a required fix for b2g18 for tef+
> 
> ::: dom/push/src/Push.js
> @@ +39,5 @@
> >    init: function(aWindow) {
> >      debug("init()");
> >  
> > +    if (!Services.prefs.getBoolPref("services.push.enabled"))
> > +      return null;
> 
> isn't the style in this file:
> 
> if () {
>   stmt;
> }

I've been on the 'single statement does not require braces' style in this file. Keeping it this way.

> 
> ::: dom/push/src/PushService.js
> @@ +281,5 @@
> >    observe: function observe(aSubject, aTopic, aData) {
> >      switch (aTopic) {
> >        case "app-startup":
> > +
> > +        if (!prefs.get("enabled"))
> 
> I think we need to move this into final-ui-startup.
But that'll mean we still end up listening for 3 events, and this is *per child process* until bug 863598 lands. Putting it here we skip those.

Comment 22

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

Landed with parentheses nit fixed.

The change to global preferences object in patch v.2 fixed pref not being available in app-startup, so I haven't changed that.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/4ad9ab79e0a0