Skip to content

Commit

Permalink
Revert "Experiment: Add KeepAlive source histogram to crash keys."
Browse files Browse the repository at this point in the history
This reverts commit 2629484.

Reason for revert: This shouldn't live past the branch, and we have the data we need.

Original change's description:
> Experiment: Add KeepAlive source histogram to crash keys.
>
> This re-lands the experiment first landed in
> https://chromium-review.googlesource.com/c/chromium/src/+/2705030 in
> an effort to discover the source of new no-BrowsingInstanceID
> failures.
>
> This CL will be reverted once the data has been logged.
>
> Bug: 1148542
> Change-Id: Ie387c8678fea9d13326bcc7360b9b8898523fda4
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2799974
> Commit-Queue: James MacLean <wjmaclean@chromium.org>
> Reviewed-by: Charlie Reis <creis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#868659}

(cherry picked from commit eb882a2)

Bug: 1148542
Change-Id: Id5d9b3a3623373bcbaf8105e1b1b179df42234e0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2815263
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: James MacLean <wjmaclean@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: James MacLean <wjmaclean@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#870794}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2826395
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/branch-heads/4472@{#66}
Cr-Branched-From: 3d60439-refs/heads/master@{#870763}
  • Loading branch information
James MacLean authored and Chromium LUCI CQ committed Apr 14, 2021
1 parent 8ef4e23 commit 93dd467
Show file tree
Hide file tree
Showing 10 changed files with 27 additions and 62 deletions.
15 changes: 3 additions & 12 deletions content/browser/child_process_security_policy_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ base::debug::CrashKeyString* GetKilledProcessOriginLockKey() {

base::debug::CrashKeyString* GetCanAccessDataFailureReasonKey() {
static auto* crash_key = base::debug::AllocateCrashKeyString(
"can_access_data_failure_reason", base::debug::CrashKeySize::Size256);
"can_access_data_failure_reason", base::debug::CrashKeySize::Size64);
return crash_key;
}

Expand Down Expand Up @@ -1654,18 +1654,9 @@ bool ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin(
// do not agree, the check might be slightly weaker (as the least common
// denominator), but the differences must never violate the ProcessLock.
if (security_state->browsing_instance_ids().empty()) {
// TODO(wjmaclean): Remove the keepalive source tracking after
// diagnosing the cause of the remaining reports in
// https://crbug.com/1148542.
RenderProcessHostImpl* child_host = static_cast<RenderProcessHostImpl*>(
RenderProcessHost::FromID(child_id));
DCHECK(child_host);
failure_reason =
base::StringPrintf("No BIids, keep_alive_count = %zu, sources = %s",
child_host->keep_alive_ref_count(),
child_host->keep_alive_sources().c_str());
failure_reason = "No BrowsingInstanceIDs.";
// This will fall through to the call to
// LogCanAccessDataForOriginCrashKeys and then return false.
// LogCanAccessDataForOriginCrashKeys below, then return false.
}
for (auto browsing_instance_id :
security_state->browsing_instance_ids()) {
Expand Down
6 changes: 2 additions & 4 deletions content/browser/renderer_host/keep_alive_handle_factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,15 @@ class KeepAliveHandleImpl final : public blink::mojom::KeepAliveHandle {
if (!process_host || process_host->IsKeepAliveRefCountDisabled()) {
return;
}
process_host->IncrementKeepAliveRefCount(
RenderProcessHost::KeepAliveSource::KEEP_ALIVE_HANDLE_FACTORY);
process_host->IncrementKeepAliveRefCount();
}
~KeepAliveHandleImpl() override {
GetContentClient()->browser()->OnKeepaliveRequestFinished();
RenderProcessHost* process_host = RenderProcessHost::FromID(process_id_);
if (!process_host || process_host->IsKeepAliveRefCountDisabled()) {
return;
}
process_host->DecrementKeepAliveRefCount(
RenderProcessHost::KeepAliveSource::KEEP_ALIVE_HANDLE_FACTORY);
process_host->DecrementKeepAliveRefCount();
}

KeepAliveHandleImpl(const KeepAliveHandleImpl&) = delete;
Expand Down
10 changes: 4 additions & 6 deletions content/browser/renderer_host/render_process_host_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2149,7 +2149,7 @@ void RenderProcessHostImpl::CreateWebSocketConnector(
void RenderProcessHostImpl::CancelProcessShutdownDelayForUnload() {
if (IsKeepAliveRefCountDisabled())
return;
DecrementKeepAliveRefCount(KeepAliveSource::KEEP_ALIVE_SUBFRAME_UNLOAD);
DecrementKeepAliveRefCount();
}

void RenderProcessHostImpl::DelayProcessShutdownForUnload(
Expand All @@ -2158,7 +2158,7 @@ void RenderProcessHostImpl::DelayProcessShutdownForUnload(
if (IsKeepAliveRefCountDisabled() || deleting_soon_ || fast_shutdown_started_)
return;

IncrementKeepAliveRefCount(KeepAliveSource::KEEP_ALIVE_SUBFRAME_UNLOAD);
IncrementKeepAliveRefCount();
GetUIThreadTaskRunner({})->PostDelayedTask(
FROM_HERE,
base::BindOnce(
Expand Down Expand Up @@ -2665,21 +2665,19 @@ bool RenderProcessHostImpl::IsProcessBackgrounded() {
return priority_.is_background();
}

void RenderProcessHostImpl::IncrementKeepAliveRefCount(KeepAliveSource source) {
void RenderProcessHostImpl::IncrementKeepAliveRefCount() {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
DCHECK(!is_keep_alive_ref_count_disabled_);
++keep_alive_ref_count_;
keep_alive_sources_ += base::StringPrintf("%d+", static_cast<int>(source));
if (keep_alive_ref_count_ == 1)
GetRendererInterface()->SetSchedulerKeepActive(true);
}

void RenderProcessHostImpl::DecrementKeepAliveRefCount(KeepAliveSource source) {
void RenderProcessHostImpl::DecrementKeepAliveRefCount() {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
DCHECK(!is_keep_alive_ref_count_disabled_);
DCHECK_GT(keep_alive_ref_count_, 0U);
--keep_alive_ref_count_;
keep_alive_sources_ += base::StringPrintf("%d-", static_cast<int>(source));
if (keep_alive_ref_count_ == 0) {
Cleanup();
GetRendererInterface()->SetSchedulerKeepActive(false);
Expand Down
7 changes: 2 additions & 5 deletions content/browser/renderer_host/render_process_host_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -241,8 +241,8 @@ class CONTENT_EXPORT RenderProcessHostImpl
override;
const base::TimeTicks& GetInitTimeForNavigationMetrics() override;
bool IsProcessBackgrounded() override;
void IncrementKeepAliveRefCount(KeepAliveSource source) override;
void DecrementKeepAliveRefCount(KeepAliveSource source) override;
void IncrementKeepAliveRefCount() override;
void DecrementKeepAliveRefCount() override;
void DisableKeepAliveRefCount() override;
bool IsKeepAliveRefCountDisabled() override;
mojom::Renderer* GetRendererInterface() override;
Expand Down Expand Up @@ -666,8 +666,6 @@ class CONTENT_EXPORT RenderProcessHostImpl
#endif

size_t keep_alive_ref_count() const { return keep_alive_ref_count_; }
// TODO(wjmaclean): remove this when the experiment is done.
std::string keep_alive_sources() const { return keep_alive_sources_; }

// Allows overriding the URLLoaderFactory creation via CreateURLLoaderFactory.
// Passing a null callback will restore the default behavior.
Expand Down Expand Up @@ -934,7 +932,6 @@ class CONTENT_EXPORT RenderProcessHostImpl
mojo::OutgoingInvitation mojo_invitation_;

size_t keep_alive_ref_count_;
std::string keep_alive_sources_;

// Set in DisableKeepAliveRefCount(). When true, |keep_alive_ref_count_| must
// no longer be modified.
Expand Down
15 changes: 5 additions & 10 deletions content/browser/service_worker/service_worker_process_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,8 @@ void ServiceWorkerProcessManager::Shutdown() {
for (const auto& it : worker_process_map_) {
if (it.second->HasProcess()) {
RenderProcessHost* process = it.second->GetProcess();
if (!process->IsKeepAliveRefCountDisabled()) {
process->DecrementKeepAliveRefCount(
RenderProcessHost::KeepAliveSource::KEEP_ALIVE_SERVICE_WORKER);
}
if (!process->IsKeepAliveRefCountDisabled())
process->DecrementKeepAliveRefCount();
}
}
}
Expand Down Expand Up @@ -163,8 +161,7 @@ ServiceWorkerProcessManager::AllocateWorkerProcess(

worker_process_map_.emplace(embedded_worker_id, std::move(site_instance));
if (!rph->IsKeepAliveRefCountDisabled())
rph->IncrementKeepAliveRefCount(
RenderProcessHost::KeepAliveSource::KEEP_ALIVE_SERVICE_WORKER);
rph->IncrementKeepAliveRefCount();
out_info->process_id = rph->GetID();
out_info->start_situation = start_situation;
return blink::ServiceWorkerStatusCode::kOk;
Expand Down Expand Up @@ -193,10 +190,8 @@ void ServiceWorkerProcessManager::ReleaseWorkerProcess(int embedded_worker_id) {

if (it->second->HasProcess()) {
RenderProcessHost* process = it->second->GetProcess();
if (!process->IsKeepAliveRefCountDisabled()) {
process->DecrementKeepAliveRefCount(
RenderProcessHost::KeepAliveSource::KEEP_ALIVE_SERVICE_WORKER);
}
if (!process->IsKeepAliveRefCountDisabled())
process->DecrementKeepAliveRefCount();
}
worker_process_map_.erase(it);
}
Expand Down
6 changes: 2 additions & 4 deletions content/browser/site_per_process_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7676,8 +7676,7 @@ IN_PROC_BROWSER_TEST_P(SitePerProcessBrowserTest,
// termination.
RenderProcessHost* subframe_process =
root->child_at(0)->current_frame_host()->GetProcess();
subframe_process->IncrementKeepAliveRefCount(
RenderProcessHost::KeepAliveSource::KEEP_ALIVE_SUBFRAME_UNLOAD);
subframe_process->IncrementKeepAliveRefCount();

// Navigate the subframe away from b.com. Since this is the last active
// frame in the b.com process, this causes the RenderWidget and RenderView to
Expand All @@ -7689,8 +7688,7 @@ IN_PROC_BROWSER_TEST_P(SitePerProcessBrowserTest,
// Release the process.
RenderProcessHostWatcher process_shutdown_observer(
subframe_process, RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
subframe_process->DecrementKeepAliveRefCount(
RenderProcessHost::KeepAliveSource::KEEP_ALIVE_SUBFRAME_UNLOAD);
subframe_process->DecrementKeepAliveRefCount();
process_shutdown_observer.Wait();
}

Expand Down
9 changes: 3 additions & 6 deletions content/browser/worker_host/shared_worker_host.cc
Original file line number Diff line number Diff line change
Expand Up @@ -90,15 +90,12 @@ class SharedWorkerHost::ScopedProcessHostRef {
public:
explicit ScopedProcessHostRef(RenderProcessHost* render_process_host)
: render_process_host_(render_process_host) {
render_process_host_->IncrementKeepAliveRefCount(
RenderProcessHost::KeepAliveSource::KEEP_ALIVE_SHARED_WORKER);
render_process_host_->IncrementKeepAliveRefCount();
}

~ScopedProcessHostRef() {
if (!render_process_host_->IsKeepAliveRefCountDisabled()) {
render_process_host_->DecrementKeepAliveRefCount(
RenderProcessHost::KeepAliveSource::KEEP_ALIVE_SHARED_WORKER);
}
if (!render_process_host_->IsKeepAliveRefCountDisabled())
render_process_host_->DecrementKeepAliveRefCount();
}

ScopedProcessHostRef(const ScopedProcessHostRef& other) = delete;
Expand Down
13 changes: 2 additions & 11 deletions content/public/browser/render_process_host.h
Original file line number Diff line number Diff line change
Expand Up @@ -122,15 +122,6 @@ class CONTENT_EXPORT RenderProcessHost : public IPC::Sender,
GENERATE_CRASH_DUMP,
};

// Temporary enum to track source of KeepAlive increments.
enum class KeepAliveSource {
KEEP_ALIVE_NONE,
KEEP_ALIVE_HANDLE_FACTORY,
KEEP_ALIVE_SUBFRAME_UNLOAD,
KEEP_ALIVE_SERVICE_WORKER,
KEEP_ALIVE_SHARED_WORKER
};

// General functions ---------------------------------------------------------

~RenderProcessHost() override {}
Expand Down Expand Up @@ -404,8 +395,8 @@ class CONTENT_EXPORT RenderProcessHost : public IPC::Sender,
// Keeps the process alive briefly to give subframe unload handlers a
// chance to execute after their parent frame navigates or is detached.
// See https://crbug.com/852204.
virtual void IncrementKeepAliveRefCount(KeepAliveSource source) = 0;
virtual void DecrementKeepAliveRefCount(KeepAliveSource source) = 0;
virtual void IncrementKeepAliveRefCount() = 0;
virtual void DecrementKeepAliveRefCount() = 0;

// Sets keep alive ref counts to zero. Called when the browser context will be
// destroyed so this RenderProcessHost can immediately die.
Expand Down
4 changes: 2 additions & 2 deletions content/public/test/mock_render_process_host.cc
Original file line number Diff line number Diff line change
Expand Up @@ -362,11 +362,11 @@ size_t MockRenderProcessHost::GetKeepAliveRefCount() const {
return keep_alive_ref_count_;
}

void MockRenderProcessHost::IncrementKeepAliveRefCount(KeepAliveSource source) {
void MockRenderProcessHost::IncrementKeepAliveRefCount() {
++keep_alive_ref_count_;
}

void MockRenderProcessHost::DecrementKeepAliveRefCount(KeepAliveSource source) {
void MockRenderProcessHost::DecrementKeepAliveRefCount() {
--keep_alive_ref_count_;
}

Expand Down
4 changes: 2 additions & 2 deletions content/public/test/mock_render_process_host.h
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,8 @@ class MockRenderProcessHost : public RenderProcessHost {
const base::TimeTicks& GetInitTimeForNavigationMetrics() override;
bool IsProcessBackgrounded() override;
size_t GetKeepAliveRefCount() const;
void IncrementKeepAliveRefCount(KeepAliveSource source) override;
void DecrementKeepAliveRefCount(KeepAliveSource source) override;
void IncrementKeepAliveRefCount() override;
void DecrementKeepAliveRefCount() override;
void DisableKeepAliveRefCount() override;
bool IsKeepAliveRefCountDisabled() override;
mojom::Renderer* GetRendererInterface() override;
Expand Down

0 comments on commit 93dd467

Please sign in to comment.