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

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

Committed: https://crrev.com/ba53b47ffb07652d639e68db92743dc9aea21e5c
Cr-Commit-Position: refs/heads/master@{#416082}

[Charlie Reis, 2016-09-01 21:09:18]

Description was changed from

==========
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
==========

to

==========
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
==========

[Charlie Reis, 2016-09-01 21:11:37]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 21:12:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Reis, 2016-09-01 21:14:15]

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

[Charlie Reis, 2016-09-01 21:14:16]

Daniel, can you review this since Nasko is out?  It follows the pattern Nasko
used and you reviewed in https://codereview.chromium.org/1756483004/.

(I'm not thrilled with the idea of using WeakPtr as a general pattern after
calls that can run script, but I at least have plans to remove the
NavigateInternal case in https://crbug.com/639842.  This CL is needed for
merging, though.)

[dcheng, 2016-09-01 21:20:15]

Hmm...

One thought, though I'm not sure if this is better... what if WebFrame just told
you if it was detached? So instead, the pattern is something like:

  WebLocalFrame* frame = frame_;
  frame->stopLoading();
  if (frame->isDetached())
    return;

This would be strictly better if we could get rid of the WeakPtrFactory
completely, but it doesn't look like that's possible.

+esprehn as well

[Charlie Reis, 2016-09-01 21:38:02]

On 2016/09/01 21:20:15, dcheng wrote:
> Hmm...
> 
> One thought, though I'm not sure if this is better... what if WebFrame just
told
> you if it was detached? So instead, the pattern is something like:
> 
>   WebLocalFrame* frame = frame_;
>   frame->stopLoading();
>   if (frame->isDetached())
>     return;
> 
> This would be strictly better if we could get rid of the WeakPtrFactory
> completely, but it doesn't look like that's possible.
> 
> +esprehn as well

We chatted a bit offline.  It does seem like having a better pattern for
detecting when a WebFrame call deletes the frame is worthwhile, though it's not
clear yet what to settle on.  In this case, isDetached probably wouldn't be safe
to call after |frame| is deleted.

Another option is having things like stopLoading (and anything else that can run
script?) return a bool for whether the frame still exists, but that does seem
subtle and awkward.  Ideas welcome, though we should find something mergeable
here for the short term.

[dcheng, 2016-09-01 21:45:51]

This fix LGTM in the short-term.

[Charlie Reis, 2016-09-01 22:02:36]

The CQ bit was unchecked by creis@chromium.org

