Closed Bug 1373888 Opened 3 years ago Closed 3 years ago

Firefox prevents my Macbook pro from sleeping automatically

Categories

(Core :: Audio/Video: Playback, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: ntim, Assigned: alwu)

References

Details

Attachments

(9 files)

After having ~100 tabs open, Firefox seems to be preventing my MBP from automatically sleeping: no videos are playing, neither is any music. And it's pretty annoying to be honest.


To check what app is preventing OSX from sleeping automatically, you can go to Activity Monitor, click on the Energy tab. Normally there's a column indicating which apps are preventing the macbook to sleep.
Thanks for reporting this issue. Could you please provide your user agent from about:support page of your browser? 

Also do you have ~100 tabs open in one browser window ? 
Can you provide a screenshot showing that Firefox is preventing your Mac from sleep automatically ?
Flags: needinfo?(ntim.bugs)
Resolved-Incomplete due to time since last update by reporter.
Please feel free to reopen if the error occurs in a current Firefox version.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INCOMPLETE
Sorry for the delay, this appears to happen when there's a video/audio open in one tab. But I think videos/audios shouldn't prevent sleep if they are paused or stopped.
Flags: needinfo?(ntim.bugs)
Thanks ntim.

Hey k17e, any idea who the right person is to point this at?
Status: RESOLVED → REOPENED
Flags: needinfo?(ajones)
Resolution: INCOMPLETE → ---
JW - can you take a look at this one?
Flags: needinfo?(ajones) → needinfo?(jwwang)
Priority: -- → P1
Chris thinks the issue has something to do with interaction between suspend/resume and inhibiting the screen saver.
Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core
Can you:
1. install this add-on, https://addons.mozilla.org/en-GB/firefox/addon/devtools-media-panel/.
2. option+command+K to open the web console and switch to the 'Media' tab.
3. press 'Start' and copy the text.

Thanks!
Flags: needinfo?(jwwang) → needinfo?(ntim.bugs)
Flags: needinfo?(ntim.bugs) → needinfo?(janx)
Flags: needinfo?(ntim.bugs)
Not sure if this is related, but lately Nightly is preventing me from shutting down my Windows 10 laptop.

