Skip to content

GitLab

T
tangled
Commit 6c2dc08b authored by Peter Kasting's avatar Peter Kasting Committed by Commit Bot
Browse files
Options

Revert "Reland "Support dynamic refresh of WebRtcEventLogCollectionAllowed policy"" 

This reverts commit 94a8710d.

Reason for revert: Seems to have caused significant flakiness of many WebRtc tests: https://findit-for-me.appspot.com/waterfall/flake/flake-culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vOTRhODcxMGRjZmViMjFhNDUxNjBjOTY4NmVkZWVjN2RlN2UzMjVjNAw

Original change's description:
> Reland "Support dynamic refresh of WebRtcEventLogCollectionAllowed policy"
> 
> This is a reland of 7dae0f63
> 
> The original CL caused flakiness because, in production code,
> WebRtcEventLogManager is torn down during Chrome shutdown, after
> all threads other the the UI thread have quit. For unit tests, this
> was not the case, and so the tests hit some use-after-destruction.
> This was fixed by making the tests stop the non-UI threads before
> tearing down WebRtcEventLogManager.
> 
> Original change's description:
> > Support dynamic refresh of WebRtcEventLogCollectionAllowed policy
> >
> > Add suport for dynamic refresh of WebRtcEventLogCollectionAllowed:
> > 1. If the policy was disabled before, and becomes enabled,
> >    subsequent calls to StartRemoteLogging() will succeed (barring
> >    unrelated issues), even for peer connections created while the
> >    policy was disabled.
> > 2. If the policy was enabled before, and becomes disabled:
> >    a. Peer connections associated with the profile reject
> >       calls to StartRemoteLogging().
> >    b. Active logs (associated with the profile) are stopped,
> >       and those log files deleted.
> >    c. Pending log files (associated with the profile) are deleted.
> >    d. If a log file associated with the relevant profile is
> >       currently being uploaded, the upload will be cancelled,
> >       and the file deleted.
> >
> > Bug: 775415
> > Change-Id: I70a02ff04eded2926e56fa8e368715ec1ff7c34f
> > Reviewed-on: https://chromium-review.googlesource.com/1162168
> > Reviewed-by: Guido Urdaneta <guidou@chromium.org>
> > Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
> > Reviewed-by: Avi Drissman <avi@chromium.org>
> > Commit-Queue: Elad Alon <eladalon@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#581167}
> 
> TBR=guidou@chromium.org
> 
> Bug: 775415
> Change-Id: Ie6070a8843012580c9a6c069f65c01b8d63525ff
> Reviewed-on: https://chromium-review.googlesource.com/1168962
> Commit-Queue: Elad Alon <eladalon@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#582162}

TBR=avi@chromium.org,pastarmovj@chromium.org,guidou@chromium.org,eladalon@chromium.org

