tzik@, could you please help to find an owner for this?

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

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

Looks like a dangling content::RenderFrameImpl::frame_ at http://crrev.com/711409eabeacb27e/content/renderer/render_frame_impl.cc#5621

clamy: Could you handle this?

Sorry missed this. This seems to be a duplicate of issue 639688 which was fixed. Marking it as such.

clamy@: My fix for issue 639688 was just a null check, which wouldn't help against a use-after-free (in which case the pointer is non-null but is no longer valid).  I'm concerned that this may still be an issue, though I haven't confirmed.

Re-opening until we can verify whether the bug still exists or not.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6372324138024960

Fuzzer: mbarbella_js_mutation_layout
Job Type: linux_asan_content_shell_drt
Platform Id: linux

Crash Type: Heap-use-after-free READ 8
Crash Address: 0x61800014f8a8
Crash State:
  content::RenderFrameImpl::NavigateInternal
  content::RenderFrameImpl::OnNavigate
  bool IPC::MessageT<FrameMsg_Navigate_Meta, std::__1::tuple<content::CommonNaviga
  
Recommended Security Severity: High

Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_content_shell_drt&range=410634:410757

Minimized Testcase (1.33 Kb): https://cluster-fuzz.appspot.com/download/AMIfv94ek5dW0Ucrxx36gKG5jPNPe9fUT44xEVQ-xF_UU2gMAQXLc_nHz1LHyc738Ouqk6G3QHCZVPR2UxIIE2NVlo-3Y4HF_lmdVBsB78yACPRPMA9vbWz2z9MK_jKbKkWyF18bTCb1zXFVuJ61nWfD2-xrn4tx5g?testcase_id=6372324138024960

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

Ignore c#8. I was planning on associating the test case with another bug based on some discussion on to how to handle this, but keeping it here is best.

I have a local repro for the crash, where we just remove an about:blank frame from the page during onload.  (This is basically what ClusterFuzz's repro is doing as well.)

Repro steps:
1) Visit bug-638166.html (attached).
2) Navigate to chromium.org.
3) Go back.

I'll also note that the underlying issue affects RenderFrameImpl::OnStop as well (issue 639689).

Fix on the way.

Deleted: bug-638166.html 226 bytes

bug-638166.html 226 bytes View Download

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

commit ba53b47ffb07652d639e68db92743dc9aea21e5c
Author: creis <creis@chromium.org>
Date: Thu Sep 01 22:19:16 2016

Avoid use-after-free if frame is deleted when stopping loading.

WebFrame::stopLoading can run onload event handlers, which have
the ability to delete the frame.  This means we must be careful
when calling it from within RenderFrameImpl, or else the
remainder of the function may try to access a deleted object.

