Closed Bug 856440 Opened 8 years ago Closed 8 years ago

Push Notification startup regression

Categories

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

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
b2g18 --- fixed

People

(Reporter: dougt, Assigned: dougt)

Details

Attachments

(3 files, 2 obsolete files)

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 ...
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.
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
Attached patch disable pref (obsolete) — Splinter Review
Attachment #731687 - Flags: review?
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-
Attached patch disable prefSplinter Review
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
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Component: DOM: Other → DOM
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attached patch patch v.2 follow up (obsolete) — Splinter Review
Attachment #735007 - Flags: review?
Attachment #735007 - Flags: review? → review?(nsm.nikhil)
Attachment #735011 - Flags: review?(nsm.nikhil)
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
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
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?
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+
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.