New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 839783 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Out until 24 Jan
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug



Sign in to add a comment

CHECK failure: webview()->MainFrame()->IsWebLocalFrame() in render_view_impl.cc

Project Member Reported by ClusterFuzz, May 4 2018

Issue description

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

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6210674838208512

Issue filed automatically.

See https://github.com/google/clusterfuzz-tools for more information.
 
Project Member

Comment 1 by ClusterFuzz, May 4 2018

Components: Internals>Core
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

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

Comment 2 by ClusterFuzz, May 4 2018

Cc: wangxianzhu@chromium.org nasko@chromium.org
Labels: Test-Predator-Auto-CC
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.
My CL only changed some names and can't cause such failure.

Comment 4 by nasko@chromium.org, May 5 2018

Cc: alex...@chromium.org
Labels: -Pri-1 Pri-2
Owner: nasko@chromium.org
Status: Assigned (was: Untriaged)
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.
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.

Comment 6 by nasko@chromium.org, May 8 2018

Status: Started (was: Assigned)
CL out for review https://chromium-review.googlesource.com/c/chromium/src/+/1032751
Project Member

Comment 7 by bugdroid1@chromium.org, 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

Project Member

Comment 8 by ClusterFuzz, 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.
Project Member

Comment 9 by ClusterFuzz, May 9 2018

Labels: ClusterFuzz-Verified
Status: Verified (was: Started)
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