Push Notification startup regression

RESOLVED FIXED in mozilla23

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dougt, Assigned: dougt)

Tracking

unspecified
mozilla23
x86
All
Points:
---

Firefox Tracking Flags

(b2g18 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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 <dougt@dougt.org> - 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 <nsm.nikhil@gmail.com> - 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 <dougt@dougt.org> - 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 <dougt@dougt.org> - 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 <nsm.nikhil@gmail.com> - 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 <dougt@dougt.org> - 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 <dougt@dougt.org> - 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 <nsm.nikhil@gmail.com> - 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 <dougt@dougt.org> - 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.
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 ...
(Assignee)

Comment 2

5 years ago
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?
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.
(Assignee)

Comment 4

5 years ago
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
(Assignee)

Comment 5

5 years ago
Created attachment 731687 [details] [diff] [review]
disable pref
Attachment #731687 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #731687 - Flags: review? → review?(nsm.nikhil)
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.
Attachment #731687 - Flags: review?(nsm.nikhil) → review-
Created attachment 732115 [details] [diff] [review]
disable pref

Updated patch which adds a (false by default) pref to all platforms and checks for pref in navigator.push as well.
Attachment #731687 - Attachment is obsolete: true
Comment on attachment 732115 [details] [diff] [review]
disable pref

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

LGTM
Attachment #732115 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/7f1a8ad8bf0c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Component: DOM: Other → DOM
Product: Core → Core
(Assignee)

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

5 years ago
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.
(Assignee)

Comment 12

5 years ago
Created attachment 735007 [details] [diff] [review]
patch v.2 follow up
Attachment #735007 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #735007 - Flags: review? → review?(nsm.nikhil)
(Assignee)

Comment 13

5 years ago
Created attachment 735011 [details] [diff] [review]
patch v.2 follow up
Attachment #735011 - Flags: review?(nsm.nikhil)
(Assignee)

Updated

5 years ago
Attachment #735007 - Attachment is obsolete: true
Attachment #735007 - Flags: review?(nsm.nikhil)
(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 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
Attachment #735011 - Flags: review?(nsm.nikhil) → review+
https://hg.mozilla.org/mozilla-central/rev/e13ccfc9b811
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Created attachment 741137 [details] [diff] [review]
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
Attachment #741137 - Flags: review?(doug.turner)
Attachment #741137 - Flags: approval-mozilla-b2g18?
(Assignee)

Comment 19

5 years ago
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.
Attachment #741137 - Flags: review?(doug.turner)
Attachment #741137 - Flags: review+
Attachment #741137 - Flags: approval-mozilla-b2g18?
Attachment #741137 - Flags: approval-mozilla-b2g18+
(Assignee)

Comment 20

5 years ago
just kidding about tef+.  I meant leo+.
(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.
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
status-b2g18: --- → fixed
You need to log in before you can comment on or make changes to this bug.