See bug 870203 comment 11. I think we want to take this patch even if it doesn't solve bug 870203, so I've split it into a new bug.

Blocks a blocker.

This also fixes an important bug where, if a process crashed while holding a wake lock, we'd send a wake lock changed notification without including the list of locking processes. This could cause us to incorrectly believe that no processes were holding the lock. A side-effect of this patch is that we make bug 870444 worse: We now fire more unnecessary wakelock changed events. Doing this right is tricky, and since these events aren't exposed to anything but certified code, I don't mind this minor regression. Kan-Ru, would you mind looking at this?

I'll see what I can do about writing a test here. It's kind of complicated because we don't expose the list of locking processes to JS.

Comment on attachment 747545 [details] [diff] [review] Patch, v1: Notify wake lock listeners when a process adds or removes a wake lock, even if the wake lock itself doesn't change state. Review of attachment 747545 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. The commit message should also add "even if there is no active listener"

> The commit message should also add "even if there is no active listener" I'm not sure what you mean. The NotifyWakeLockChange calls are still guarded by if (sActiveListeners), as before.

I'd really like to write tests for this, but we shouldn't block on that. I'll figure it out in a separate bug.

(In reply to Justin Lebar [:jlebar] from comment #5) > > The commit message should also add "even if there is no active listener" > > I'm not sure what you mean. The NotifyWakeLockChange calls are still > guarded by if (sActiveListeners), as before. We didn't modify the hashtable if (!sActiveListeners) in RemoveChildFromList

> We didn't modify the hashtable if (!sActiveListeners) in RemoveChildFromList Okay, we can call that bug fix out in the commit message. Is this (in particular, (2)) more like what you had in mind? Bug 870480 - Notify wake lock listeners when a process adds or removes a wake lock, even if the wake lock itself doesn't change state. This also fixes two other bugs: 1) If a process crashed while holding a wake lock, we'd send a wake lock changed notification without including the list of locking processes. This could cause us to incorrectly believe that no processes were holding the lock. 2) If a process crashed while holding a wake lock on topic X, and if no other process held a wake lock on that topic, and there were no wake lock listeners registered, we wouldn't remove X from our hashtable of wake lock topics. We would correctly record the fact that the lock was not held, so this bug was benign.

Yes!

Great. Thanks again for the quick review here. https://hg.mozilla.org/projects/birch/rev/150241980e66

And a follow-up to hopefully fix gcc -werror: https://hg.mozilla.org/projects/birch/rev/3044afa610b2

https://hg.mozilla.org/mozilla-central/rev/150241980e66 https://hg.mozilla.org/mozilla-central/rev/3044afa610b2

https://hg.mozilla.org/releases/mozilla-b2g18/rev/f50cda3b98f4 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/6dcc5cbadb48 Includes the bustage fix.

Based on the comments in bug 871015, this introduced a test failure that was fixed by disabling the test on Android.

Correct; this is a result of what I wrote in comment 2: > A side-effect of this patch is that we make bug 870444 worse: We now fire more unnecessary wakelock > changed events. Doing this right is tricky, and since these events aren't exposed to anything but > certified code, I don't mind this minor regression. The test failure is benign. It's not good that I had to introduce this, but I had to do this to meet the tef deadline.