WebRTCInternals construction only allowed from scope that allows blocking |
||
Issue descriptionWebRTCInternals is a lazily constructed singleton whose constructor calls GetContentClient()->browser()->GetDefaultDownloadDirectory(), which is only allowed from a scope that allows blocking. That is brittle, because it's difficult to guarantee when a lazily constructed singleton would be constructed.
,
Jan 11 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ce383b564cd0ea6510551c77c81e13422d78f069 commit ce383b564cd0ea6510551c77c81e13422d78f069 Author: Friedrich Horschig <fhorschig@chromium.org> Date: Thu Jan 11 12:47:26 2018 Revert "Move ownership of WebRTCInternals to BrowserMainLoop" This reverts commit 0823211ed4a4abbd26ecb7df1408fe010ecb3412. Reason for revert: Compile on Cast Audio Linux broke https://ci.chromium.org/buildbot/chromium.linux/Cast%20Audio%20Linux/7958 Log message: error: undefined symbol: content::WebRTCInternals::CreateSingletonInstance() >>> referenced by browser_main_loop.cc:1572 (../../content/browser/browser_main_loop.cc:1572) Original change's description: > Move ownership of WebRTCInternals to BrowserMainLoop > > WebRTCInternals was a lazily-constructed singleton. This CL changes > this, to allow a similar change in WebRtcEventLogManager, where this > lazy construction (and subsequent leaking) created problems for > unit tests due to WebRtcEventLogManager's task runner. (The change > to WebRtcEventLogManager will be done in a separate, subsequent CL.) > > Piggy-backed on this - plug the leak in DownloadProtectionServiceTest.VerifyReferrerChainWithEmptyNavigationHistory > > Bug: 775415, 800404 , 792847 > Change-Id: Iddc2569933b83bbe3a4e16c161ac531748448477 > Reviewed-on: https://chromium-review.googlesource.com/857470 > Reviewed-by: John Abd-El-Malek <jam@chromium.org> > Commit-Queue: Elad Alon <eladalon@chromium.org> > Cr-Commit-Position: refs/heads/master@{#528597} TBR=jam@chromium.org,terelius@chromium.org,eladalon@chromium.org Change-Id: Ifdeac7829d2d0e940ed938555793f38410d30272 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 775415, 800404 , 792847 Reviewed-on: https://chromium-review.googlesource.com/860062 Reviewed-by: Friedrich Horschig <fhorschig@chromium.org> Commit-Queue: Friedrich Horschig <fhorschig@chromium.org> Cr-Commit-Position: refs/heads/master@{#528608} [modify] https://crrev.com/ce383b564cd0ea6510551c77c81e13422d78f069/chrome/browser/safe_browsing/download_protection/download_protection_service_unittest.cc [modify] https://crrev.com/ce383b564cd0ea6510551c77c81e13422d78f069/content/browser/browser_main_loop.cc [modify] https://crrev.com/ce383b564cd0ea6510551c77c81e13422d78f069/content/browser/browser_main_loop.h [modify] https://crrev.com/ce383b564cd0ea6510551c77c81e13422d78f069/content/browser/renderer_host/media/peer_connection_tracker_host.cc [modify] https://crrev.com/ce383b564cd0ea6510551c77c81e13422d78f069/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/ce383b564cd0ea6510551c77c81e13422d78f069/content/browser/webrtc/webrtc_internals.cc [modify] https://crrev.com/ce383b564cd0ea6510551c77c81e13422d78f069/content/browser/webrtc/webrtc_internals.h [modify] https://crrev.com/ce383b564cd0ea6510551c77c81e13422d78f069/content/browser/webrtc/webrtc_internals_message_handler.cc
,
Jan 11 2018
With the CL that was just landed, this is no longer lazily constructed, so it's no longer a problem.
,
Jan 13 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a619d5567e6e32af78321ea591cbfbd9a7ab2bc8 commit a619d5567e6e32af78321ea591cbfbd9a7ab2bc8 Author: Elad Alon <eladalon@chromium.org> Date: Sat Jan 13 01:30:15 2018 Revert "Revert "Move ownership of WebRTCInternals to BrowserMainLoop"" This reverts commit ce383b564cd0ea6510551c77c81e13422d78f069. Reason for revert: Fix the build-break introduced by the original CL and reland. Original change's description: > Revert "Move ownership of WebRTCInternals to BrowserMainLoop" > > This reverts commit 0823211ed4a4abbd26ecb7df1408fe010ecb3412. > > Reason for revert: Compile on Cast Audio Linux broke > https://ci.chromium.org/buildbot/chromium.linux/Cast%20Audio%20Linux/7958 > > Log message: > error: undefined symbol: content::WebRTCInternals::CreateSingletonInstance() > >>> referenced by browser_main_loop.cc:1572 (../../content/browser/browser_main_loop.cc:1572) > > Original change's description: > > Move ownership of WebRTCInternals to BrowserMainLoop > > > > WebRTCInternals was a lazily-constructed singleton. This CL changes > > this, to allow a similar change in WebRtcEventLogManager, where this > > lazy construction (and subsequent leaking) created problems for > > unit tests due to WebRtcEventLogManager's task runner. (The change > > to WebRtcEventLogManager will be done in a separate, subsequent CL.) > > > > Piggy-backed on this - plug the leak in DownloadProtectionServiceTest.VerifyReferrerChainWithEmptyNavigationHistory > > > > Bug: 775415, 800404 , 792847 > > Change-Id: Iddc2569933b83bbe3a4e16c161ac531748448477 > > Reviewed-on: https://chromium-review.googlesource.com/857470 > > Reviewed-by: John Abd-El-Malek <jam@chromium.org> > > Commit-Queue: Elad Alon <eladalon@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#528597} > > TBR=jam@chromium.org,terelius@chromium.org,eladalon@chromium.org > > Change-Id: Ifdeac7829d2d0e940ed938555793f38410d30272 > Bug: 775415, 800404 , 792847 > Reviewed-on: https://chromium-review.googlesource.com/860062 > Reviewed-by: Friedrich Horschig <fhorschig@chromium.org> > Commit-Queue: Friedrich Horschig <fhorschig@chromium.org> > Cr-Commit-Position: refs/heads/master@{#528608} TBR=jam@chromium.org,terelius@chromium.org,fhorschig@chromium.org,eladalon@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:cast_shell_audio_linux,fuchsia_arm64_cast_audio,fuchsia_x64_cast_audio No-Try: true Bug: 775415, 800404 , 792847 Change-Id: I28f34f417cfde490710a82a3e29711a0f1e9b15c Reviewed-on: https://chromium-review.googlesource.com/860404 Commit-Queue: Elad Alon <eladalon@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Friedrich Horschig <fhorschig@chromium.org> Cr-Commit-Position: refs/heads/master@{#529135} [modify] https://crrev.com/a619d5567e6e32af78321ea591cbfbd9a7ab2bc8/chrome/browser/safe_browsing/download_protection/download_protection_service_unittest.cc [modify] https://crrev.com/a619d5567e6e32af78321ea591cbfbd9a7ab2bc8/content/browser/browser_main_loop.cc [modify] https://crrev.com/a619d5567e6e32af78321ea591cbfbd9a7ab2bc8/content/browser/browser_main_loop.h [modify] https://crrev.com/a619d5567e6e32af78321ea591cbfbd9a7ab2bc8/content/browser/renderer_host/media/peer_connection_tracker_host.cc [modify] https://crrev.com/a619d5567e6e32af78321ea591cbfbd9a7ab2bc8/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/a619d5567e6e32af78321ea591cbfbd9a7ab2bc8/content/browser/webrtc/webrtc_internals.cc [modify] https://crrev.com/a619d5567e6e32af78321ea591cbfbd9a7ab2bc8/content/browser/webrtc/webrtc_internals.h [modify] https://crrev.com/a619d5567e6e32af78321ea591cbfbd9a7ab2bc8/content/browser/webrtc/webrtc_internals_message_handler.cc |
||
►
Sign in to add a comment |
||
Comment 1 by bugdroid1@chromium.org
, Jan 11 2018