Bug 82653 – Merge FrameLoader::finishedLoading() into DocumentLoader::finishedLoading()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

FrameLoader::finishedLoading(): Calls a bunch of stuff on DocumentLoader (including finishedLoading), then calls FrameLoader::checkLoadComplete.  There's no reason for this division, so long as we're careful to handle the fact that DocumentLoader::finishedLoading() is called by FrameLoader::init(), when things aren't yet in a fully consistent state..

[Nate Chapin]

Created attachment 134661 [details]
patch

[Adam Barth]

Comment on attachment 134661 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134661&action=review

> Source/WebCore/loader/FrameLoader.cpp:-1979
> -    // Retain because the stop may release the last reference to it.
> -    RefPtr<Frame> protect(m_frame);

Do we not need this protect variable?

[Nate Chapin]

(In reply to comment #2)
> (From update of attachment 134661 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134661&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:-1979
> > -    // Retain because the stop may release the last reference to it.
> > -    RefPtr<Frame> protect(m_frame);
> 
> Do we not need this protect variable?

Nothing obviously broke, but I don't have proof that we don't need it. Will look more before committing.

[Nate Chapin]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 134661 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=134661&action=review
> > 
> > > Source/WebCore/loader/FrameLoader.cpp:-1979
> > > -    // Retain because the stop may release the last reference to it.
> > > -    RefPtr<Frame> protect(m_frame);
> > 
> > Do we not need this protect variable?
> 
> Nothing obviously broke, but I don't have proof that we don't need it. Will look more before committing.

I'm not sure there's a good way to tell whether it's necessary. Nothing in the callstack below is protecting the Frame, and DocumentLoader::m_frame is a raw pointer. On the other hand, I don't want to just cargo-cult it, especially when the comment uses a term as vague as "the stop" and was added in r17652.

[Adam Barth]

Comment on attachment 134661 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134661&action=review

>>>> Source/WebCore/loader/FrameLoader.cpp:-1979
>>>> -    RefPtr<Frame> protect(m_frame);
>>> 
>>> Do we not need this protect variable?
>> 
>> Nothing obviously broke, but I don't have proof that we don't need it. Will look more before committing.
> 
> I'm not sure there's a good way to tell whether it's necessary. Nothing in the callstack below is protecting the Frame, and DocumentLoader::m_frame is a raw pointer. On the other hand, I don't want to just cargo-cult it, especially when the comment uses a term as vague as "the stop" and was added in r17652.

checkLoadComplete() triggers the load event, right?  We must have a test that rips and iframe out of the DOM when the load event fires.  If this was necessary, we'd run into trouble on that test.  I'm inclined to think it's not necessary, but I'm not sure how to become completely convinced.

[Nate Chapin]

(In reply to comment #5)
> (From update of attachment 134661 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134661&action=review
> 
> >>>> Source/WebCore/loader/FrameLoader.cpp:-1979
> >>>> -    RefPtr<Frame> protect(m_frame);
> >>> 
> >>> Do we not need this protect variable?
> >> 
> >> Nothing obviously broke, but I don't have proof that we don't need it. Will look more before committing.
> > 
> > I'm not sure there's a good way to tell whether it's necessary. Nothing in the callstack below is protecting the Frame, and DocumentLoader::m_frame is a raw pointer. On the other hand, I don't want to just cargo-cult it, especially when the comment uses a term as vague as "the stop" and was added in r17652.
> 
> checkLoadComplete() triggers the load event, right?  We must have a test that rips and iframe out of the DOM when the load event fires.  If this was necessary, we'd run into trouble on that test.  I'm inclined to think it's not necessary, but I'm not sure how to become completely convinced.

Ok, I think I'm convinced.

1. DocumentLoader is protected in MainResourceLoader::didFinishLoading(), so we don't need to worry about it.
2. The load event is fired from within m_writer.end() in DocumentLoader::finishedLoading().
2a. Inside DocumentWrtier::end(), the callstack passes through the parser, then calls Document::finishedParsing(), which protects the Frame until it exits.
2b. Nothing happens between exiting finishedParsing() and exiting DocumentWriter::end() except nulling the parser.
3. Suppose that the Frame was detached during the load event. We therefore called FrameLoader::detachFromParent(), which called DocumentLoader::detachFromFrame(), which called DocumentLoader::stopLoading().
4. stopLoading() called DocumentLoader::setMainResourceError() either directly, through mainReceivedError() or through MainResourceLoader::cancel().
5. Therefore, when control returns to DocumentLoader::finishedLoading(), though m_frame is null, m_mainResourceError is now non-null. We exit finishedLoading() before dereferencing m_frame again.

...and indeed, if I reverse the 2 clauses in "if (!m_mainDocumentError.isNull() || frameLoader()->stateMachine()->creatingInitialEmptyDocument())", I get a null Frame* deref on, among others, fast/dom/Document/document-close-iframe-load.html

[WebKit Review Bot]

Comment on attachment 134661 [details]
patch

Clearing flags on attachment: 134661

Committed r112728: <http://trac.webkit.org/changeset/112728>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.