Closed Bug 868943 Opened 7 years ago Closed 7 years ago

HTMLMediaElement::mWakeLock has to be unlocked and not just set to null

Categories

(Core :: DOM: Core & HTML, 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: baku, Assigned: baku)

References

Details

Attachments

(2 files, 4 obsolete files)

Follow up of Bug 868325 Comment 7
Attached patch patch (obsolete) — Splinter Review
This patch moves AddSystemEventListener() to a separated class.
Attachment #745874 - Flags: review?(justin.lebar+bug)
The canonical way of doing this is to use a weak ref to the wake lock (and make the wake lock implement nsSupportsWeakReference if necessary).  This is safer than using raw pointers.

Also, please unregister these listeners when the DOM element goes away.  In fact, please hold a weak ref to the DOM element as well.  Otherwise we have the potential for problems with raw pointers there, too.
Oh, it looks like you already had weak refs to
Summary: WakeLock is not released when it set to null → WakeLocks are leaked forever (?)
> Oh, it looks like you already had weak refs to

Sorry, I misunderstood this patch.  Give me a few more minutes.

This looks like a pretty bad bug; thanks for catching it.
Summary: WakeLocks are leaked forever (?) → WakeLocks are leaked for the lifetime of their document
Comment on attachment 745874 [details] [diff] [review]
patch

Okay, I understand this better now.  :)

I think we actually can't do this; I think we have to leave it how it was and add a comment to the WakeLock class saying that C++ callers should not rely on the WakeLock automatically unlocking itself.  Or alternatively, we could do this for C++ callers and not JS callers, if we really wanted to.

Here's the problem: Suppose a page does

  var foo = navigator.requestWakeLock('cpu');
  foo = null;

With this patch, we'll unlock the wake lock once foo gets GC'ed.  That's not good; we don't like to expose GC behavior to pages.  I think the right thing to do is what we do now, which is to keep the lock held until the page closes.

Anyway if we /were/ to do this, I'd want you to hold a weak ref to mWakeLock, particularly since it dangles after Detach().  :)
Attachment #745874 - Flags: review?(justin.lebar+bug) → review-
Attached patch patch (obsolete) — Splinter Review
Attachment #745874 - Attachment is obsolete: true
Attachment #745900 - Flags: review?(justin.lebar+bug)
Attached patch patch (obsolete) — Splinter Review
more testing
Attachment #745900 - Attachment is obsolete: true
Attachment #745900 - Flags: review?(justin.lebar+bug)
Attachment #745903 - Flags: review?(justin.lebar+bug)
>diff --git a/content/html/content/test/test_wakelocks.html b/content/html/content/test/test_wakelocks.html
>new file mode 100644
>--- /dev/null
>+++ b/content/html/content/test/test_wakelocks.html

test_audio_wakelock.html or test_audio_element_wakelock.html, maybe?

>+function testAudioPlay() {
>+  var lockState = true;
>+  var count = 0;
>+
>+  var content = document.getElementById('content');
>+
>+  var audio = document.createElement('audio');
>+  audio.src = "silence.ogg";
>+  content.appendChild(audio);
>+
>+  navigator.mozPower.addWakeLockListener(function testAudioPlayListener(topic, state) {
>+    is(topic, "cpu", "Audio element locked the target == cpu");
>+    var locked = state == "locked-foreground" ||
>+                 state == "locked-background";
>+
>+    is(locked, lockState, "Audio element locked the cpu - no paused");
>+    count++;
>+
>+    if (count == 1) {
>+      lockState = false;

This one needs a comment explaining what's going on.

Also, please add a comment to the WakeLock class indicating that it lives for
the lifetime of the document and explaining why it has to be this way, so we
don't make this same mistake again.  (We should probably call
RemoveEventListeners() from DoUnlock(), but that's not a big deal.)
Can you also edit the bug summary to reflect what we're doing here?
Attachment #745903 - Flags: review?(justin.lebar+bug) → review+
If you mark this as blocking the <audio> bug where we introduced this bug, I can mark this as tef+, since it's a serious regression.
Summary: WakeLocks are leaked for the lifetime of their document → HTMLMediaElement::mWakeLock has to be unlocked and not just set to null
Depends on: 859824
Attached patch patch (obsolete) — Splinter Review
A new quick review
Attachment #745903 - Attachment is obsolete: true
Attachment #745910 - Flags: review?(justin.lebar+bug)
Can we make a C++ RAII helper for this?
Comment on attachment 745910 [details] [diff] [review]
patch

>+  // Note: WakeLock lives for the lifetime of the document in order to avoid to
>+  // expose GC behavior to pages. This means that
>+  // |var foo = navigator.requestWakeLock('cpu'); foo = null;|
>+  // doesn't unlock the 'cpu' resource.

Nit: s/to expose GC behavior/exposing GC/

Of course there's no rule about when to use an infinitive versus a gerund in a
sentence like this.  It depends on the main verb.  And sometimes either is
acceptable, but the meaning is different depending on which you use!  See
"Comparing Gerunds and Infinitives" in
http://owl.english.purdue.edu/owl/resource/627/04/; notice that "avoid" is
listed under "verbs that take only gerunds as verbal direct objects."

>+    if (count == 1) {
>+      // The next step is to unlock the resource.

Sorry, I should have been more specific.  I was hoping you'd explain why we
expect the resource to be unlocked after this point.  It's totally implicit
here.

I just checked, and I think the "silence" file is too long for this particular
test.  You want it to be long when you're checking .pause(), because you don't
want the file to finish playing in the middle of that test.  But here, we don't
want to wait 20s for the test to complete.
Attachment #745910 - Flags: review?(justin.lebar+bug) → review+
Attached patch b2g18 patchSplinter Review
Attachment #745910 - Attachment is obsolete: true
Attachment #746278 - Flags: review+
Keywords: checkin-needed
Attached patch gecko m-cSplinter Review
Attachment #746419 - Flags: review+
Attachment #746278 - Attachment description: patch → b2g18 patch
https://hg.mozilla.org/mozilla-central/rev/3b401ef43264
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
This  bug must be a tef+ becuase it's a regression that has been introduced by bug 859824
blocking-b2g: --- → tef?
And the regression is, we don't unlock the CPU wake lock, which is very bad.
jlebar, can you mark this bug as tef+ ? Patch for b2g18 is ready. We have just to land it.
Flags: needinfo?(justin.lebar+bug)
critical power regression from a tef+ bug, which makes this tef+
blocking-b2g: tef? → tef+
Flags: needinfo?(justin.lebar+bug)
Depends on: 876210
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.