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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrea Marchesini [:baku]]

Follow up of Bug 868325 Comment 7

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

This patch moves AddSystemEventListener() to a separated class.

Comment 2

[Justin Lebar (not reading bugmail)]

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.

Comment 3

[Justin Lebar (not reading bugmail)]

Oh, it looks like you already had weak refs to

Comment 4

[Justin Lebar (not reading bugmail)]

> 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.

Comment 5

[Justin Lebar (not reading bugmail)]

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().  :)

Comment 6

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch

more testing

Comment 8

[Justin Lebar (not reading bugmail)]

>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.)

Comment 9

[Justin Lebar (not reading bugmail)]

Can you also edit the bug summary to reflect what we're doing here?

Comment 10

[Justin Lebar (not reading bugmail)]

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.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: patch

A new quick review

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Can we make a C++ RAII helper for this?

Comment 13

[Justin Lebar (not reading bugmail)]

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.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: b2g18 patch

Comment 15

[Andrea Marchesini [:baku]]

Attachment: gecko m-c

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/3b401ef43264

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3b401ef43264

Comment 18

[Andrea Marchesini [:baku]]

This  bug must be a tef+ becuase it's a regression that has been introduced by bug 859824

Comment 19

[Justin Lebar (not reading bugmail)]

And the regression is, we don't unlock the CPU wake lock, which is very bad.

Comment 20

[Andrea Marchesini [:baku]]

jlebar, can you mark this bug as tef+ ? Patch for b2g18 is ready. We have just to land it.

Comment 21

[Alex Keybl [:akeybl]]

critical power regression from a tef+ bug, which makes this tef+

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/f70b9426bb50
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/708513fa9502