Fix use-after-free when navigating a subframe to about:blank.

Fix use-after-free when navigating a subframe to about:blank.

Navigation to about:blank is a synchronous navigation. If the parent frame
has registered load event handler for the frame and removes it from the
DOM, it will result in RenderFrame being deleted while still being on the
stack. This CL is fixing this by returning immediately if the object is
destructed as part of the navigation.

BUG=571166, 591341
Committed: https://crrev.com/13b8e77d00895fd3d24aaef7f32eeb4adb68a080
Cr-Commit-Position: refs/heads/master@{#379060}

[nasko, 2016-03-03 00:05:23]

nasko@chromium.org changed reviewers:
+ dcheng@chromium.org

[nasko, 2016-03-03 00:05:24]

Hey Daniel,
Can you review this CL for me? We've discussed the fix on chat, so it should be
an easy one.
Thanks!
Nasko

[dcheng, 2016-03-03 00:46:59]

https://codereview.chromium.org/1756483004/diff/1/content/browser/site_per_pr...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1756483004/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:4762: base::StringPrintf("f.src
= '%s'", url::kAboutBlankURL)));
So this navigation is sync?

https://codereview.chromium.org/1756483004/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1756483004/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:5514: if (!weak_this.get())
No .get()

https://codereview.chromium.org/1756483004/diff/1/content/test/data/remove_fr...
File content/test/data/remove_frame_on_load.html (right):

https://codereview.chromium.org/1756483004/diff/1/content/test/data/remove_fr...
content/test/data/remove_frame_on_load.html:7: f = document.getElementById("f");
var f = document.querySelector('iframe'), then you don't need to even name it

[nasko, 2016-03-03 16:59:18]

https://codereview.chromium.org/1756483004/diff/1/content/browser/site_per_pr...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1756483004/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:4762: base::StringPrintf("f.src
= '%s'", url::kAboutBlankURL)));
On 2016/03/03 00:46:59, dcheng wrote:
> So this navigation is sync?

Yes, otherwise it won't repro. I don't think the statement execution actually
results in a sync navigation. Rather, it hits the RemoteFrame, which sends the
navigation to the browser, which then does the cross-process swap and sends a
navigation to the newly created LocalFrame for "about:blank".

https://codereview.chromium.org/1756483004/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1756483004/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:5514: if (!weak_this.get())
On 2016/03/03 00:46:59, dcheng wrote:
> No .get()

Done.

https://codereview.chromium.org/1756483004/diff/1/content/test/data/remove_fr...
File content/test/data/remove_frame_on_load.html (right):

https://codereview.chromium.org/1756483004/diff/1/content/test/data/remove_fr...
content/test/data/remove_frame_on_load.html:7: f = document.getElementById("f");
On 2016/03/03 00:46:59, dcheng wrote:
> var f = document.querySelector('iframe'), then you don't need to even name it

Done.

[dcheng, 2016-03-03 18:24:14]

dcheng@chromium.org changed reviewers:
+ japhet@chromium.org

[dcheng, 2016-03-03 18:24:16]

I thought I sent this out already but lgtm

+japhet, is there anything in the spec that says we have to complete load of a
non-initial about:blank page synchronously? Maybe that'd be a better way in the
long run to mitigate this bug?

https://codereview.chromium.org/1756483004/diff/20001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1756483004/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:4761:
ExecuteScript(root->current_frame_host(),
I was going to suggest that this test could be written entirely in JS… but I
guess that would make the synchronization harder. So this seems fine.

[Nate Chapin, 2016-03-03 19:04:45]

On 2016/03/03 18:24:16, dcheng wrote:
> I thought I sent this out already but lgtm
> 
> +japhet, is there anything in the spec that says we have to complete load of a
> non-initial about:blank page synchronously? Maybe that'd be a better way in
the
> long run to mitigate this bug?

I can't find anything in the spec, but I wouldn't be surprised if webpages
depend on it anyway :)

I'm not sure what FF and IE/Edge do.


> 
>
https://codereview.chromium.org/1756483004/diff/20001/content/browser/site_pe...
> File content/browser/site_per_process_browsertest.cc (right):
> 
>
https://codereview.chromium.org/1756483004/diff/20001/content/browser/site_pe...
> content/browser/site_per_process_browsertest.cc:4761:
> ExecuteScript(root->current_frame_host(),
> I was going to suggest that this test could be written entirely in JS… but I
> guess that would make the synchronization harder. So this seems fine.

[nasko, 2016-03-03 19:34:55]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2016-03-03 19:35:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1756483004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1756483004/20001

[commit-bot: I haz the power, 2016-03-03 19:41:43]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-03 19:45:15]

Description was changed from

==========
Fix use-after-free when navigating a subframe to about:blank.

Navigation to about:blank is a synchronous navigation. If the parent frame
has registered load event handler for the frame and removes it from the
DOM, it will result in RenderFrame being deleted while still being on the
stack. This CL is fixing this by returning immediately if the object is
destructed as part of the navigation.

BUG=571166, 591341
==========

to

==========
Fix use-after-free when navigating a subframe to about:blank.

Navigation to about:blank is a synchronous navigation. If the parent frame
has registered load event handler for the frame and removes it from the
DOM, it will result in RenderFrame being deleted while still being on the
stack. This CL is fixing this by returning immediately if the object is
destructed as part of the navigation.

BUG=571166, 591341

Committed: https://crrev.com/13b8e77d00895fd3d24aaef7f32eeb4adb68a080
Cr-Commit-Position: refs/heads/master@{#379060}
==========

[commit-bot: I haz the power, 2016-03-03 19:45:16]

Patchset 2 (id:??) landed as
https://crrev.com/13b8e77d00895fd3d24aaef7f32eeb4adb68a080
Cr-Commit-Position: refs/heads/master@{#379060}