I have to force quit Nightly in order to be able to shut down (waiting doesn't help).
Flags: needinfo?(janx)
I'm wondering whether you have the silent video or audio is playing on one of those tabs, so you didn't aware about that. Now we would request wake lock once the media starts, but we don't check whether it's audible or not.
Flags: needinfo?(janx)
Considering user might watch movie but mute it in some quite place. SO, my idea is, cancel wake lock for "the video which doesn't have audio track" or "video with silent audio track" and "silent audio".
(In reply to Alastor Wu [:alwu][please needinfo me][GMT+8] from comment #9)
> I'm wondering whether you have the silent video or audio is playing on one
> of those tabs, so you didn't aware about that. Now we would request wake
> lock once the media starts, but we don't check whether it's audible or not.

I was wrong, the wake lock would be released when the video is hidden [1]. So this issue might not be related with video.

[1] http://searchfox.org/mozilla-central/rev/13148faaa91a1c823a7d68563d9995480e714979/dom/html/HTMLVideoElement.cpp#335
Flags: needinfo?(janx)
After testing with Chrome, they would request wake lock if audio or video is playing and can be aware by user (eg. video is in in the foreground, audio is audible). 

For example, 
1. playing visible video and it's also audible 
  - request the wake lock
2. turn the video tab goes to background 
  - still own the wake lock, audio is audible
3. turn the video tab goes to foreground and mute it 
  - still own the wake lock, video is visible
4. turn the video tab goes to background 
  - release wake lock, because user can't aware of both audio and video

Note. 
The mute in step3 is to change value via |video.muted=true| or |video.volume=0|, it's not equal to mute the video via tab's menu. (it's equal to our sound indicator)
Chrome would still request wake lock if we mute the video via tab's menu (right click, and choose mute tab), but I think it's not reasonable. I would think it's a bug, not a normal behavior.

---

I would make most our behavior as same as Chrome's, but change a little bit. 
Like above I mentioned, I would release the wake lock if the video is muted via sound indicator.
Assignee: nobody → alwu
I found that the wake lock [1] we require in media element didn't work on OSX, because MacWakeLockListener only accepts topic "sceen" [2].

I'll change their name to "audio-playing" and "video-playing", it's more clear for knowing the acutal usage of the wake lock.

For example, 
Chrome use "Playing audio" and "Playing video" as their wake lock topic, it's very clear. You can see the OSX's power usage details using "$ pmset -g assertions"

[1] http://searchfox.org/mozilla-central/rev/5ce320a16f6998dc2b36e30458131d029d107077/dom/html/HTMLMediaElement.cpp#4234

[2] http://searchfox.org/mozilla-central/rev/5ce320a16f6998dc2b36e30458131d029d107077/widget/cocoa/nsAppShell.mm#63
Comment on attachment 8898639 [details]
Bug 1373888 - part1 : remove useless function.

https://reviewboard.mozilla.org/r/169972/#review175756

(Looks like dom.wakelock.enabled is false by default.)
Attachment #8898639 - Flags: review?(bugs) → review+
See Also: → 1358454
Comment on attachment 8898639 [details]
Bug 1373888 - part1 : remove useless function.

https://reviewboard.mozilla.org/r/169972/#review176076
Attachment #8898639 - Flags: review?(cpearce) → review+
Comment on attachment 8898640 [details]
Bug 1373888 - part2 : remove the timer which was used for b2g.

https://reviewboard.mozilla.org/r/169974/#review176078
Attachment #8898640 - Flags: review?(cpearce) → review+
Comment on attachment 8898641 [details]
Bug 1373888 - part3 : rename the topic of the wake lock.

https://reviewboard.mozilla.org/r/169976/#review176082
Attachment #8898641 - Flags: review?(cpearce) → review+
Comment on attachment 8898642 [details]
Bug 1373888 - part4 : request non-display wake lock for audio playing.

https://reviewboard.mozilla.org/r/169978/#review176084
Attachment #8898642 - Flags: review?(cpearce) → review+
Comment on attachment 8898643 [details]
Bug 1373888 - part5 : only request audio wake lock when it's audible.

https://reviewboard.mozilla.org/r/169980/#review176080

::: dom/html/HTMLMediaElement.cpp:4179
(Diff revision 3)
>  
>    bool playing = !mValue;
> -
> -  if (playing) {
> +  bool isAudible = mOuter->Volume() > 0.0 &&
> +                   !mOuter->mMuted &&
> +                   mOuter->mIsAudioTrackAudible;
> +  // only create wake lock when playing audible audio.

I would say "when playing audible media" (as you said down below), as this logic covers both audio and video.
Attachment #8898643 - Flags: review?(cpearce) → review+
I still don't understand how we end up with the screensaver disabled but no obvious audio or video playing, as this bug was filed for? When I've seen this problem on my MBP I don't see any tabs with an audio indicator showing. Could there be a muted audio playing which causes the screen saver to lock?
Flags: needinfo?(alwu)
(In reply to Chris Pearce (:cpearce) from comment #31)
> I still don't understand how we end up with the screensaver disabled but no
> obvious audio or video playing, as this bug was filed for? When I've seen
> this problem on my MBP I don't see any tabs with an audio indicator showing.
> Could there be a muted audio playing which causes the screen saver to lock?

No, the problem is caused by three reasons, I'll upload new patches to fix it.

I also found the reproduce steps,

STR.
1. open youtube and playing any video
2. open another youtube video in another tab
3. switch tabs multiple times
4. you would see the leak wakelocks by "$ pmset -g assertions"

---

* create/release wakelock depend on hidden status
 
We would release the wakelock when the tab becomes invisible [1]. However, the wakelock would do that by itself, it would automatically handle the lock/unlock by listening "hidden/visible" [2]. 

[1] http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/dom/html/HTMLVideoElement.cpp#333

[2] http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/dom/power/WakeLock.cpp#260-268


* wakelock won't be released if there are multiple wakelocks

See above STR step(2), we would create new wakelock first, and then release old one. However, at that time,
the wakelock state would be WAKE_LOCK_STATE_VISIBLE [3], and  wakelock listener would get the "locked-foreground" state[4].

Although the HTMLVideoElement call unlock, the wakelock listener won't release the os-lock, it would create a new os-lock, because we only release wakelock when state is "locked-background" or "unlocked". [5]

[3] http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/hal/HalWakeLock.cpp#176

[4] http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/dom/power/PowerManagerService.cpp#75

[5] http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/widget/cocoa/nsAppShell.mm#84


*  MacWakeLockListener doesn't consider the multiple lock

Everytime we create the new lock, we would remember the lock-id [6], and use it to release the specific os-lock. However, if we got the "locked-foreground" twice, the old id would be overwritten by the new id, and it causes we can't release the old os-lock.

[6] http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/widget/cocoa/nsAppShell.mm#79
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(alwu)
Duplicate of this bug: 1358454
Comment on attachment 8899788 [details]
Bug 1373888 - part7 : modify platform wakelocks.

https://reviewboard.mozilla.org/r/171120/#review176784
Attachment #8899788 - Flags: review?(snorp) → review+
Comment on attachment 8898642 [details]
Bug 1373888 - part4 : request non-display wake lock for audio playing.

https://reviewboard.mozilla.org/r/169978/#review176986

r+ for the cocoa parts, with nit addressed. Thanks!

::: widget/cocoa/nsAppShell.mm:70
(Diff revision 4)
>          !aTopic.EqualsASCII("audio-playing") &&
>          !aTopic.EqualsASCII("video-playing")) {
>        return NS_OK;
>      }
> +
> +    bool shouldKeepDisplayOn = !aTopic.EqualsASCII("audio-playing");

nit: This is identical behavior, but I would prefer that we specifically set shouldKeepDisplayOn to true if aTopic.EqualsASCII("screen") || aTopic.EqualsASCII("video-playing") is true. It makes for easier reading and the reader does not have to remember the three possible topics. The same applies on the Windows side.
Attachment #8898642 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8899788 [details]
Bug 1373888 - part7 : modify platform wakelocks.

https://reviewboard.mozilla.org/r/171120/#review177004

r+ for Cocoa parts. Thanks!
Attachment #8899788 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8899787 [details]
Bug 1373888 - part6 : only release the wakelock when video is paused.

https://reviewboard.mozilla.org/r/171118/#review178824

::: dom/html/HTMLVideoElement.cpp
(Diff revision 1)
>  {
>    return HTMLVideoElementBinding::Wrap(aCx, this, aGivenProto);
>  }
>  
> -void
> -HTMLVideoElement::NotifyOwnerDocumentActivityChanged()

I think this means that you can now make  HTMLMediaElement::NotifyOwnerDocumentActivityChanged() non-virtual.

Please can you remove "virtual" from HTMLMediaElement::NotifyOwnerDocumentActivityChanged().
Attachment #8899787 - Flags: review?(cpearce) → review+
Comment on attachment 8899788 [details]
Bug 1373888 - part7 : modify platform wakelocks.

https://reviewboard.mozilla.org/r/171120/#review178826

::: widget/windows/nsAppShell.cpp:92
(Diff revision 2)
>        SetThreadExecutionState(ES_CONTINUOUS);
>      }
>      return NS_OK;
>    }
> +
> +  bool mRequireForDisplay;

If you declared this:

  bool mRequireForDisplay = false;

Then you don't need to define the constructor. Then the code is more concise.
Attachment #8899788 - Flags: review?(cpearce) → review+
Comment on attachment 8899789 [details]
Bug 1373888 - part8 : add log.

https://reviewboard.mozilla.org/r/171122/#review178828
Attachment #8899789 - Flags: review?(cpearce) → review+
Thanks for fixing this bug, it's been annoying me for some time!
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eb9e9eb13016
part1 : remove useless function. r=cpearce,smaug
https://hg.mozilla.org/integration/autoland/rev/e4e4bda4e5ca
part2 : remove the timer which was used for b2g. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/0f86a8347393
part3 : rename the topic of the wake lock. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/ecc2c4a42ff9
part4 : request non-display wake lock for audio playing. r=cpearce,spohl
https://hg.mozilla.org/integration/autoland/rev/d5b01a46e823
part5 : only request audio wake lock when it's audible. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/02eceb0ab6fc
part6 : only release the wakelock when video is paused. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/c35f4075bd33
part7 : modify platform wakelocks. r=cpearce,snorp,spohl
https://hg.mozilla.org/integration/autoland/rev/8b6374396a5a
part8 : add log. r=cpearce
I would like to uplift these patches to beta, ask qe-verify before uplifting.
QA Whiteboard: qe
Flags: qe-verify?
(In reply to Alastor Wu [:alwu][please needinfo me][GMT+8] from comment #62)
> I would like to uplift these patches to beta, ask qe-verify before uplifting.

Looks as if we have a possible regression from this landing, see Bug 1395359
It's too late to uplift, just let it in the 57.
Flags: qe-verify?
Blocks: 1495064
You need to log in before you can comment on or make changes to this bug.