If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

TimeChangeObserver::FireMozTimeChangeEvent bails out if *any* weak windows have gone away

RESOLVED FIXED in Firefox 21

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ben Turner (not reading bugmail, use the needinfo flag!), Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

(Blocks: 1 bug)

unspecified
mozilla21
Points:
---

Firefox Tracking Flags

(firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

Details

Attachments

(1 attachment)

This doesn't seem right to me, but TimeChangeObserver::FireMozTimeChangeEvent bails out of its loop if any of the weak windows it was holding have gone away. I think you want to continue through the loop and notify all the others that could still be alive, right?
FTR I think bent volunteered to write this patch on IRC.  :)
Blocks: 714349
Created attachment 714413 [details] [diff] [review]
Patch, v1
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Attachment #714413 - Flags: review?(justin.lebar+bug)
Comment on attachment 714413 [details] [diff] [review]
Patch, v1

Wow, I did a pretty bad review of this originally.  Thanks for fixing this.
Attachment #714413 - Flags: review?(justin.lebar+bug) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1b4f443548d
https://hg.mozilla.org/mozilla-central/rev/f1b4f443548d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Comment on attachment 714413 [details] [diff] [review]
Patch, v1

Any webpage can register timechange listeners.  If a webpage does so and then is closed, this code will cause us not to fire listeners which were added after that page's listener.

In other words, we will drop timechange events.  This will break alarms on the phone.  See the embarrassment Apple has had wrt alarms and daylight savings time.

Can we please uplift this patch?

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 714358
User impact if declined: See above.
Testing completed: Simple patch; it's hard to imagine how this patch could make things worse than the clearly buggy state we're currently in.
Risk to taking this patch (and alternatives if risky): Simple patch
String or UUID changes made by this patch: None
Attachment #714413 - Flags: approval-mozilla-b2g18?
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
Comment on attachment 714413 [details] [diff] [review]
Patch, v1

Approving for v1-train uplift.
Attachment #714413 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
We can't take this for b2g18-v1.0.1?

Are we really OK with not firing alarms?
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e308b95f64e4
status-b2g18: affected → fixed
status-firefox20: --- → wontfix
status-firefox21: --- → fixed
You need to log in before you can comment on or make changes to this bug.