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

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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?

Comment 1

[Justin Lebar (not reading bugmail)]

FTR I think bent volunteered to write this patch on IRC.  :)

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Comment 3

[Justin Lebar (not reading bugmail)]

Comment on attachment 714413 [details] [diff] [review]
Patch, v1

Wow, I did a pretty bad review of this originally.  Thanks for fixing this.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1b4f443548d

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f1b4f443548d

Comment 6

[Justin Lebar (not reading bugmail)]

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

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 714413 [details] [diff] [review]
Patch, v1

Approving for v1-train uplift.

Comment 8

[Justin Lebar (not reading bugmail)]

We can't take this for b2g18-v1.0.1?

Are we really OK with not firing alarms?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/e308b95f64e4