Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.

This wasn't filed earlier since a few test cases got grouped together incorrectly.

joedow: Would you mind taking a look at this when you have a chance? It seems a bit flaky, but we've been seeing this crash off and on in ClusterFuzz for a while.

This is a critical security issue. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

It looks like this was found on July 20th but only repros sporadically.  I've tried reproducing this on my local workstation w/o any success.

I will make a few changes to mitigate the issue and possibly give me a more meaningful stack trace.

Also, I'm not sure the Pri-0 is correct given that this is so difficult to reproduce.

joedow@, sorry for the inconvenience with the reproducibility :( 

Pri-0 is a usual for Critical bugs like this one. Even though we can't reliably reproduce it, that doesn't mean that no one else (e.g. a potential attacker) doesn't have a better testcase that works more reliably.

Once you land a speculative fix, we should check STATISTICS section on CF testcase page: https://clusterfuzz.com/v2/testcase-detail/5787467773640704?noredirect=1

That would give us some signal whether the issue is fixed or not. I'm changing this back to Pri-0 as per the guidelines: https://chromium.googlesource.com/chromium/src/+/master/docs/security/severity-guidelines.md#Critical-severity

Sorry again for the reproducibility problem.

Sorry, Critical severity bugs are Pri-0.

The problem here is that KeyboardLockServiceImpl takes a raw pointer to a RenderFrameHost but doesn't observe the destruction of the RenderFrameHost. Nothing guarantees that KeyboardLockServiceImpl won't outlive the RenderFrameHost.

One fix is to just make the service impl a WebContentsObserver and watch for RenderFrameDeleted to match the stored RenderFrameHost. We could also use content::FrameServiceBase: this was originally written to scope the lifetime of media services to one logical 'document', and it seems like it'd probably work here as well.

(+oksamyt, +rockot, this is the sort of thing I mean about setting up robust patterns for making frame-associated services.)

Thanks for the suggestion Daniel.  I have a CL which uses FrameServiceBase, unfortunately I can't repro locally so I'm going to have to check it in and see if this stops reproducing.

If so, I can request a merge for M70 and see if it is approved. 

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

commit 9d46e6e6ca8b9021b2d9f60bc3f3261b8718c616
Author: Joe Downing <joedow@chromium.org>
Date: Thu Sep 27 00:24:13 2018

Destroy KeyboardLockServiceImpl instance when RenderFrameHost goes away

This CL updates KeyboardLockServiceImpl to release its mojo binding if
the RenderFrameHost instance it is linked to is destroyed.

Bug:  888678 
Change-Id: Icea5fe1a5c76df4d71fa4e78c423e49828664637
Reviewed-on: https://chromium-review.googlesource.com/1246290
Commit-Queue: Joe Downing <joedow@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594534}
[modify] https://crrev.com/9d46e6e6ca8b9021b2d9f60bc3f3261b8718c616/content/browser/keyboard_lock/keyboard_lock_service_impl.cc
[modify] https://crrev.com/9d46e6e6ca8b9021b2d9f60bc3f3261b8718c616/content/browser/keyboard_lock/keyboard_lock_service_impl.h

Based on the clusterfuzz tool, there hasn't been a reproduction of this issue in the last ~24 hours.  We need to wait a few more days to make sure (as there were a few days in the last week which did not have a repro) but it seems reasonable to start the merge request process.

Hi, I'd like to request a merge for this issue for M70.  It is a security issue with a relatively simple fix which affects the browser process.

One caveat is that I've been unable to reproduce this problem locally so I am watching the clusterfuzz runs to see if we get another hit.  If the runs remain clean for another day or so then I think it is safe to say the issue was addressed.  I'm requesting a merge at this point to get the process started since I think it is likely that the problem has been fixed.

This bug requires manual review: M70 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

abdulsyed@ - good for 70

branch:3538

Thanks!  We've been clear for two days now so I will recheck on Monday AM and merge the CL assuming the runs continue to look good.

The NextAction date has arrived: 2018-10-01

No repros for the last several days so I am going to merge the fix into M70.

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

Commit: 548aa5e2cedde2d31d236e4a9fd6db32ee61eacb
Author: joedow@chromium.org
Commiter: joedow@chromium.org
Date: 2018-10-01 15:27:56 +0000 UTC

Destroy KeyboardLockServiceImpl instance when RenderFrameHost goes away

This CL updates KeyboardLockServiceImpl to release its mojo binding if
the RenderFrameHost instance it is linked to is destroyed.

Bug:  888678 
Change-Id: Icea5fe1a5c76df4d71fa4e78c423e49828664637
Reviewed-on: https://chromium-review.googlesource.com/1246290
Commit-Queue: Joe Downing <joedow@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#594534}(cherry picked from commit 9d46e6e6ca8b9021b2d9f60bc3f3261b8718c616)
Reviewed-on: https://chromium-review.googlesource.com/1254503
Reviewed-by: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#769}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}

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

commit 548aa5e2cedde2d31d236e4a9fd6db32ee61eacb
Author: Joe Downing <joedow@chromium.org>
Date: Mon Oct 01 15:27:56 2018

Destroy KeyboardLockServiceImpl instance when RenderFrameHost goes away

This CL updates KeyboardLockServiceImpl to release its mojo binding if
the RenderFrameHost instance it is linked to is destroyed.

Bug:  888678 
Change-Id: Icea5fe1a5c76df4d71fa4e78c423e49828664637
Reviewed-on: https://chromium-review.googlesource.com/1246290
Commit-Queue: Joe Downing <joedow@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#594534}(cherry picked from commit 9d46e6e6ca8b9021b2d9f60bc3f3261b8718c616)
Reviewed-on: https://chromium-review.googlesource.com/1254503
Reviewed-by: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#769}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/548aa5e2cedde2d31d236e4a9fd6db32ee61eacb/content/browser/keyboard_lock/keyboard_lock_service_impl.cc
[modify] https://crrev.com/548aa5e2cedde2d31d236e4a9fd6db32ee61eacb/content/browser/keyboard_lock/keyboard_lock_service_impl.h

Change merged into M70.

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot