Speculative fix for null m_documentLoader deref in FrameLoader::loadInSameDocument

Speculative fix for null m_documentLoader deref in FrameLoader::loadInSameDocument

This should fix an uncommon crash during history traversal.

I believe the steps to get into this state are:
1. Load a page with a slow-loading iframe. The iframe load must begin before the main frame's load event fires.
2. While the iframe is still in the provisional load state, attempt a same-document history navigation in the child frame.
3. The child frame's provisional load is cancelled by the history navigation in FrameLoader::loadInSameDocument, which in turn causes the parent frame's load event to fire synchronously.
4. The parent frame's onload event handler detaches the iframe.
5. No checks are performed after cancelling the provisional load in FrameLoader::loadInSameDocument, leading to a null deref and crash.

We should be able to prevent a crash in this case by checking whether the frame is still attached after cancelling the provisional load in FrameLoader::loadInSameDocument.

BUG=374391
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175174

[Nate Chapin, 2014-05-29 22:37:15]

No test, since I have no idea how to repro :(

[dcheng, 2014-05-29 22:43:43]

https://codereview.chromium.org/303133004/diff/1/Source/core/loader/FrameLoad...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/303133004/diff/1/Source/core/loader/FrameLoad...
Source/core/loader/FrameLoader.cpp:1398: // See detachClient().
I don't quite understand this comment. Do we check that m_frame->page() is not
null somewhere before we call this?

[Nate Chapin, 2014-05-29 22:57:00]

On 2014/05/29 22:43:43, dcheng wrote:
>
https://codereview.chromium.org/303133004/diff/1/Source/core/loader/FrameLoad...
> File Source/core/loader/FrameLoader.cpp (right):
> 
>
https://codereview.chromium.org/303133004/diff/1/Source/core/loader/FrameLoad...
> Source/core/loader/FrameLoader.cpp:1398: // See detachClient().
> I don't quite understand this comment. Do we check that m_frame->page() is not
> null somewhere before we call this?

Actually, now that I look at it, my theory seems bogus. Let me look some more.

[Nate Chapin, 2014-05-29 23:37:58]

On 2014/05/29 22:57:00, Nate Chapin wrote:
> On 2014/05/29 22:43:43, dcheng wrote:
> >
>
https://codereview.chromium.org/303133004/diff/1/Source/core/loader/FrameLoad...
> > File Source/core/loader/FrameLoader.cpp (right):
> > 
> >
>
https://codereview.chromium.org/303133004/diff/1/Source/core/loader/FrameLoad...
> > Source/core/loader/FrameLoader.cpp:1398: // See detachClient().
> > I don't quite understand this comment. Do we check that m_frame->page() is
not
> > null somewhere before we call this?
> 
> Actually, now that I look at it, my theory seems bogus. Let me look some more.

Uploaded a new attempted fix + renamed the issue to indicate that I'm not sure
it actually fixes the problem.

This case should be possible if we attempt a same-document history navigation in
an iframe that has a provisional load in progress. The history navigation causes
the provisional load to be cancelled. If this in turn triggers a js event that
detaches the iframe, we could see a null m_documentLoader in this location, so
check whether the frame is still attached before proceeding.

Unfortunately, I'm not sure how to get either a layout test or a unit test into
this state.

[dcheng, 2014-05-30 20:54:30]

On 2014/05/29 23:37:58, Nate Chapin wrote:
> On 2014/05/29 22:57:00, Nate Chapin wrote:
> > On 2014/05/29 22:43:43, dcheng wrote:
> > >
> >
>
https://codereview.chromium.org/303133004/diff/1/Source/core/loader/FrameLoad...
> > > File Source/core/loader/FrameLoader.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/303133004/diff/1/Source/core/loader/FrameLoad...
> > > Source/core/loader/FrameLoader.cpp:1398: // See detachClient().
> > > I don't quite understand this comment. Do we check that m_frame->page() is
> not
> > > null somewhere before we call this?
> > 
> > Actually, now that I look at it, my theory seems bogus. Let me look some
more.
> 
> Uploaded a new attempted fix + renamed the issue to indicate that I'm not sure
> it actually fixes the problem.
> 
> This case should be possible if we attempt a same-document history navigation
in
> an iframe that has a provisional load in progress. The history navigation
causes
> the provisional load to be cancelled. If this in turn triggers a js event that
> detaches the iframe, we could see a null m_documentLoader in this location, so
> check whether the frame is still attached before proceeding.
> 
> Unfortunately, I'm not sure how to get either a layout test or a unit test
into
> this state.

It should be possible to set this up in a unit test, right? Something like this:

m_helper.initializeAndLoad("page-with-frame.html");
/* Do a same-document navigation in the frame--I guess an anchor navigation
should do this? */
FrameTestHelpers::loadFrame(mainFrame,
"javascript:document.getElementById('frame').src='some-page.html'");
childFrame->loadHistoryItem(...);
FrameTestHelpers::pumpPendingRequests();

[Nate Chapin, 2014-05-30 20:59:59]

On 2014/05/30 20:54:30, dcheng wrote:
> On 2014/05/29 23:37:58, Nate Chapin wrote:
> > On 2014/05/29 22:57:00, Nate Chapin wrote:
> > > On 2014/05/29 22:43:43, dcheng wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/303133004/diff/1/Source/core/loader/FrameLoad...
> > > > File Source/core/loader/FrameLoader.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/303133004/diff/1/Source/core/loader/FrameLoad...
> > > > Source/core/loader/FrameLoader.cpp:1398: // See detachClient().
> > > > I don't quite understand this comment. Do we check that m_frame->page()
is
> > not
> > > > null somewhere before we call this?
> > > 
> > > Actually, now that I look at it, my theory seems bogus. Let me look some
> more.
> > 
> > Uploaded a new attempted fix + renamed the issue to indicate that I'm not
sure
> > it actually fixes the problem.
> > 
> > This case should be possible if we attempt a same-document history
navigation
> in
> > an iframe that has a provisional load in progress. The history navigation
> causes
> > the provisional load to be cancelled. If this in turn triggers a js event
that
> > detaches the iframe, we could see a null m_documentLoader in this location,
so
> > check whether the frame is still attached before proceeding.
> > 
> > Unfortunately, I'm not sure how to get either a layout test or a unit test
> into
> > this state.
> 
> It should be possible to set this up in a unit test, right? Something like
this:
> 
> m_helper.initializeAndLoad("page-with-frame.html");
> /* Do a same-document navigation in the frame--I guess an anchor navigation
> should do this? */
> FrameTestHelpers::loadFrame(mainFrame,
> "javascript:document.getElementById('frame').src='some-page.html'");
> childFrame->loadHistoryItem(...);
> FrameTestHelpers::pumpPendingRequests();

The hard part is finding an event that can fire when the some-page.html load
gets cancelled. I think that has to be the main frame's load event, but getting
into a state where some-page.html can be slow and get cancelled before the
parent frame's load event fires is what's stumping me.

[dcheng, 2014-05-30 21:19:49]

I'd love a test, but if we can't figure out a good way, that's OK.

Do you mind updating the CL description of how we can get in this state?

[Nate Chapin, 2014-05-30 21:46:09]

On 2014/05/30 21:19:49, dcheng wrote:
> I'd love a test, but if we can't figure out a good way, that's OK.
> 
> Do you mind updating the CL description of how we can get in this state?

I've tried hooking some WebFrameClient callbacks that aren't supposed to be
starting loads, but that causing us to crash due to violated internal
assumptions. I'm out of ideas.

Description updated.

[dcheng, 2014-05-30 21:52:26]

LGTM.

It's pretty non-intuitive that DocumentLoader::stopLoading may end up trigger a
load event to fire =(

[dcheng, 2014-05-30 21:52:48]

On 2014/05/30 21:52:26, dcheng wrote:
> LGTM.
> 
> It's pretty non-intuitive that DocumentLoader::stopLoading may end up trigger
a
> load event to fire =(

(Perhaps add a comment? Not sure if that will help or hurt in the long run
though)

[Nate Chapin, 2014-05-30 21:55:51]

On 2014/05/30 21:52:48, dcheng wrote:
> On 2014/05/30 21:52:26, dcheng wrote:
> > LGTM.
> > 
> > It's pretty non-intuitive that DocumentLoader::stopLoading may end up
trigger
> a
> > load event to fire =(
> 
> (Perhaps add a comment? Not sure if that will help or hurt in the long run
> though)

Yeah, idk. As a general rule, anything you do in the loader should be presumed
capable of destroying the world, so I'm a little hesitant to add a comment
everywhere that's true. :(

stopAllLoaders() has a comparable warning in
http://src.chromium.org/viewvc/blink/trunk/Source/core/loader/FrameLoader.h,
since it kind of resembles a public API

[Nate Chapin, 2014-05-30 21:56:52]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-05-30 21:59:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/japhet@chromium.org/303133004/40001

[commit-bot: I haz the power, 2014-05-30 23:53:15]

Change committed as 175174