tommi: Can someone from your team look into this?

The PowerSaveBlocker is created in WebRTCInternals::OnAddPeerConnection() (here: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/media/webrtc/webrtc_internals.cc&l=103). And, it is removed by a call to WebRTCInternals::OnRemovePeerConnection().

Seems the original poster is saying this is too coarse-grained in that we are power-save blocking for the entire lifetime of a PeerConnection object, but should only be blocking while the PC is open and connected to a peer.

History:  bug 400552 

(I've touched this code and am happy to do code reviews here.)

I agree; I believe that the original intent of this code was to only block power management while the peer is connected.

miu - can you provide some background on why the powersaveblocker is in webrtc_internals?

I'm interpreting your question two ways:

1. What functionality does it provide? Prevents the system from sleeping while remoting content (whether that's webcam content, or screen captured content). For example, if you were mirroring your presentation Slides to a Hangout, you probably wouldn't want your laptop to go to sleep.

2. Why does it exist where it is and not some other code module? Entirely my fault if it's in the wrong place. I'm the one that added it (over a year ago). At the time, this looked like the right place to do it, and this code was conveniently being run in the browser process, on the right thread, and seemed to track the PC life-cycle accurately. Of course, the OP has pointed out the power save block should be released earlier, when the PC is closed, and not when it is eventually (many seconds/minutes/hours later) auto-garbage-collected.

tommi: To add: Even though I originally put this in-place, I want it to be up your team to evaluate the various known use cases and decide: a) whether it's a good idea to keep it; b) desired end-user experience; and c) when/where power save blocking should be turned on/off.

Oh sorry, I mean because webrtc-internals code should only be related to
chrome://webrtc-internals. If that page isn't being viewed, the code should
be as passive as possible. It's only supposed to be used for monitoring. It
looks now like there's actually some auxiliary functionality in there.

That makes sense. I'll take a look tomorrow.

guido - can you take a look at this?

I had initially thought that the webrtc-internals code wasn't the best place to keep the powersaveblocker, but I don't think this is much of an issue actually.

The issue is still that powerblock shouldn't be activated if there are no currently active peerconnections.  Maybe this could be improved by handling the "stop" update notifications and keep track of what peerconnection objects have connected tracks and which ones have been stopped.

 Issue 664462  has been merged into this issue.

 Issue 660610  has been merged into this issue.

This looks like the cause of the bug I've noticed for quite some time (but have been unable to diagnose very far). At some point in the past few months my Mac laptops have stopped going to sleep when connected to power. I've suspected Chrome but couldn't confirm it. This is a bad bug that needs to be fixed ASAP.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/542c74f6d402696e770d70a8716dbcf0ffe53094

commit 542c74f6d402696e770d70a8716dbcf0ffe53094
Author: guidou <guidou@chromium.org>
Date: Thu Nov 17 18:34:28 2016

Release PowerSaveBlock on PeerConnection stop instead of removal.
Removal occurs at GC time, which may result in an undeterminate amount of time with power saving blocked.

Drive-by: fix a couple of lint issues.

BUG= 612294 

Review-Url: https://codereview.chromium.org/2502413002
Cr-Commit-Position: refs/heads/master@{#432920}

[modify] https://crrev.com/542c74f6d402696e770d70a8716dbcf0ffe53094/content/browser/webrtc/webrtc_internals.cc
[modify] https://crrev.com/542c74f6d402696e770d70a8716dbcf0ffe53094/content/browser/webrtc/webrtc_internals.h
[modify] https://crrev.com/542c74f6d402696e770d70a8716dbcf0ffe53094/content/browser/webrtc/webrtc_internals_unittest.cc

shrike@: can you verify the fix with the most recent canary for Mac?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4d3f975a472365ec195da4b8b5b75f28219182b7

commit 4d3f975a472365ec195da4b8b5b75f28219182b7
Author: guidou <guidou@chromium.org>
Date: Fri Nov 18 10:36:06 2016

Mark new accessor methods in WebRTCInternals as const.

This addresses a comment in a previous CL.

BUG= 612294 
TBR=tommi@chromium.org

Review-Url: https://codereview.chromium.org/2516573003
Cr-Commit-Position: refs/heads/master@{#433168}

[modify] https://crrev.com/4d3f975a472365ec195da4b8b5b75f28219182b7/content/browser/webrtc/webrtc_internals.h

I have just tested with the latest canary. It is working correctly. Thanks.

OmahaProxy says c#13 landed in 56.0.2924.0, and I'm running 56.0.2924.0 Canary.

Without following any steps to try to reproduce I ran

    pmset -g assertions

from the command line and got the following:

   pid 89518(Google Chrome Canary): [0x0000c52c0001273d] 00:59:15 NoIdleSleepAssertion named: "WebRTC has active PeerConnections" 

This is after closing all tabs. Seems like the issue is not fixed?

I still get "WebRTC has active PeerConnections" with all tabs closed.

There's an email thread suggesting that this may also be showing up on Chrome OS 56.0.2924.87. See e.g. http://feedback/product/208/neutron?lView=rd&lReport=52828819623.

This is a critical bug because it drains the battery while the machine is sleep and disconnected from power.

I cannot reproduce with the instructions on this bug. If the PeerConnections are closed, the Chrome disappears from the output of "pmset -g assertions".

shrike@: Can you provide new repro instructions?

I don't know that this covers all cases but I can repro with the following steps:

1. Launch Chrome - pmset -g assertions shows sleep not prevented
2. Open a new Hangout - pmset -g assertions shows sleep prevented
3. Find the Hangout process in Task Manager and kill the process

After that, pmset -g assertions shows Chrome preventing sleep even though the hangout no longer exists.

Looking at the code for WebRTCInternals::OnRendererExit(), it appears that you call peer_connection_data_.Remove() without first calling MaybeClosePeerConnection(), as you do in OnRemovePeerConnection(). Without that call, num_open_connections_ never gets decremented and the power save block never goes away. I looked over the rest of the code and it seems to be correct, but I think it's difficult to reason about its correctness (two different paths handling the closing of peer connections, the implicit and delicate interrelationship of num_open_connections_, MaybeClosePeerConnection(), and peer_connection_data_.Remove()).

I don't believe users are running into this bug by their killing of renderers in the Task Manager; I have to believe there's some other path getting them to OnRendererExit(). In going through my tabs looking for one that might be preventing sleep, I noticed that there were a number that were sad tabs or blank. If a tab is continually active it can end up never seeing a GC event (that's another something I need to pursue), so an overly active tab blocking power saving could be killed off after reaching a memory limit.

I am definitely seeing issues related to sleep on mac.  Running Version 56.0.2924.87 (64-bit) 

And seeing:

  pid 490(Google Chrome): [0x0000d71800018c93] 26:17:03 NoIdleSleepAssertion named: "WebRTC has active PeerConnections" 

In pmset -g assertions

alex@: Do you see that message when there are no active PeerConnections?

Thanks for the repro instructions, shrike@. I can reproduce too.
MaybeClosePeerConnection() should be called in OnRendererExit(). Otherwise, crashed tabs will leave sleeping disabled (patch sent for review).
I think this will cover most, if not all, of the remaining cases of PeerConnections incorrectly preventing sleep.

Yes, still there with no active connections.  From webrtc-internals:

{
 "getUserMedia": [],
 "PeerConnections": {}
}

alex@moore.mx: Do you have a way to reproduce that case?

guidou: I've been struggling with this for a couple of days. Happy to find that it is a known issue. I'll try to find a way to reproduce. Since I work a lot with the WebRTC API I might find something.

For me, I seem to be able to fairly consistently reproduce by starting chrome, opening webrtc-internals tab - verify no open rtc connections, check pmset assertions and verify no Chrome assertion is listed.  Then go to : https://test.webrtc.org/ - start the test, wait until at least the 640x480 video test, close the tab, validate on the web-rtc internals that the session has cleared.  Check pmset assertions and it still has a power assertion set due to "WebRTC has active PeerConnections".  I have noticed it is not 100% reproducible...that perhaps if you close the test too early it does clear up the power assertion - perhaps it is dependent on the video elements? I would need to experiment more.

I was able to consistently reproduce the problem using the steps in c#33, and could no longer reproduce it with those steps after applying the patch in codereview 2693263003.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/223163006109b9b54a8421f4ca8139403ad63e0d

commit 223163006109b9b54a8421f4ca8139403ad63e0d
Author: guidou <guidou@chromium.org>
Date: Wed Feb 15 19:15:03 2017

Release RTCPeerConnection PowerSaveBlock on process exit.

Also, do cleanup on RenderProcessExited() instead of
RenderProcessHostDestroyed() as the latter can take an indeterminate
amount of time to be called.

BUG= 612294 

Review-Url: https://codereview.chromium.org/2693263003
Cr-Commit-Position: refs/heads/master@{#450760}

[modify] https://crrev.com/223163006109b9b54a8421f4ca8139403ad63e0d/content/browser/webrtc/webrtc_internals.cc
[modify] https://crrev.com/223163006109b9b54a8421f4ca8139403ad63e0d/content/browser/webrtc/webrtc_internals.h

Marking as fixed, as per comment #34. I had the same results as shrike@.
The fix is already in Canary and will request merge for M57.

Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Please merge your change to M57 branch 2987 before 5:00 PM PT Friday (02/17), so we can pick it up for next week Beta release. Thank you.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4855c33b4687d4ebe35f384652f59872a19fb52b

commit 4855c33b4687d4ebe35f384652f59872a19fb52b
Author: Guido Urdaneta <guidou@chromium.org>
Date: Fri Feb 17 10:24:24 2017

Release RTCPeerConnection PowerSaveBlock on process exit.

Also, do cleanup on RenderProcessExited() instead of
RenderProcessHostDestroyed() as the latter can take an indeterminate
amount of time to be called.

BUG= 612294 

Review-Url: https://codereview.chromium.org/2693263003
Cr-Commit-Position: refs/heads/master@{#450760}
(cherry picked from commit 223163006109b9b54a8421f4ca8139403ad63e0d)

Review-Url: https://codereview.chromium.org/2705583002 .
Cr-Commit-Position: refs/branch-heads/2987@{#571}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/4855c33b4687d4ebe35f384652f59872a19fb52b/content/browser/webrtc/webrtc_internals.cc
[modify] https://crrev.com/4855c33b4687d4ebe35f384652f59872a19fb52b/content/browser/webrtc/webrtc_internals.h

 Issue 693804  has been merged into this issue.

Comment from gaocuo@gmail.com on issue 689017:

" Chrome version: 59.0.3050.3 canary (64-bit) still have this bug.

you can open https://aixuexi.tmall.com/?ali_trackid=17_802f2a00025e4fd75b683ed4d94c923e 

when this tab open, use "pmset -g assertions" result:

  pid 52034(Google Chrome Canary): [0x0003a64d0001858e] 00:02:59 NoIdleSleepAssertion named: "WebRTC has active PeerConnections""

I took a look at that site; it's just using webrtc to scrape the user's IP address, and it doesn't bother to close the PeerConnection. So this seems like expected behavior.

Reopening as per #43.

I misread #43. Marking as  Fixed again.

In Chrome go to Advanced Settings

remove the check marks in:
Continue running background apps when Google Chrome is closed
Use hardware acceleration when available

Then open Administrator Windows PowerShell, windows key + X
type: powercfg -requests

I hope this helps you, 

Deleted: PeerConnection.jpg 686 KB

	PeerConnection.jpg 686 KB View Download

Deleted: PeerConnection-2.jpg 69.2 KB

	PeerConnection-2.jpg 69.2 KB View Download

Go to YouTube play a video to test, then pause that video then 
run the powercfg -requests again