[Charlie Reis, 2016-09-01 22:02:40]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2016-09-01 22:03:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 22:19:47]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-01 22:22:48]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/ba53b47ffb07652d639e68db92743dc9aea21e5c
Cr-Commit-Position: refs/heads/master@{#416082}
==========

[commit-bot: I haz the power, 2016-09-01 22:22:49]

Patchset 1 (id:??) landed as
https://crrev.com/ba53b47ffb07652d639e68db92743dc9aea21e5c
Cr-Commit-Position: refs/heads/master@{#416082}

[Charlie Reis, 2016-09-12 18:55:54]

Description was changed from

==========
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

Committed: https://crrev.com/ba53b47ffb07652d639e68db92743dc9aea21e5c
Cr-Commit-Position: refs/heads/master@{#416082}
==========

to

==========
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

Committed: https://crrev.com/ba53b47ffb07652d639e68db92743dc9aea21e5c
Cr-Commit-Position: refs/heads/master@{#416082}
==========

[Charlie Reis, 2016-09-12 18:56:16]

Description was changed from

==========
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

Committed: https://crrev.com/ba53b47ffb07652d639e68db92743dc9aea21e5c
Cr-Commit-Position: refs/heads/master@{#416082}
==========

to

==========
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

Committed: https://crrev.com/ba53b47ffb07652d639e68db92743dc9aea21e5c
Cr-Commit-Position: refs/heads/master@{#416082}
==========

[LaurentBarbareau, 2016-09-20 09:49:13]

Hi,

I suspect this modification to be responsible of new problems we're facing with
the bad rendering of frames of the web application we develop.

Prior to version 53.0.2785.116 m it was ok and just after having installed this
update, most of the time at least one or two frames are not rendered properly.
Something "funny", if I refresh only one frame at a time, this latter is then
reloaded and well rendered but another frame will be messed up, yet I think it
is not supposed to be reload with JS or any other way (no request sent to the
server).

Verified on different PCs, compared versions of Chrome, on Windows 7, 10. Our
web application is still working fine with other web browsers.

I also checked the head declaration of our frame sources and some of them were
not specifying any DOCTYPE, then I added it (<!DOCTYPE HTML PUBLIC "-//W3C//DTD
HTML 4.01 Frameset//EN" "http://www.w3.org/TR/html4/frameset.dtd">) everywhere
it could be necessary. At first sight it looked like better but finally, there
is no change...

I'm not opening a bug report here, as mentioned, I suspect this modification to
be the cause of our problem. What do you think about it?

Thank you.

[dcheng, 2016-09-20 09:59:14]

On 2016/09/20 09:49:13, LaurentBarbareau wrote:
> Hi,
> 
> I suspect this modification to be responsible of new problems we're facing
with
> the bad rendering of frames of the web application we develop.
> 
> Prior to version 53.0.2785.116 m it was ok and just after having installed
this
> update, most of the time at least one or two frames are not rendered properly.
> Something "funny", if I refresh only one frame at a time, this latter is then
> reloaded and well rendered but another frame will be messed up, yet I think it
> is not supposed to be reload with JS or any other way (no request sent to the
> server).
> 
> Verified on different PCs, compared versions of Chrome, on Windows 7, 10. Our
> web application is still working fine with other web browsers.
> 
> I also checked the head declaration of our frame sources and some of them were
> not specifying any DOCTYPE, then I added it (<!DOCTYPE HTML PUBLIC
"-//W3C//DTD
> HTML 4.01 Frameset//EN" "http://www.w3.org/TR/html4/frameset.dtd">) everywhere
> it could be necessary. At first sight it looked like better but finally, there
> is no change...
> 
> I'm not opening a bug report here, as mentioned, I suspect this modification
to
> be the cause of our problem. What do you think about it?
> 
> Thank you.

It's unlikely to be this fix: this simply prevents Chrome from crashing if a
frame is detached from the DOM via something like
iframe.parentElement.removeChild(iframe): in that case, the frame won't even be
visible anymore. If you have a reliable repro, I'd recommend using the
tools/bisect-builds.py script to get an accurate regression range: or if the
repro is publicly accessible, file a bug, and someone can help bisect it down as
well.

[LaurentBarbareau, 2016-09-20 16:49:01]

> It's unlikely to be this fix: this simply prevents Chrome from crashing if a
> frame is detached from the DOM via something like
> iframe.parentElement.removeChild(iframe): in that case, the frame won't even
be
> visible anymore. If you have a reliable repro, I'd recommend using the
> tools/bisect-builds.py script to get an accurate regression range: or if the
> repro is publicly accessible, file a bug, and someone can help bisect it down
as
> well.
Thank you for your reply. 
Just one thing, I only talk about frame and frameset, not iframe.
I'll try to give you a temporary public address of our intranet application if
necessary.

[Charlie Reis, 2016-09-20 18:15:30]

On 2016/09/20 16:49:01, LaurentBarbareau wrote:
> > It's unlikely to be this fix: this simply prevents Chrome from crashing if a
> > frame is detached from the DOM via something like
> > iframe.parentElement.removeChild(iframe): in that case, the frame won't even
> be
> > visible anymore. If you have a reliable repro, I'd recommend using the
> > tools/bisect-builds.py script to get an accurate regression range: or if the
> > repro is publicly accessible, file a bug, and someone can help bisect it
down
> as
> > well.
> Thank you for your reply. 
> Just one thing, I only talk about frame and frameset, not iframe.
> I'll try to give you a temporary public address of our intranet application if
> necessary.

Yes, please file a bug at https://new.crbug.com/ (with a link or repro steps if
possible) and reply here with the bug number, and we'll take a look.  We are
interested in tracking down the issue you're seeing, but so far it doesn't sound
likely that this is the responsible CL.  Thanks!

[LaurentBarbareau, 2016-09-21 08:17:35]

On 2016/09/20 18:15:30, Charlie Reis (slow) wrote:
> Yes, please file a bug at https://new.crbug.com/ (with a link or repro steps
if
> possible) and reply here with the bug number, and we'll take a look.  We are
> interested in tracking down the issue you're seeing, but so far it doesn't
sound
> likely that this is the responsible CL.  Thanks!

The bug we're facing is confirmed by other ones. Cf.
https://bugs.chromium.org/p/chromium/issues/detail?id=645119&can=2&start=0&nu...

Thx

[Charlie Reis, 2016-09-21 16:59:52]

On 2016/09/21 08:17:35, LaurentBarbareau wrote:
> On 2016/09/20 18:15:30, Charlie Reis (slow) wrote:
> > Yes, please file a bug at https://new.crbug.com/ (with a link or repro steps
> if
> > possible) and reply here with the bug number, and we'll take a look.  We are
> > interested in tracking down the issue you're seeing, but so far it doesn't
> sound
> > likely that this is the responsible CL.  Thanks!
> 
> The bug we're facing is confirmed by other ones. Cf.
>
https://bugs.chromium.org/p/chromium/issues/detail?id=645119&can=2&start=0&nu...
> 
> Thx

Great-- looks like the fix has landed in https://crbug.com/647151 and is being
evaluated for a merge.