CHECK failure: webview()->MainFrame()->IsWebLocalFrame() in render_view_impl.cc |
|||||
Issue descriptionDetailed report: https://clusterfuzz.com/testcase?key=6210674838208512 Fuzzer: inferno_layout_test_fuzzer Job Type: linux_ubsan_vptr_content_shell_drt Platform Id: linux Crash Type: CHECK failure Crash Address: Crash State: webview()->MainFrame()->IsWebLocalFrame() in render_view_impl.cc content::RenderViewImpl::OnSetFocus test_runner::TestRunner::SetFocus Sanitizer: undefined (UBSAN) Regressed: https://clusterfuzz.com/revisions?job=linux_ubsan_vptr_content_shell_drt&range=555986:555990 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6210674838208512 Issue filed automatically. See https://github.com/google/clusterfuzz-tools for more information.
,
May 4 2018
Automatically adding ccs based on suspected regression changelists: [CI] PaintInvalidationReason::kChunkAppeared and kChunkDisappeared by wangxianzhu@chromium.org - https://chromium.googlesource.com/chromium/src/+/fe5edcbdc288826325a27bcf0c9fdb0948e3764f Implement process isolation for chrome-error:// documents. by nasko@chromium.org - https://chromium.googlesource.com/chromium/src/+/d83b571dc700010f144b68fe3d8628923c008773 If this is incorrect, please let us know why and apply the Test-Predator-Wrong-CLs label.
,
May 4 2018
My CL only changed some names and can't cause such failure.
,
May 5 2018
Yes, this is indeed caused by my CL. However, this is test harness code, so I will move the priority to P2 as this is not problem in production code.
The underlying issue is that the layout tests support code is calling directly into content::RenderViewImpl::OnSetFocus, even when the main frame is in a remote process. The test case basically does a window.open() to random string, which is not a valid URL and that causes an error. My CL changes the error page to be committed in a different process, which causes the RenderViewImpl to not have a LocalFrame for the main frame. OnSetFocus expects to only be called for main frame cases where it is LocalFrame, hence why we hit a CHECK.
This patch avoids the crash, which is clearly in test only code, though I don't know whether it does the right thing for the case of disabling focus:
@@ -2300,11 +2300,19 @@ void RenderViewImpl::SetFocusAndActivateForTesting(bool enable) {
if (enable) {
if (has_focus())
return;
+ if (!webview()->MainFrame()->IsWebLocalFrame()) {
+ RenderFrameProxy* proxy = RenderFrameProxy::FromWebFrame(
+ webview()->MainFrame()->ToWebRemoteFrame());
+ proxy->FrameFocused();
+ return;
+ }
OnSetActive(true);
OnSetFocus(true);
} else {
if (!has_focus())
return;
+ if (!webview()->MainFrame()->IsWebLocalFrame())
+ return;
OnSetFocus(false);
OnSetActive(false);
}
I will put up a CL next week and discuss with alexmos@ what is the right way to handle the case where enable is false and the main frame is remote.
,
May 7 2018
OnSetActive() and OnSetFocus() actually both pertain to page-level focus, not frame focus. I.e., they shouldn't alter which frame is focused, but just focus or blur the whole window. They're also replicated across all renderers - see RenderFrameProxy::OnSetPageFocus(), which calls into RVI::SetFocus(). So, in the case of a remote main frame, I'd expect this to go through proxy->OnSetPageFocus() rather than proxy->FrameFocused() (the latter would be for something like window.focus). That said, it might actually be better to just early return from SetFocusAndActivateForTesting in the remote frame case and add that support later if any layout tests actually need it, since it's likely that if a test needs this, it should set page focus from the browser side so that it gets replicated to all renderers.
,
May 8 2018
CL out for review https://chromium-review.googlesource.com/c/chromium/src/+/1032751
,
May 8 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a463a050fe32a472e54df98096fedf388949ecc0 commit a463a050fe32a472e54df98096fedf388949ecc0 Author: Nasko Oskov <nasko@chromium.org> Date: Tue May 08 16:42:14 2018 Return early for WebRemoteFrames in RenderViewImpl::SetFocusAndActivateForTesting RenderViewImpl::SetFocusAndActivateForTesting is setting page level focus which is not supported for out-of-process main frame. This CL adds an early return for this case to avoid hitting CHECK during tests. Bug: 839783 Change-Id: Ie1d613647f78e72a943fc35cc0366b96e84cdad5 Reviewed-on: https://chromium-review.googlesource.com/1049114 Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Commit-Queue: Nasko Oskov <nasko@chromium.org> Cr-Commit-Position: refs/heads/master@{#556833} [modify] https://crrev.com/a463a050fe32a472e54df98096fedf388949ecc0/content/renderer/render_view_impl.cc
,
May 9 2018
ClusterFuzz has detected this issue as fixed in range 556831:556834. Detailed report: https://clusterfuzz.com/testcase?key=6210674838208512 Fuzzer: inferno_layout_test_fuzzer Job Type: linux_ubsan_vptr_content_shell_drt Platform Id: linux Crash Type: CHECK failure Crash Address: Crash State: webview()->MainFrame()->IsWebLocalFrame() in render_view_impl.cc content::RenderViewImpl::OnSetFocus test_runner::TestRunner::SetFocus Sanitizer: undefined (UBSAN) Regressed: https://clusterfuzz.com/revisions?job=linux_ubsan_vptr_content_shell_drt&range=555986:555990 Fixed: https://clusterfuzz.com/revisions?job=linux_ubsan_vptr_content_shell_drt&range=556831:556834 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6210674838208512 See https://github.com/google/clusterfuzz-tools for more information. If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
,
May 9 2018
ClusterFuzz testcase 6210674838208512 is verified as fixed, so closing issue as verified. If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by ClusterFuzz
, May 4 2018Labels: Test-Predator-Auto-Components