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..
Created attachment 134661 [details] patch
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?
(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.
(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.
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.
(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
Comment on attachment 134661 [details] patch Clearing flags on attachment: 134661 Committed r112728: <http://trac.webkit.org/changeset/112728>
All reviewed patches have been landed. Closing bug.