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

Issue 907106 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 4
Cc:
Components:
EstimatedDays: 2
NextAction: 2018-12-07
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug



Sign in to add a comment

CHECK failure: !render_view->main_render_frame_ in render_frame_impl.cc

Project Member Reported by ClusterFuzz, Nov 20

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=4576166574555136

Fuzzer: mbarbella_js_mutation_layout
Job Type: linux_msan_content_shell_drt
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  !render_view->main_render_frame_ in render_frame_impl.cc
  content::RenderFrameImpl::OnSwapOut
  bool IPC::MessageT<FrameMsg_SwapOut_Meta, std::__1::tuple<int, bool, content::Fr
  
Sanitizer: memory (MSAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_msan_content_shell_drt&range=606033:606039

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

Issue filed automatically.

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

Comment 1 by ClusterFuzz, Nov 20

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, Nov 20

Labels: Test-Predator-Auto-Owner
Owner: arthurso...@chromium.org
Status: Assigned (was: Untriaged)
Automatically assigning owner based on suspected regression changelist https://chromium.googlesource.com/chromium/src/+/f8840b9771e2686d3054a311440abb48ac636e65 (Keep subframe alive in pending deletion.).

If this is incorrect, please let us know why and apply the Test-Predator-Wrong-CLs label. If you aren't the correct owner for this issue, please unassign yourself as soon as possible so it can be re-triaged.
Cc: clamy@chromium.org alex...@chromium.org nasko@chromium.org
Components: -Internals>Core UI>Browser>Navigation
EstimatedDays: 2
Labels: OS-Android OS-Chrome OS-Fuchsia OS-Mac OS-Windows
NextAction: 2018-11-26
I don't have time to fix it immediately, but I will as soon as my other bugs are fixed.

+CC: FYI.
Status: Started (was: Assigned)
I took a quick look:
Test case:
~~~
<a href="about:blank" id="a"></a>
<iframe id="iframe"></iframe>
<script>
  iframe.contentWindow.onunload = () => {
    a.click();
  };
  location.href = 'check-ping.php';
</script>
~~~

In RenderFrameImpl::Swapout(). It reaches:
~~~
  // For main frames, the swap should have cleared the RenderView's pointer to
  // this frame.
  if (is_main_frame)
    CHECK(!render_view->main_render_frame_);
~~~

It looks like an easy to fix bug. As soon as I understand what this check is about...
The NextAction date has arrived: 2018-11-26
I understand.
1) The document in the main frame runs its unload handler and detaches itself. It set main_render_frame_ to null.
2) Then the document in the subframe runs its unload handler and makes the main frame to navigate to about:blank. It sets main_render_frame_ to non-nullptr.
3) The CHECK is reached after leaving the Swap(..) function.

Navigations in an unloading frame shouldn't be allowed.
Fix + regression tests:
https://chromium-review.googlesource.com/c/chromium/src/+/1350970
Labels: ReleaseBlock-Stable RegressedIn-72 Target-72
Project Member

Comment 8 by sheriffbot@chromium.org, Nov 29

This issue is marked as a release blocker with no milestone associated. Please add an appropriate milestone.

All release blocking issues should have milestones associated to it, so that the issue can tracked and the fixes can be pushed promptly.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: M-72
Project Member

Comment 10 by bugdroid1@chromium.org, Dec 4

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

commit 05083730be6c1218024b485584a440fb78e675f3
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Tue Dec 04 10:42:09 2018

Block new navigations while unloading.

One hole in blocking navigation while unloading remained. It's when the
navigation is triggered from the subframe's unload handler.

It fixes  bug 907106  and 773683.

Test ContentBrowserTest.HistoryBackInUnloadCancelsReload is removed,
because it was explicitly relying on bug 773683.

Bug:  907106 ,773683
Change-Id: I65755f636bb258cc55fba9d6775eb101da31b735
Reviewed-on: https://chromium-review.googlesource.com/c/1350970
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613498}
[modify] https://crrev.com/05083730be6c1218024b485584a440fb78e675f3/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/05083730be6c1218024b485584a440fb78e675f3/third_party/blink/renderer/core/loader/frame_loader.cc
[add] https://crrev.com/05083730be6c1218024b485584a440fb78e675f3/third_party/blink/web_tests/http/tests/navigation/navigate-to-about-blank-from-subframe-unload-handler-cross-origin-expected.txt
[add] https://crrev.com/05083730be6c1218024b485584a440fb78e675f3/third_party/blink/web_tests/http/tests/navigation/navigate-to-about-blank-from-subframe-unload-handler-cross-origin.html
[add] https://crrev.com/05083730be6c1218024b485584a440fb78e675f3/third_party/blink/web_tests/http/tests/navigation/navigate-to-about-blank-from-subframe-unload-handler-same-origin-expected.txt
[add] https://crrev.com/05083730be6c1218024b485584a440fb78e675f3/third_party/blink/web_tests/http/tests/navigation/navigate-to-about-blank-from-subframe-unload-handler-same-origin.html

Labels: Merge-Request-72
I am requesting merging this change into M72:

- This is a one line change, it is not very complicated.
- It fixed two M72 stable blockers:  bug 907106  and probably bug 904248.
- It fixes an old bug (bug 773683). Malicous website are no more able to prevent the user from navigating away from they page.

I will wait for this patch to be tested on canary for 4 days before going to M72.

Project Member

Comment 12 by ClusterFuzz, Dec 4

ClusterFuzz has detected this issue as fixed in range 613497:613498.

Detailed report: https://clusterfuzz.com/testcase?key=4576166574555136

Fuzzer: mbarbella_js_mutation_layout
Job Type: linux_msan_content_shell_drt
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  !render_view->main_render_frame_ in render_frame_impl.cc
  content::RenderFrameImpl::OnSwapOut
  bool IPC::MessageT<FrameMsg_SwapOut_Meta, std::__1::tuple<int, bool, content::Fr
  
Sanitizer: memory (MSAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_msan_content_shell_drt&range=606033:606039
Fixed: https://clusterfuzz.com/revisions?job=linux_msan_content_shell_drt&range=613497:613498

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

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.

Comment 13 Deleted

Thanks ClusterFuzz! :-)

Waiting on merging this patch into M72 before closing this bug. See comment 11.
Project Member

Comment 15 by ClusterFuzz, Dec 4

Labels: ClusterFuzz-Verified
Status: Verified (was: Started)
ClusterFuzz testcase 4576166574555136 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Labels: Merge-Approved-72
Pls merge your change to M72 branch 3626 ASAP so we can pick it up for tomororw's dev release. Thank you.
Cc: gov...@chromium.org
NextAction: 2018-12-07
I prepared and approved the cherry-pick here:
https://chromium-review.googlesource.com/c/chromium/src/+/1361711

As I said in comment 11, I would like to be sure it behaves nicely on Canary first. It has never been used in any version so far. Can I wait for 2 days?
Maybe I'm too careful, it's only a Dev release. Feel free to submit it immediately if you think it should land now.
Project Member

Comment 19 by sheriffbot@chromium.org, Dec 5

Labels: -Merge-Request-72 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M72. Please go ahead and merge the CL to branch 3626 manually. Please contact milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Re #18, Sure, it can wait for 2 days, if  you like to wait more even Monday morning is also fine. 
Project Member

Comment 21 by bugdroid1@chromium.org, Dec 7

Labels: -merge-approved-72 merge-merged-3626
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f3faa6ac1915d41cd4bb06adcc4d8b2802666f54

commit f3faa6ac1915d41cd4bb06adcc4d8b2802666f54
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Fri Dec 07 08:45:11 2018

Block new navigations while unloading.

One hole in blocking navigation while unloading remained. It's when the
navigation is triggered from the subframe's unload handler.

It fixes  bug 907106  and 773683.

Test ContentBrowserTest.HistoryBackInUnloadCancelsReload is removed,
because it was explicitly relying on bug 773683.

Bug:  907106 ,773683
Change-Id: I65755f636bb258cc55fba9d6775eb101da31b735
Reviewed-on: https://chromium-review.googlesource.com/c/1350970
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#613498}(cherry picked from commit 05083730be6c1218024b485584a440fb78e675f3)
Reviewed-on: https://chromium-review.googlesource.com/c/1361711
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#132}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/f3faa6ac1915d41cd4bb06adcc4d8b2802666f54/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/f3faa6ac1915d41cd4bb06adcc4d8b2802666f54/third_party/blink/renderer/core/loader/frame_loader.cc
[add] https://crrev.com/f3faa6ac1915d41cd4bb06adcc4d8b2802666f54/third_party/blink/web_tests/http/tests/navigation/navigate-to-about-blank-from-subframe-unload-handler-cross-origin-expected.txt
[add] https://crrev.com/f3faa6ac1915d41cd4bb06adcc4d8b2802666f54/third_party/blink/web_tests/http/tests/navigation/navigate-to-about-blank-from-subframe-unload-handler-cross-origin.html
[add] https://crrev.com/f3faa6ac1915d41cd4bb06adcc4d8b2802666f54/third_party/blink/web_tests/http/tests/navigation/navigate-to-about-blank-from-subframe-unload-handler-same-origin-expected.txt
[add] https://crrev.com/f3faa6ac1915d41cd4bb06adcc4d8b2802666f54/third_party/blink/web_tests/http/tests/navigation/navigate-to-about-blank-from-subframe-unload-handler-same-origin.html

Status: Fixed (was: Verified)
The NextAction date has arrived: 2018-12-07
Labels: Merge-Merged-72-3626
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/f3faa6ac1915d41cd4bb06adcc4d8b2802666f54

Commit: f3faa6ac1915d41cd4bb06adcc4d8b2802666f54
Author: arthursonzogni@chromium.org
Commiter: arthursonzogni@chromium.org
Date: 2018-12-07 08:45:11 +0000 UTC

Block new navigations while unloading.

One hole in blocking navigation while unloading remained. It's when the
navigation is triggered from the subframe's unload handler.

It fixes  bug 907106  and 773683.

Test ContentBrowserTest.HistoryBackInUnloadCancelsReload is removed,
because it was explicitly relying on bug 773683.

Bug:  907106 ,773683
Change-Id: I65755f636bb258cc55fba9d6775eb101da31b735
Reviewed-on: https://chromium-review.googlesource.com/c/1350970
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#613498}(cherry picked from commit 05083730be6c1218024b485584a440fb78e675f3)
Reviewed-on: https://chromium-review.googlesource.com/c/1361711
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#132}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}

Sign in to add a comment