Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(139)

Unified Diff: third_party/WebKit/Source/core/loader/FrameLoader.cpp

Issue 1714063002: Protect the provisional loader from detaching during prepareForCommit (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Added a layout test, fixed bugs with initial fix Created 4 years, 10 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « third_party/WebKit/Source/core/loader/FrameLoader.h ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: third_party/WebKit/Source/core/loader/FrameLoader.cpp
diff --git a/third_party/WebKit/Source/core/loader/FrameLoader.cpp b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
index 038cf60a8afc2b21412fab348d49efb7f0661d53..82f525465393b389980b2b71d1d7c81cadaef6a2 100644
--- a/third_party/WebKit/Source/core/loader/FrameLoader.cpp
+++ b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
@@ -168,6 +168,7 @@ FrameLoader::FrameLoader(LocalFrame* frame)
, m_didAccessInitialDocumentTimer(this, &FrameLoader::didAccessInitialDocumentTimerFired)
, m_forcedSandboxFlags(SandboxNone)
, m_dispatchingDidClearWindowObjectInMainWorld(false)
+ , m_protectProvisionalLoader(false)
{
}
@@ -253,6 +254,9 @@ void FrameLoader::saveScrollState()
void FrameLoader::dispatchUnloadEvent()
{
+ // If the frame is unloading, the provisional loader should no longer be
+ // protected. It will be detached soon.
+ m_protectProvisionalLoader = false;
saveScrollState();
if (m_frame->document() && !SVGImage::isInSVGImage(m_frame->document()))
@@ -992,13 +996,16 @@ void FrameLoader::stopAllLoaders()
}
m_frame->document()->suppressLoadEvent();
- if (m_provisionalDocumentLoader)
+ // Don't stop loading the provisional loader if it is being protected (i.e.
+ // it is about to be committed) See prepareForCommit() for more details.
+ if (m_provisionalDocumentLoader && !m_protectProvisionalLoader)
m_provisionalDocumentLoader->stopLoading();
if (m_documentLoader)
m_documentLoader->stopLoading();
m_frame->document()->cancelParsing();
- detachDocumentLoader(m_provisionalDocumentLoader);
+ if (!m_protectProvisionalLoader)
dcheng 2016/02/22 18:12:07 Would it be slightly more robust to put this check
Charlie Harrison 2016/02/24 17:09:15 I'm not sure. My first attempt is giving me crashe
dcheng 2016/02/24 21:22:59 Hmm, mind posting a stack on here? I'm kind of cur
+ detachDocumentLoader(m_provisionalDocumentLoader);
m_checkTimer.stop();
m_frame->navigationScheduler().cancel();
@@ -1062,17 +1069,22 @@ bool FrameLoader::prepareForCommit()
// we need to abandon the current load.
if (pdl != m_provisionalDocumentLoader)
return false;
+ // detachFromFrame() will abort XHRs that haven't completed, which can
+ // trigger event listeners for 'abort'. These event listeners might call
+ // stop(), which will in turn detach the provisional document loader.
dcheng 2016/02/22 18:12:07 Nit: window.stop()
Charlie Harrison 2016/02/24 17:09:15 Done.
+ // At this point, the provisional document loader should not detach, because
+ // then the FrameLoader would not have any attached DocumentLoaders.
if (m_documentLoader) {
FrameNavigationDisabler navigationDisabler(*m_frame);
+ TemporaryChange<bool> inDetachDocumentLoader(m_protectProvisionalLoader, true);
detachDocumentLoader(m_documentLoader);
}
- // detachFromFrame() will abort XHRs that haven't completed, which can
- // trigger event listeners for 'abort'. These event listeners might detach
- // the frame.
+ // 'abort' listeners can also detach the frame.
// TODO(dcheng): Investigate if this can be moved above the check that
dcheng 2016/02/22 18:12:07 Nit: clean up this TODO as well, it doesn't really
Charlie Harrison 2016/02/24 17:09:15 Done.
// m_provisionalDocumentLoader hasn't changed.
if (!m_frame->client())
return false;
+ ASSERT(m_provisionalDocumentLoader == pdl);
// No more events will be dispatched so detach the Document.
// TODO(yoav): Should we also be nullifying domWindow's document (or domWindow) since the doc is now detached?
if (m_frame->document())
« no previous file with comments | « third_party/WebKit/Source/core/loader/FrameLoader.h ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698