BUG= 638166 , 639689
TEST=See  bug 638166  comment 11
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2307463003
Cr-Commit-Position: refs/heads/master@{#416082}

[modify] https://crrev.com/ba53b47ffb07652d639e68db92743dc9aea21e5c/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/ba53b47ffb07652d639e68db92743dc9aea21e5c/content/renderer/render_frame_impl.cc
[add] https://crrev.com/ba53b47ffb07652d639e68db92743dc9aea21e5c/content/test/data/navigation_controller/remove_blank_iframe_on_load.html

Should be fixed in r416082.  I'll request a merge to M54 once this has baked on Canary.

I can confirm that the repro steps in comment 11 are fixed in 55.0.2847.0.  If it looks good after the weekend, I'll request a merge on Tuesday.

ClusterFuzz has detected this issue as fixed in range 415934:416243.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6372324138024960

Fuzzer: mbarbella_js_mutation_layout
Job Type: linux_asan_content_shell_drt
Platform Id: linux

Crash Type: Heap-use-after-free READ 8
Crash Address: 0x61800014f8a8
Crash State:
  content::RenderFrameImpl::NavigateInternal
  content::RenderFrameImpl::OnNavigate
  bool IPC::MessageT<FrameMsg_Navigate_Meta, std::__1::tuple<content::CommonNaviga
  
Recommended Security Severity: High

Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_content_shell_drt&range=410634:410757
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_content_shell_drt&range=415934:416243

Minimized Testcase (1.33 Kb): https://cluster-fuzz.appspot.com/download/AMIfv94ek5dW0Ucrxx36gKG5jPNPe9fUT44xEVQ-xF_UU2gMAQXLc_nHz1LHyc738Ouqk6G3QHCZVPR2UxIIE2NVlo-3Y4HF_lmdVBsB78yACPRPMA9vbWz2z9MK_jKbKkWyF18bTCb1zXFVuJ61nWfD2-xrn4tx5g?testcase_id=6372324138024960

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

Things look good, and ClusterFuzz confirmed the fix.  Ok to merge to M54?

Your change meets the bar and is auto-approved for M54 (branch: 2840)

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

commit 670a635d98f3f5674b87d75141bd17253f2f0a5c
Author: Charles Reis <creis@chromium.org>
Date: Tue Sep 06 16:15:54 2016

Avoid use-after-free if frame is deleted when stopping loading.

WebFrame::stopLoading can run onload event handlers, which have
the ability to delete the frame.  This means we must be careful
when calling it from within RenderFrameImpl, or else the
remainder of the function may try to access a deleted object.

BUG= 638166 , 639689
TEST=See  bug 638166  comment 11
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2307463003
Cr-Commit-Position: refs/heads/master@{#416082}
(cherry picked from commit ba53b47ffb07652d639e68db92743dc9aea21e5c)

Review URL: https://codereview.chromium.org/2312243002 .

Cr-Commit-Position: refs/branch-heads/2840@{#171}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/670a635d98f3f5674b87d75141bd17253f2f0a5c/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/670a635d98f3f5674b87d75141bd17253f2f0a5c/content/renderer/render_frame_impl.cc
[add] https://crrev.com/670a635d98f3f5674b87d75141bd17253f2f0a5c/content/test/data/navigation_controller/remove_blank_iframe_on_load.html

The fix was released in Beta in 54.0.2840.16 on 2016-09-08, and must bake for at least 48 hours before merging to 2785.  Assuming no problems are highlighted, please merge on Sunday 2016-09-11 or Monday 2016-09-12 before 1500 Pacific, as we're going to be spinning a stable update then.  Thanks and pardon the short notice.

[Automated comment] Request affecting a post-stable build (M53), manual review required.

Approving merge to M53 branch 2785, please see comment #21 for when to merge.

awhalley@: The NavigateInternal bug wasn't introduced until r410748, which was in M54.  That bug didn't exist in M53, so we can't merge the CL as is.

That said, my CL also fixes a similar theoretical problem with OnStop, which did exist in M53.  I don't have repro steps for that one (and ClusterFuzz didn't notice it), but it might be possible for me to merge just that part to M53.  It would boil down to the following patch:

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 9136254fbe5ed2d697e2982dbaaadd6b2ce68e3a..972c57952405706bef205a6a8ad6c33dff2950c0 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -4473,7 +4473,14 @@ void RenderFrameImpl::RemoveObserver(RenderFrameObserver* observer) {
 
 void RenderFrameImpl::OnStop() {
   DCHECK(frame_);
+
+  // The stopLoading call may run script, which may cause this frame to be
+  // detached/deleted.  If that happens, return immediately.
+  base::WeakPtr<RenderFrameImpl> weak_this = weak_factory_.GetWeakPtr();
   frame_->stopLoading();
+  if (!weak_this)
+    return;
+
   if (frame_ && !frame_->parent())
     FOR_EACH_OBSERVER(RenderViewObserver, render_view_->observers_, OnStop());


Do you want me to try to merge just that part?

creis@ - thanks for the details. Yes, That would be great, and I can't see it causing any problems that wouldn't also be exhibited by the larger change.

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

commit 0a8161cadb9c8b3112b7ef1c04238ce762ade97a
Author: creis <creis@chromium.org>
Date: Mon Sep 12 19:04:10 2016

Avoid use-after-free if frame is deleted when stopping loading.

WebFrame::stopLoading can run onload event handlers, which have
the ability to delete the frame.  This means we must be careful
when calling it from within RenderFrameImpl, or else the
remainder of the function may try to access a deleted object.

BUG= 638166 , 639689
TEST=See  bug 638166  comment 11
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2307463003
Cr-Commit-Position: refs/heads/master@{#416082}
(cherry picked from commit ba53b47ffb07652d639e68db92743dc9aea21e5c)

Review-Url: https://codereview.chromium.org/2335753002
Cr-Commit-Position: refs/branch-heads/2785@{#876}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/0a8161cadb9c8b3112b7ef1c04238ce762ade97a/content/renderer/render_frame_impl.cc

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

commit 670a635d98f3f5674b87d75141bd17253f2f0a5c
Author: Charles Reis <creis@chromium.org>
Date: Tue Sep 06 16:15:54 2016

Avoid use-after-free if frame is deleted when stopping loading.

WebFrame::stopLoading can run onload event handlers, which have
the ability to delete the frame.  This means we must be careful
when calling it from within RenderFrameImpl, or else the
remainder of the function may try to access a deleted object.

BUG= 638166 , 639689
TEST=See  bug 638166  comment 11
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2307463003
Cr-Commit-Position: refs/heads/master@{#416082}
(cherry picked from commit ba53b47ffb07652d639e68db92743dc9aea21e5c)

Review URL: https://codereview.chromium.org/2312243002 .

Cr-Commit-Position: refs/branch-heads/2840@{#171}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/670a635d98f3f5674b87d75141bd17253f2f0a5c/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/670a635d98f3f5674b87d75141bd17253f2f0a5c/content/renderer/render_frame_impl.cc
[add] https://crrev.com/670a635d98f3f5674b87d75141bd17253f2f0a5c/content/test/data/navigation_controller/remove_blank_iframe_on_load.html

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