Closed Bug 870480 Opened 7 years ago Closed 7 years ago

Notify wake lock listeners when a process adds or removes a wake lock, even if the wake lock itself doesn't change state.

Categories

(Core :: Hardware Abstraction Layer (HAL), defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23
blocking-b2g tef+
Tracking Status
firefox21 --- wontfix
firefox22 --- wontfix
firefox23 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

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.
blocking-b2g: --- → tef+
Assignee: nobody → justin.lebar+bug
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?
Attachment #747545 - Flags: review?(kchen)
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.
Whiteboard: [status: needs review]
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"
Attachment #747545 - Flags: review?(kchen) → review+
> 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.
Depends on: 870675
(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.
Great.  Thanks again for the quick review here.

https://hg.mozilla.org/projects/birch/rev/150241980e66
Blocks: 847592
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
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [status: needs review] → [status: needs uplift]
Target Milestone: --- → mozilla23
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.
Depends on: 871015
You need to log in before you can comment on or make changes to this bug.