Don't use `AlarmService` for reconnect timers

RESOLVED FIXED in Firefox 47

Status

()

Core
DOM: Push Notifications
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kitcambridge, Assigned: kitcambridge)

Tracking

(Blocks: 1 bug)

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: dom-triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

It's not supported on Android, and complicates the reconnect timer and test code. Let's just replace it with timers for now; we can revisit when we need to support the new DOM API on B2G.
Blocks: 1214362
No longer blocks: 1206207
With this in place, we should be able to run the Push mochitests -- backed by websockets, not GCM and Autopush -- on Android.  That'll go a long way to improving our confidence that the Web Push API is being exposed correctly across all platforms.

We'll need other tests to ensure that swapping in the GCM and Autopush backend doesn't regress.
Whiteboard: dom-noted
Created attachment 8715550 [details]
MozReview Request: Bug 1243856 - Remove alarms from the Push WebSocket backend. r?dragana

Review commit: https://reviewboard.mozilla.org/r/33507/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33507/
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
https://reviewboard.mozilla.org/r/33507/#review30197

::: dom/push/PushServiceWebSocket.jsm:492
(Diff revision 1)
> +           Object.keys(this._registerRequests).length > 0;

Note to self: `_registerRequests` should really be a map.
Comment on attachment 8715550 [details]
MozReview Request: Bug 1243856 - Remove alarms from the Push WebSocket backend. r?dragana

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33507/diff/1-2/
Attachment #8715550 - Attachment description: MozReview Request: Bug 1243856 - Remove alarms from the Push WebSocket backend. → MozReview Request: Bug 1243856 - Remove alarms from the Push WebSocket backend. r?dragana
Attachment #8715550 - Flags: review?(dd.mozilla)
Created attachment 8715617 [details]
MozReview Request: Bug 1243856 - Remove alarms from the Push H2 backend. r?dragana

Review commit: https://reviewboard.mozilla.org/r/33575/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33575/
Attachment #8715617 - Flags: review?(dd.mozilla)
Comment on attachment 8715550 [details]
MozReview Request: Bug 1243856 - Remove alarms from the Push WebSocket backend. r?dragana

https://reviewboard.mozilla.org/r/33507/#review30303

looks good.
Attachment #8715550 - Flags: review?(dd.mozilla) → review+
Comment on attachment 8715617 [details]
MozReview Request: Bug 1243856 - Remove alarms from the Push H2 backend. r?dragana

https://reviewboard.mozilla.org/r/33575/#review30305

::: dom/push/PushServiceHttp2.jsm:626
(Diff revision 1)
> +        this._conns[subscriptionUri].channel.listener = null;

this should be:
this._conns[subscriptionUri].listener = null;
Attachment #8715617 - Flags: review?(dd.mozilla) → review+
Whiteboard: dom-noted → dom-triaged
https://reviewboard.mozilla.org/r/33575/#review30305

> this should be:
> this._conns[subscriptionUri].listener = null;

Eek, that was an egregious copy-paste fail. Thanks! :-)
Depends on: 1246066
Priority: -- → P1
The code that was clearing the timeout wasn't being run due to bug 1251113 and caused the intermittent crash. With the patch in bug 1251113 applied I no can no longer reproduce so hopefully it should be sufficient to re-land this work.
Depends on: 1251113
No longer depends on: 1246066

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e58bd4bbf964
https://hg.mozilla.org/mozilla-central/rev/af9a9799032d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.