Closed Bug 1243856 Opened 4 years ago Closed 4 years ago

Don't use `AlarmService` for reconnect timers

Categories

(Core :: DOM: Push Notifications, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: Lina, Assigned: Lina)

References

(Blocks 1 open bug)

Details

(Whiteboard: dom-triaged)

Attachments

(2 files)

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: android-push
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
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)
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
https://hg.mozilla.org/mozilla-central/rev/e58bd4bbf964
https://hg.mozilla.org/mozilla-central/rev/af9a9799032d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.