New issue
Advanced search Search tips

Issue 780855 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

WebRTCEventLogHost::StopWebRTCEventLog() not called when a PeerConnection is torn down

Project Member Reported by eladalon@chromium.org, Nov 2 2017

Issue description

This leads to number_active_log_files_ never being decremented, which in turn leads us to have up to 5 logs per Chrome session, rather than up to 5 ACTIVE logs per Chrome session.
 
Status: Started (was: Untriaged)
Description: Show this description
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 7 2017

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

commit d138e38ab67e6badaacbd041dcdd67eeeaadd735
Author: Elad Alon <eladalon@chromium.org>
Date: Tue Nov 07 14:36:25 2017

Unregister RTCPeerConnectionHandler upon native PC's Close()

The existing code only unregisters the handler it is destructed. 
However, garbage collection might not destroy the object for a while.

Bug:  780855 
Change-Id: I68d2c1b49cc5042d53464878977400bfd3b75c81
Reviewed-on: https://chromium-review.googlesource.com/754838
Commit-Queue: Elad Alon <eladalon@chromium.org>
Reviewed-by: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514466}
[modify] https://crrev.com/d138e38ab67e6badaacbd041dcdd67eeeaadd735/content/renderer/media/rtc_peer_connection_handler.cc

Status: Fixed (was: Started)
Anatoli, this is too new for git-branch to let me know into which cut it's made it. I will update when you ping me. :-)
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 7 2017

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

commit c497f4cf075d4f5253822b99183d2292049c8607
Author: Elad Alon <eladalon@chromium.org>
Date: Tue Nov 07 16:14:27 2017

Fix limit on concurrent RTC event log files

WebRTCEventLogHost::StopWebRTCEventLog() is not called when a 
PeerConnection is torn down. This leads to number_active_log_files_ 
never being decremented, which in turn leads us to have up to 5 logs 
per Chrome session, rather than up to 5 ACTIVE logs per Chrome session.

Bug:  780855 
Change-Id: I06e916136bc539d6783a91d8967f8a09f0f31c9e
Reviewed-on: https://chromium-review.googlesource.com/751666
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Commit-Queue: Elad Alon <eladalon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514481}
[modify] https://crrev.com/c497f4cf075d4f5253822b99183d2292049c8607/content/browser/webrtc/webrtc_eventlog_host.cc
[modify] https://crrev.com/c497f4cf075d4f5253822b99183d2292049c8607/content/browser/webrtc/webrtc_eventlog_host.h

Project Member

Comment 6 by bugdroid1@chromium.org, Nov 12 2017

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

commit 443c81f945a354dd3b11d220895b9c98009552f0
Author: Benjamin Joyce <bjoyce@google.com>
Date: Sun Nov 12 21:52:50 2017

Revert "Unregister RTCPeerConnectionHandler upon native PC's Close()"

This reverts commit d138e38ab67e6badaacbd041dcdd67eeeaadd735.

Reason for revert: assessing if cause for https://bugs.chromium.org/p/chromium/issues/detail?id=783010

Original change's description:
> Unregister RTCPeerConnectionHandler upon native PC's Close()
> 
> The existing code only unregisters the handler it is destructed. 
> However, garbage collection might not destroy the object for a while.
> 
> Bug:  780855 
> Change-Id: I68d2c1b49cc5042d53464878977400bfd3b75c81
> Reviewed-on: https://chromium-review.googlesource.com/754838
> Commit-Queue: Elad Alon <eladalon@chromium.org>
> Reviewed-by: Henrik Boström <hbos@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#514466}

TBR=hbos@chromium.org,guidou@chromium.org,terelius@chromium.org,eladalon@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  780855 
Change-Id: I567bfb7f5aa7f2658b1dbee06a3fe5b66f3dec4b
Reviewed-on: https://chromium-review.googlesource.com/765147
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Commit-Queue: Benjamin Joyce <bjoyce@google.com>
Cr-Commit-Position: refs/heads/master@{#515870}
[modify] https://crrev.com/443c81f945a354dd3b11d220895b9c98009552f0/content/renderer/media/rtc_peer_connection_handler.cc

Status: Assigned (was: Fixed)
We forgot to reopen the bug when unrolling the fix.
Project Member

Comment 8 by bugdroid1@chromium.org, Feb 8 2018

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

commit 8e05bb782597ac21ac3e42abed0fe08243ce69fd
Author: Elad Alon <eladalon@chromium.org>
Date: Thu Feb 08 16:24:45 2018

Propagate PeerConnection closing signal to WebRtcEventLogManager

Propagate he signal that a PeerConnection was closed to
WebRtcEventLogManager. This is done separately from its
removal, since removal waits for garbage collection.

Bug:  780855 ,  805398 , 775415
Change-Id: I8f40cc2a3b6c859c9c185b3b90b56a21305186d9
Reviewed-on: https://chromium-review.googlesource.com/904003
Commit-Queue: Elad Alon <eladalon@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535398}
[modify] https://crrev.com/8e05bb782597ac21ac3e42abed0fe08243ce69fd/content/browser/renderer_host/media/peer_connection_tracker_host.cc
[modify] https://crrev.com/8e05bb782597ac21ac3e42abed0fe08243ce69fd/content/browser/webrtc/webrtc_event_log_manager.cc
[modify] https://crrev.com/8e05bb782597ac21ac3e42abed0fe08243ce69fd/content/browser/webrtc/webrtc_event_log_manager.h

Status: Fixed (was: Assigned)
For posterity - that exact class (WebRTCEventLogHost) has since been removed, but the issue has been resolved by the attached CL (WebRtcEventLogManager now gets an indication that the PeerConnection has been stopped).

Sign in to add a comment