Change-Id: I39bd5b6ac26325522f068e78846dde2443f067ec
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 775415
Reviewed-on: https://chromium-review.googlesource.com/1171263Reviewed-by: default avatarPeter Kasting <pkasting@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582261}
parent 1ac9b351
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 54 additions and 451 deletions
chrome/browser/media/webrtc/webrtc_event_log_manager.cc
View file @ 6c2dc08b
......@@ -130,8 +130,6 @@ WebRtcEventLogManager::~WebRtcEventLogManager() {
g_webrtc_event_log_manager = nullptr;
}
// TODO(crbug.com/775415): If a BrowserContext had the policy as active in
// the past, but no longer does, purge pending log files from before.
void WebRtcEventLogManager::EnableForBrowserContext(
BrowserContext* browser_context,
base::OnceClosure reply) {
......@@ -144,39 +142,33 @@ void WebRtcEventLogManager::EnableForBrowserContext(
first_browser_context_initializations_done_ = true;
}
StartListeningForPrefChangeForBrowserContext(browser_context);
if (!IsRemoteLoggingAllowedForBrowserContext(browser_context)) {
MaybeReply(FROM_HERE, std::move(reply));
return;
}
const bool enable_for_remote_logging =
IsRemoteLoggingAllowedForBrowserContext(browser_context);
// |this| is destroyed by ~BrowserProcessImpl(), so base::Unretained(this)
// will not be dereferenced after destruction.
task_runner_->PostTask(
FROM_HERE,
base::BindOnce(
&WebRtcEventLogManager::EnableRemoteBoundLoggingForBrowserContext,
base::Unretained(this), GetBrowserContextId(browser_context),
browser_context->GetPath(), std::move(reply)));
base::BindOnce(&WebRtcEventLogManager::EnableForBrowserContextInternal,
base::Unretained(this),
GetBrowserContextId(browser_context),
browser_context->GetPath(), enable_for_remote_logging,
std::move(reply)));
}
void WebRtcEventLogManager::DisableForBrowserContext(
content::BrowserContext* browser_context,
const content::BrowserContext* browser_context,
base::OnceClosure reply) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
DCHECK(browser_context);
StopListeningForPrefChangeForBrowserContext(browser_context);
// |this| is destroyed by ~BrowserProcessImpl(), so base::Unretained(this)
// will not be dereferenced after destruction.
task_runner_->PostTask(
FROM_HERE,
base::BindOnce(
&WebRtcEventLogManager::DisableRemoteBoundLoggingForBrowserContext,
base::Unretained(this), GetBrowserContextId(browser_context),
std::move(reply)));
base::BindOnce(&WebRtcEventLogManager::DisableForBrowserContextInternal,
base::Unretained(this),
GetBrowserContextId(browser_context), std::move(reply)));
}
void WebRtcEventLogManager::PeerConnectionAdded(
......@@ -502,14 +494,14 @@ void WebRtcEventLogManager::OnRemoteLogStopped(
void WebRtcEventLogManager::OnLoggingTargetStarted(LoggingTarget target,
PeerConnectionKey key) {
DCHECK(task_runner_->RunsTasksInCurrentSequence());
auto it = peer_connections_with_event_logging_enabled_in_webrtc_.find(key);
if (it != peer_connections_with_event_logging_enabled_in_webrtc_.end()) {
auto it = peer_connections_with_event_logging_enabled_.find(key);
if (it != peer_connections_with_event_logging_enabled_.end()) {
DCHECK_EQ((it->second & target), 0u);
it->second |= target;
} else {
// This is the first client for WebRTC event logging - let WebRTC know
// that it should start informing us of events.
peer_connections_with_event_logging_enabled_in_webrtc_.emplace(key, target);
peer_connections_with_event_logging_enabled_.emplace(key, target);
pc_tracker_proxy_->SetWebRtcEventLoggingState(key, true);
}
}
......@@ -519,92 +511,19 @@ void WebRtcEventLogManager::OnLoggingTargetStopped(LoggingTarget target,
DCHECK(task_runner_->RunsTasksInCurrentSequence());
// Record that we're no longer performing this type of logging for this PC.
auto it = peer_connections_with_event_logging_enabled_in_webrtc_.find(key);
CHECK(it != peer_connections_with_event_logging_enabled_in_webrtc_.end());
auto it = peer_connections_with_event_logging_enabled_.find(key);
CHECK(it != peer_connections_with_event_logging_enabled_.end());
DCHECK_NE(it->second, 0u);
it->second &= ~target;
// If we're not doing any other type of logging for this peer connection,
// it's time to stop receiving notifications for it from WebRTC.
if (it->second == 0u) {
peer_connections_with_event_logging_enabled_in_webrtc_.erase(it);
peer_connections_with_event_logging_enabled_.erase(it);
pc_tracker_proxy_->SetWebRtcEventLoggingState(key, false);
}
}
void WebRtcEventLogManager::StartListeningForPrefChangeForBrowserContext(
BrowserContext* browser_context) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
DCHECK(first_browser_context_initializations_done_);
CHECK(!browser_context->IsOffTheRecord());
const auto browser_context_id = GetBrowserContextId(browser_context);
auto it = pref_change_registrars_.emplace(std::piecewise_construct,
std::make_tuple(browser_context_id),
std::make_tuple());
DCHECK(it.second) << "Already listening.";
PrefChangeRegistrar& registrar = it.first->second;
Profile* profile = Profile::FromBrowserContext(browser_context);
DCHECK(profile);
registrar.Init(profile->GetPrefs());
// * |this| is destroyed by ~BrowserProcessImpl(), so base::Unretained(this)
// will not be dereferenced after destruction.
// * base::Unretained(browser_context) is safe, because |browser_context|
// stays alive until Chrome shut-down, at which point we'll stop listening
// as part of its (BrowserContext's) tear-down process.
registrar.Add(prefs::kWebRtcEventLogCollectionAllowed,
base::BindRepeating(&WebRtcEventLogManager::OnPrefChange,
base::Unretained(this),
base::Unretained(browser_context)));
}
void WebRtcEventLogManager::StopListeningForPrefChangeForBrowserContext(
BrowserContext* browser_context) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
const auto browser_context_id = GetBrowserContextId(browser_context);
size_t erased_count = pref_change_registrars_.erase(browser_context_id);
DCHECK_EQ(erased_count, 1u);
}
void WebRtcEventLogManager::OnPrefChange(BrowserContext* browser_context) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
DCHECK(first_browser_context_initializations_done_);
const Profile* profile = Profile::FromBrowserContext(browser_context);
DCHECK(profile);
const bool enabled =
profile->GetPrefs()->GetBoolean(prefs::kWebRtcEventLogCollectionAllowed);
if (!enabled) {
// Dynamic refresh of the policy to DISABLED; stop ongoing logs, remove
// pending log files and stop any active uploads.
ClearCacheForBrowserContext(browser_context, base::Time::Min(),
base::Time::Max(), base::DoNothing());
}
// |this| is destroyed by ~BrowserProcessImpl(), so base::Unretained(this)
// will not be dereferenced after destruction.
base::OnceClosure task;
if (enabled) {
task = base::BindOnce(
&WebRtcEventLogManager::EnableRemoteBoundLoggingForBrowserContext,
base::Unretained(this), GetBrowserContextId(browser_context),
browser_context->GetPath(), base::OnceClosure());
} else {
task = base::BindOnce(
&WebRtcEventLogManager::DisableRemoteBoundLoggingForBrowserContext,
base::Unretained(this), GetBrowserContextId(browser_context),
base::OnceClosure());
}
task_runner_->PostTask(FROM_HERE, std::move(task));
}
void WebRtcEventLogManager::OnFirstBrowserContextLoaded() {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
......@@ -649,26 +568,27 @@ void WebRtcEventLogManager::OnFirstBrowserContextLoadedInternal(
std::move(log_file_writer_factory));
}
void WebRtcEventLogManager::EnableRemoteBoundLoggingForBrowserContext(
void WebRtcEventLogManager::EnableForBrowserContextInternal(
BrowserContextId browser_context_id,
const base::FilePath& browser_context_dir,
bool enable_for_remote_logging,
base::OnceClosure reply) {
DCHECK(task_runner_->RunsTasksInCurrentSequence());
DCHECK_NE(browser_context_id, kNullBrowserContextId);
remote_logs_manager_.EnableForBrowserContext(browser_context_id,
browser_context_dir);
if (enable_for_remote_logging) {
remote_logs_manager_.EnableForBrowserContext(browser_context_id,
browser_context_dir);
}
MaybeReply(FROM_HERE, std::move(reply));
}
void WebRtcEventLogManager::DisableRemoteBoundLoggingForBrowserContext(
void WebRtcEventLogManager::DisableForBrowserContextInternal(
BrowserContextId browser_context_id,
base::OnceClosure reply) {
DCHECK(task_runner_->RunsTasksInCurrentSequence());
// Note that the BrowserContext might never have been enabled in the
// remote-bound manager; that's not a problem.
remote_logs_manager_.DisableForBrowserContext(browser_context_id);
MaybeReply(FROM_HERE, std::move(reply));
......
chrome/browser/media/webrtc/webrtc_event_log_manager.h
View file @ 6c2dc08b
......@@ -19,7 +19,6 @@
#include "chrome/browser/media/webrtc/webrtc_event_log_manager_common.h"
#include "chrome/browser/media/webrtc/webrtc_event_log_manager_local.h"
#include "chrome/browser/media/webrtc/webrtc_event_log_manager_remote.h"
#include "components/prefs/pref_change_registrar.h"
#include "content/public/browser/render_process_host_observer.h"
#include "content/public/browser/webrtc_event_logger.h"
......@@ -74,7 +73,7 @@ class WebRtcEventLogManager final : public content::RenderProcessHostObserver,
void EnableForBrowserContext(content::BrowserContext* browser_context,
base::OnceClosure reply) override;
void DisableForBrowserContext(content::BrowserContext* browser_context,
void DisableForBrowserContext(const content::BrowserContext* browser_context,
base::OnceClosure reply) override;
void PeerConnectionAdded(int render_process_id,
......@@ -199,13 +198,6 @@ class WebRtcEventLogManager final : public content::RenderProcessHostObserver,
void OnLoggingTargetStarted(LoggingTarget target, PeerConnectionKey key);
void OnLoggingTargetStopped(LoggingTarget target, PeerConnectionKey key);
void StartListeningForPrefChangeForBrowserContext(
content::BrowserContext* browser_context);
void StopListeningForPrefChangeForBrowserContext(
content::BrowserContext* browser_context);
void OnPrefChange(content::BrowserContext* browser_context);
// network_connection_tracker() and system_request_context() are not available
// during instantiation; we get them when the first profile is loaded, which
// is also the earliest time when they could be needed.
......@@ -219,14 +211,17 @@ class WebRtcEventLogManager final : public content::RenderProcessHostObserver,
net::URLRequestContextGetter* url_request_context_getter,
std::unique_ptr<LogFileWriter::Factory> log_file_writer_factory);
void EnableRemoteBoundLoggingForBrowserContext(
// The BrowserContext is always enabled for local-bound logs.
// |enable_for_remote_logging| indicates whether it should also be enabled
// for remote-bound logs. This depends on a Chrome policy.
void EnableForBrowserContextInternal(
BrowserContextId browser_context_id,
const base::FilePath& browser_context_dir,
bool enable_for_remote_logging,
base::OnceClosure reply);
void DisableRemoteBoundLoggingForBrowserContext(
BrowserContextId browser_context_id,
base::OnceClosure reply);
void DisableForBrowserContextInternal(BrowserContextId browser_context_id,
base::OnceClosure reply);
void PeerConnectionAddedInternal(PeerConnectionKey key,
const std::string& peer_connection_id,
......@@ -333,15 +328,10 @@ class WebRtcEventLogManager final : public content::RenderProcessHostObserver,
// This is only possible when the appropriate Chrome policy is configured.
WebRtcRemoteEventLogManager remote_logs_manager_;
// Each loaded BrowserContext is mapped to a PrefChangeRegistrar, which keeps
// us informed about preference changes, thereby allowing as to support
// dynamic refresh.
std::map<BrowserContextId, PrefChangeRegistrar> pref_change_registrars_;
// This keeps track of which peer connections have event logging turned on
// in WebRTC, and for which client(s).
std::map<PeerConnectionKey, LoggingTargetBitmap>
peer_connections_with_event_logging_enabled_in_webrtc_;
peer_connections_with_event_logging_enabled_;
// The set of RenderProcessHosts with which the manager is registered for
// observation. Allows us to register for each RPH only once, and get notified
......
chrome/browser/media/webrtc/webrtc_event_log_manager_remote.cc
View file @ 6c2dc08b
......@@ -17,6 +17,7 @@
#include "base/logging.h"
#include "base/strings/string_number_conversions.h"
#include "base/strings/string_util.h"
#include "base/threading/sequenced_task_runner_handle.h"
#include "base/unguessable_token.h"
#include "chrome/common/chrome_switches.h"
#include "content/public/browser/browser_thread.h"
......@@ -216,10 +217,10 @@ void WebRtcRemoteEventLogManager::EnableForBrowserContext(
return;
}
enabled_browser_contexts_.insert(browser_context_id);
AddPendingLogs(browser_context_id, remote_bound_logs_dir);
enabled_browser_contexts_.insert(browser_context_id);
if (!proactive_prune_scheduling_delta_.is_zero() &&
!proactive_prune_scheduling_started_) {
proactive_prune_scheduling_started_ = true;
......@@ -227,6 +228,7 @@ void WebRtcRemoteEventLogManager::EnableForBrowserContext(
}
}
// TODO(crbug.com/775415): Add unit tests.
void WebRtcRemoteEventLogManager::DisableForBrowserContext(
BrowserContextId browser_context_id) {
DCHECK(task_runner_->RunsTasksInCurrentSequence());
......@@ -238,14 +240,8 @@ void WebRtcRemoteEventLogManager::DisableForBrowserContext(
enabled_browser_contexts_.erase(browser_context_id);
#if DCHECK_IS_ON()
// DisableForBrowserContext() is called in one of two cases:
// 1. If Chrome is shutting down. In that case, all the RPHs associated with
// this BrowserContext must already have exited, which should have
// implicitly stopped all active logs.
// 2. Remote-bound logging is no longer allowed for this BrowserContext.
// In that case, some peer connections associated with this BrowserContext
// might still be active, or become active at a later time, but all
// logs must have already been stopped.
// All of the RPHs associated with this BrowserContext must already have
// exited, which should have implicitly stopped all active logs.
auto pred = [browser_context_id](decltype(active_logs_)::value_type& log) {
return log.first.browser_context_id == browser_context_id;
};
......@@ -253,6 +249,7 @@ void WebRtcRemoteEventLogManager::DisableForBrowserContext(
#endif
// Pending logs for this BrowserContext are no longer eligible for upload.
// (Active uploads, if any, are not affected.)
for (auto it = pending_logs_.begin(); it != pending_logs_.end();) {
if (it->browser_context_id == browser_context_id) {
it = pending_logs_.erase(it);
......@@ -261,9 +258,6 @@ void WebRtcRemoteEventLogManager::DisableForBrowserContext(
}
}
// Active uploads of logs associated with this BrowserContext must be stopped.
MaybeCancelUpload(base::Time::Min(), base::Time::Max(), browser_context_id);
// Active logs may have been removed, which could remove upload suppression,
// or pending logs which were about to be uploaded may have been removed,
// so uploading may no longer be possible.
......@@ -423,7 +417,6 @@ void WebRtcRemoteEventLogManager::RenderProcessHostExitedDestroyed(
void WebRtcRemoteEventLogManager::OnConnectionChanged(
network::mojom::ConnectionType type) {
DCHECK(task_runner_->RunsTasksInCurrentSequence());
// Even if switching from WiFi to Ethernet, or between to WiFi connections,
// reset the timer (if running) until an upload is permissible due to stable
// upload-supporting conditions.
......@@ -480,7 +473,6 @@ bool WebRtcRemoteEventLogManager::AreLogParametersValid(
bool WebRtcRemoteEventLogManager::BrowserContextEnabled(
BrowserContextId browser_context_id) const {
DCHECK(task_runner_->RunsTasksInCurrentSequence());
const auto it = enabled_browser_contexts_.find(browser_context_id);
return it != enabled_browser_contexts_.cend();
}
......@@ -637,7 +629,7 @@ void WebRtcRemoteEventLogManager::RecurringPendingLogsPrune() {
// |this| is only destroyed (on the UI thread) after |task_runner_| stops,
// so both base::Unretained(this) is safe.
task_runner_->PostDelayedTask(
base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
FROM_HERE,
base::BindOnce(&WebRtcRemoteEventLogManager::RecurringPendingLogsPrune,
base::Unretained(this)),
......@@ -674,7 +666,6 @@ void WebRtcRemoteEventLogManager::MaybeCancelActiveLogs(
const base::Time& delete_begin,
const base::Time& delete_end,
BrowserContextId browser_context_id) {
DCHECK(task_runner_->RunsTasksInCurrentSequence());
for (auto it = active_logs_.begin(); it != active_logs_.end();) {
// Since the file is active, assume it's still being modified.
if (LogFileMatchesFilter(it->first.browser_context_id, base::Time::Now(),
......@@ -690,7 +681,6 @@ void WebRtcRemoteEventLogManager::MaybeCancelUpload(
const base::Time& delete_begin,
const base::Time& delete_end,
base::Optional<BrowserContextId> browser_context_id) {
DCHECK(task_runner_->RunsTasksInCurrentSequence());
if (uploader_) {
const WebRtcLogFileInfo& info = uploader_->GetWebRtcLogFileInfo();
if (LogFileMatchesFilter(info.browser_context_id, info.last_modified,
......@@ -714,7 +704,6 @@ bool WebRtcRemoteEventLogManager::LogFileMatchesFilter(
base::Optional<BrowserContextId> filter_browser_context_id,
const base::Time& filter_range_begin,
const base::Time& filter_range_end) const {
DCHECK(task_runner_->RunsTasksInCurrentSequence());
if (filter_browser_context_id &&
*filter_browser_context_id != log_browser_context_id) {
return false;
......@@ -753,7 +742,6 @@ bool WebRtcRemoteEventLogManager::UploadSuppressed() const {
}
bool WebRtcRemoteEventLogManager::UploadConditionsHold() const {
DCHECK(task_runner_->RunsTasksInCurrentSequence());
return !uploader_ && !pending_logs_.empty() && !UploadSuppressed() &&
uploading_supported_for_connection_type_;
}
......
chrome/browser/media/webrtc/webrtc_event_log_manager_remote.h
View file @ 6c2dc08b
......@@ -57,18 +57,12 @@ class WebRtcRemoteEventLogManager final
// peer connections associated with this BrowserContext, in the
// BrowserContext's user-data directory, becomes possible.
// This method would typically be called when a BrowserContext is initialized.
// Enabling for the same BrowserContext twice in a row, without disabling
// in between, is an error.
void EnableForBrowserContext(BrowserContextId browser_context_id,
const base::FilePath& browser_context_dir);
// Disables remote-bound logging for a given BrowserContext. Pending logs from
// earlier (while it was enabled) may no longer be uploaded, additional
// logs will not be created, and any active uploads associated with the
// BrowserContext will be cancelled.
// Disabling for a BrowserContext which was not enabled is not an error,
// because the caller is not required to know whether a previous call
// to EnableForBrowserContext() was successful.
// Enables remote-bound logging for a given BrowserContext. Pending logs from
// earlier (while it was enabled) may still be uploaded, but no additional
// logs will be created.
void DisableForBrowserContext(BrowserContextId browser_context_id);
// Called to inform |this| of peer connections being added/removed.
......
components/policy/resources/policy_templates.json
View file @ 6c2dc08b
......@@ -12677,10 +12677,11 @@
'name': 'WebRtcEventLogCollectionAllowed',
'type': 'main',
'schema': { 'type': 'boolean' },
# TODO(crbug.com/775415): Support for ChromeOS as well.
'supported_on': ['chrome.*:70-'],
'features': {
'dynamic_refresh': True,
# TODO(crbug.com/775415): Support dynamic refresh, then add support
# for ChromeOS as well.
'dynamic_refresh': False,
'per_profile': True,
},
'example_value': True,
......
content/public/browser/webrtc_event_logger.h
View file @ 6c2dc08b
......@@ -54,7 +54,7 @@ class CONTENT_EXPORT WebRtcEventLogger {
// therefore be careful note to call any of BrowserContext's virtual methods.
// TODO(eladalon): After changing to a Profile-centered interface, change this
// to not even receive a pointer. https://crbug.com/775415
virtual void DisableForBrowserContext(BrowserContext* browser_context,
virtual void DisableForBrowserContext(const BrowserContext* browser_context,
base::OnceClosure reply) = 0;
// Call this to let the logger know when a PeerConnection was created.
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment