LockToOriginIfNeeded() might set some crash keys even without a crash |
||||||
Issue description
When not locking a process to an origin, SiteInstanceImpl::LockToOriginIfNeeded() has the following sanity check:
base::debug::SetCrashKeyString(bad_message::GetRequestedSiteURLKey(),
site_.spec());
base::debug::SetCrashKeyString(
bad_message::GetKilledProcessOriginLockKey(),
policy->GetOriginLock(process_->GetID()).spec());
CHECK_EQ(lock_state,
ChildProcessSecurityPolicyImpl::CheckOriginLockResult::NO_LOCK)
<< "Trying to commit non-isolated site " << site_
<< " in process locked to " << policy->GetOriginLock(process_->GetID());
This might set the request_site_url and killed_process_origin_lock unnecessarily even if the CHECK_EQ doesn't cause a crash. Instead, we should check the lock_state first, and only set these crash keys if we are actually going to crash.
,
Apr 30 2018
Should be fixed by r554607. Requesting merge to M67 - this is a safe change that should make site isolation crash reports more useful on M67. I also verified on canary for a few "renderer kill 4" crashes - these used to have requested_site_url set to an NTP URL that probably had nothing to do with the kill, but that crash key isn't present in the latest reports from 68.0.3415.0.
,
Apr 30 2018
This bug requires manual review: M67 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 30 2018
Just looked at r554607. Agreed that it's a very safe change that would avoid having a lot of false leads in M67 crash investigations (due to crash keys being logged in too many cases). Merging it should help us diagnose M67 crash reports more quickly, so I think it's worth it. Thanks! (Also marking fixed, since the issue should be resolved on trunk.)
,
Apr 30 2018
Approving merge for CL r554607 to M67 branch 3396 based on commenta #2 and #4. Pls merge ASAP so we can pick it up for this week M67 beta release. Thank you.
,
Apr 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ad1172a55f6d98ad21e3ac201089a5a62f8d76b5 commit ad1172a55f6d98ad21e3ac201089a5a62f8d76b5 Author: Alex Moshchuk <alexmos@chromium.org> Date: Mon Apr 30 20:48:04 2018 Don't set crash keys if there's no crash in LockToOriginIfNeeded(). (Merge to M67) TBR=alexmos@chromium.org (cherry picked from commit 1656c6780e6e347520391b1a1c6bfcc68c4b0cad) Bug: 837009 Change-Id: Ia239d85b546c2265e77564356d2c1a2e2fc3aa74 Reviewed-on: https://chromium-review.googlesource.com/1029006 Reviewed-by: Nasko Oskov <nasko@chromium.org> Commit-Queue: Alex Moshchuk <alexmos@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#554607} Reviewed-on: https://chromium-review.googlesource.com/1036111 Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/branch-heads/3396@{#397} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/ad1172a55f6d98ad21e3ac201089a5a62f8d76b5/content/browser/site_instance_impl.cc |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by bugdroid1@chromium.org
, Apr 28 2018