Crash when website does a history.back() followed by an alert()
https://bugs.webkit.org/show_bug.cgi?id=29686
rdar://problem/6984996
Patch by Darin Adler <darin@apple.com> on 2009-09-23
Reviewed by Sam Weinig.
When loading is deferred, we need to defer timer-based loads
too, not just networking-driven loads. Otherwise we can get
syncronouse navigation while running a script, which leads to
crashes and other badness.
This patch includes a manual test; an automated test may be
possible some time in the future.
* dom/Document.cpp:
(WebCore::Document::processHttpEquiv): Use scheduleLocationChange
instead of scheduleHTTPRedirection to implement the navigation
needed for x-frame-options.
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::FrameLoader): Updated for data members with
new names and new data members.
(WebCore::FrameLoader::setDefersLoading): When turning deferral
off, call startRedirectionTimer and startCheckCompleteTimer, since
either of them might have been fired and ignored while defersLoading
was true.
(WebCore::FrameLoader::clear): Updated for replacement of the
m_checkCompletedTimer and m_checkLoadCompleteTimer timers.
(WebCore::FrameLoader::allAncestorsAreComplete): Added.
(WebCore::FrameLoader::checkCompleted): Added code to set
m_shouldCallCheckCompleted to false. Changed code that calls
startRedirectionTimer to call it unconditionally, since that
function now knows when to do work and doesn't expect callers
to handle that any more.
(WebCore::FrameLoader::checkTimerFired): Added. Replaces the old
timer fired callbacks. Calls checkCompleted and checkLoadComplete
as appropriate, but not when defersLoading is true.
(WebCore::FrameLoader::startCheckCompleteTimer): Added. Replaces
the two different calls to start timers before. Only starts the
timers if they are needed.
(WebCore::FrameLoader::scheduleCheckCompleted): Changed to call
startCheckCompleteTimer after setting boolean.
(WebCore::FrameLoader::scheduleCheckLoadComplete): Ditto.
(WebCore::FrameLoader::scheduleHistoryNavigation): Removed
canGoBackOrForward check. The logic works more naturally when
we don't do anything until the timer fires.
(WebCore::FrameLoader::redirectionTimerFired): Do nothing if
defersLoading is true. Also moved canGoBackOrForward check here.
(WebCore::FrameLoader::scheduleRedirection): Changed code that
calls startRedirectionTimer to do so unconditionally. That
function now handles the rules about when to start the timer
rather than expecting the caller to do so.
(WebCore::FrameLoader::startRedirectionTimer): Added code to
handle the case where there is no redirection scheduled,
where the timer is already active, or where this is a classic
redirection and there is an ancestor that has not yet completed
loading.
(WebCore::FrameLoader::completed): Call startRedirectionTimer
here directly instead of calling a cover named parentCompleted.
Hooray! One less function in the giant FrameLoader class!
(WebCore::FrameLoader::checkLoadComplete): Added code to set
m_shouldCallCheckLoadComplete to false.
* loader/FrameLoader.h: Replaced the two functions
checkCompletedTimerFired and checkLoadCompleteTimerFired with
one function, checkTimerFired. Removed the parentCompleted
function. Added the startCheckCompleteTimer and
allAncestorsAreComplete functions. Replaced the
m_checkCompletedTimer and m_checkLoadCompleteTimer data
members with m_checkTimer, m_shouldCallCheckCompleted, and
m_shouldCallCheckLoadComplete.
* manual-tests/go-back-after-alert.html: Added.
* manual-tests/resources/alert-and-go-back.html: Added.
git-svn-id: svn://svn.chromium.org/blink/trunk@48687 bbb929c8-8fbe-4397-9dbb-9b2b20218538
diff --git a/third_party/WebKit/WebCore/ChangeLog b/third_party/WebKit/WebCore/ChangeLog
index a6e5215..2eee0cb 100644
--- a/third_party/WebKit/WebCore/ChangeLog
+++ b/third_party/WebKit/WebCore/ChangeLog
@@ -1,3 +1,80 @@
+2009-09-23 Darin Adler <darin@apple.com>
+
+ Reviewed by Sam Weinig.
+
+ Crash when website does a history.back() followed by an alert()
+ https://bugs.webkit.org/show_bug.cgi?id=29686
+ rdar://problem/6984996
+
+ When loading is deferred, we need to defer timer-based loads
+ too, not just networking-driven loads. Otherwise we can get
+ syncronouse navigation while running a script, which leads to
+ crashes and other badness.
+
+ This patch includes a manual test; an automated test may be
+ possible some time in the future.
+
+ * dom/Document.cpp:
+ (WebCore::Document::processHttpEquiv): Use scheduleLocationChange
+ instead of scheduleHTTPRedirection to implement the navigation
+ needed for x-frame-options.
+
+ * loader/FrameLoader.cpp:
+ (WebCore::FrameLoader::FrameLoader): Updated for data members with
+ new names and new data members.
+ (WebCore::FrameLoader::setDefersLoading): When turning deferral
+ off, call startRedirectionTimer and startCheckCompleteTimer, since
+ either of them might have been fired and ignored while defersLoading
+ was true.
+ (WebCore::FrameLoader::clear): Updated for replacement of the
+ m_checkCompletedTimer and m_checkLoadCompleteTimer timers.
+ (WebCore::FrameLoader::allAncestorsAreComplete): Added.
+ (WebCore::FrameLoader::checkCompleted): Added code to set
+ m_shouldCallCheckCompleted to false. Changed code that calls
+ startRedirectionTimer to call it unconditionally, since that
+ function now knows when to do work and doesn't expect callers
+ to handle that any more.
+ (WebCore::FrameLoader::checkTimerFired): Added. Replaces the old
+ timer fired callbacks. Calls checkCompleted and checkLoadComplete
+ as appropriate, but not when defersLoading is true.
+ (WebCore::FrameLoader::startCheckCompleteTimer): Added. Replaces
+ the two different calls to start timers before. Only starts the
+ timers if they are needed.
+ (WebCore::FrameLoader::scheduleCheckCompleted): Changed to call
+ startCheckCompleteTimer after setting boolean.
+ (WebCore::FrameLoader::scheduleCheckLoadComplete): Ditto.
+ (WebCore::FrameLoader::scheduleHistoryNavigation): Removed
+ canGoBackOrForward check. The logic works more naturally when
+ we don't do anything until the timer fires.
+ (WebCore::FrameLoader::redirectionTimerFired): Do nothing if
+ defersLoading is true. Also moved canGoBackOrForward check here.
+ (WebCore::FrameLoader::scheduleRedirection): Changed code that
+ calls startRedirectionTimer to do so unconditionally. That
+ function now handles the rules about when to start the timer
+ rather than expecting the caller to do so.
+ (WebCore::FrameLoader::startRedirectionTimer): Added code to
+ handle the case where there is no redirection scheduled,
+ where the timer is already active, or where this is a classic
+ redirection and there is an ancestor that has not yet completed
+ loading.
+ (WebCore::FrameLoader::completed): Call startRedirectionTimer
+ here directly instead of calling a cover named parentCompleted.
+ Hooray! One less function in the giant FrameLoader class!
+ (WebCore::FrameLoader::checkLoadComplete): Added code to set
+ m_shouldCallCheckLoadComplete to false.
+
+ * loader/FrameLoader.h: Replaced the two functions
+ checkCompletedTimerFired and checkLoadCompleteTimerFired with
+ one function, checkTimerFired. Removed the parentCompleted
+ function. Added the startCheckCompleteTimer and
+ allAncestorsAreComplete functions. Replaced the
+ m_checkCompletedTimer and m_checkLoadCompleteTimer data
+ members with m_checkTimer, m_shouldCallCheckCompleted, and
+ m_shouldCallCheckLoadComplete.
+
+ * manual-tests/go-back-after-alert.html: Added.
+ * manual-tests/resources/alert-and-go-back.html: Added.
+
2009-09-23 David Kilzer <ddkilzer@apple.com>
<http://webkit.org/b/29660> Move "Generate 64-bit Export File" build phase script into DerivedSources.make
diff --git a/third_party/WebKit/WebCore/dom/Document.cpp b/third_party/WebKit/WebCore/dom/Document.cpp
index f26ee87..57e22e1 100644
--- a/third_party/WebKit/WebCore/dom/Document.cpp
+++ b/third_party/WebKit/WebCore/dom/Document.cpp
@@ -2173,7 +2173,7 @@
FrameLoader* frameLoader = frame->loader();
if (frameLoader->shouldInterruptLoadForXFrameOptions(content, url())) {
frameLoader->stopAllLoaders();
- frameLoader->scheduleHTTPRedirection(0, blankURL());
+ frameLoader->scheduleLocationChange(blankURL(), String());
}
}
}
diff --git a/third_party/WebKit/WebCore/loader/FrameLoader.cpp b/third_party/WebKit/WebCore/loader/FrameLoader.cpp
index 52b6d77..f48c607 100644
--- a/third_party/WebKit/WebCore/loader/FrameLoader.cpp
+++ b/third_party/WebKit/WebCore/loader/FrameLoader.cpp
@@ -272,8 +272,9 @@
, m_encodingWasChosenByUser(false)
, m_containsPlugIns(false)
, m_redirectionTimer(this, &FrameLoader::redirectionTimerFired)
- , m_checkCompletedTimer(this, &FrameLoader::checkCompletedTimerFired)
- , m_checkLoadCompleteTimer(this, &FrameLoader::checkLoadCompleteTimerFired)
+ , m_checkTimer(this, &FrameLoader::checkTimerFired)
+ , m_shouldCallCheckCompleted(false)
+ , m_shouldCallCheckLoadComplete(false)
, m_opener(0)
, m_openedByDOM(false)
, m_creatingInitialEmptyDocument(false)
@@ -323,6 +324,11 @@
m_provisionalDocumentLoader->setDefersLoading(defers);
if (m_policyDocumentLoader)
m_policyDocumentLoader->setDefersLoading(defers);
+
+ if (!defers) {
+ startRedirectionTimer();
+ startCheckCompleteTimer();
+ }
}
Frame* FrameLoader::createWindow(FrameLoader* frameLoaderForFrameLookup, const FrameLoadRequest& request, const WindowFeatures& features, bool& created)
@@ -849,8 +855,9 @@
m_redirectionTimer.stop();
m_scheduledRedirection.clear();
- m_checkCompletedTimer.stop();
- m_checkLoadCompleteTimer.stop();
+ m_checkTimer.stop();
+ m_shouldCallCheckCompleted = false;
+ m_shouldCallCheckLoadComplete = false;
m_receivedData = false;
m_isDisplayingInitialEmptyDocument = false;
@@ -1252,8 +1259,19 @@
return true;
}
+bool FrameLoader::allAncestorsAreComplete() const
+{
+ for (Frame* ancestor = m_frame; ancestor; ancestor = ancestor->tree()->parent()) {
+ if (!ancestor->loader()->m_isComplete)
+ return false;
+ }
+ return true;
+}
+
void FrameLoader::checkCompleted()
{
+ m_shouldCallCheckCompleted = false;
+
if (m_frame->view())
m_frame->view()->checkStopDelayingDeferredRepaints();
@@ -1279,38 +1297,44 @@
RefPtr<Frame> protect(m_frame);
checkCallImplicitClose(); // if we didn't do it before
- // Do not start a redirection timer for subframes here.
- // That is deferred until the parent is completed.
- if (m_scheduledRedirection && !m_frame->tree()->parent())
- startRedirectionTimer();
+ startRedirectionTimer();
completed();
if (m_frame->page())
checkLoadComplete();
}
-void FrameLoader::checkCompletedTimerFired(Timer<FrameLoader>*)
+void FrameLoader::checkTimerFired(Timer<FrameLoader>*)
{
- checkCompleted();
+ if (Page* page = m_frame->page()) {
+ if (page->defersLoading())
+ return;
+ }
+ if (m_shouldCallCheckCompleted)
+ checkCompleted();
+ if (m_shouldCallCheckLoadComplete)
+ checkLoadComplete();
+}
+
+void FrameLoader::startCheckCompleteTimer()
+{
+ if (!(m_shouldCallCheckCompleted || m_shouldCallCheckLoadComplete))
+ return;
+ if (m_checkTimer.isActive())
+ return;
+ m_checkTimer.startOneShot(0);
}
void FrameLoader::scheduleCheckCompleted()
{
- if (!m_checkCompletedTimer.isActive())
- m_checkCompletedTimer.startOneShot(0);
-}
-
-void FrameLoader::checkLoadCompleteTimerFired(Timer<FrameLoader>*)
-{
- if (!m_frame->page())
- return;
- checkLoadComplete();
+ m_shouldCallCheckCompleted = true;
+ startCheckCompleteTimer();
}
void FrameLoader::scheduleCheckLoadComplete()
{
- if (!m_checkLoadCompleteTimer.isActive())
- m_checkLoadCompleteTimer.startOneShot(0);
+ m_shouldCallCheckLoadComplete = true;
+ startCheckCompleteTimer();
}
void FrameLoader::checkCallImplicitClose()
@@ -1439,12 +1463,6 @@
if (!m_frame->page())
return;
- // navigation will always be allowed in the 0 steps case, which is OK because that's supposed to force a reload.
- if (!canGoBackOrForward(steps)) {
- cancelRedirection();
- return;
- }
-
scheduleRedirection(new ScheduledRedirection(steps));
}
@@ -1482,6 +1500,9 @@
{
ASSERT(m_frame->page());
+ if (m_frame->page()->defersLoading())
+ return;
+
OwnPtr<ScheduledRedirection> redirection(m_scheduledRedirection.release());
switch (redirection->type) {
@@ -1498,7 +1519,8 @@
}
// go(i!=0) from a frame navigates into the history of the frame only,
// in both IE and NS (but not in Mozilla). We can't easily do that.
- goBackOrForward(redirection->historySteps);
+ if (canGoBackOrForward(redirection->historySteps))
+ goBackOrForward(redirection->historySteps);
return;
case ScheduledRedirection::formSubmission:
// The submitForm function will find a target frame before using the redirection timer.
@@ -1722,12 +1744,6 @@
return widget != 0;
}
-void FrameLoader::parentCompleted()
-{
- if (m_scheduledRedirection && !m_redirectionTimer.isActive())
- startRedirectionTimer();
-}
-
String FrameLoader::outgoingReferrer() const
{
return m_outgoingReferrer;
@@ -2139,16 +2155,22 @@
m_scheduledRedirection = redirection;
if (!m_isComplete && m_scheduledRedirection->type != ScheduledRedirection::redirection)
completed();
- if (m_isComplete || m_scheduledRedirection->type != ScheduledRedirection::redirection)
- startRedirectionTimer();
+ startRedirectionTimer();
}
void FrameLoader::startRedirectionTimer()
{
- ASSERT(m_frame->page());
- ASSERT(m_scheduledRedirection);
+ if (!m_scheduledRedirection)
+ return;
- m_redirectionTimer.stop();
+ ASSERT(m_frame->page());
+
+ if (m_redirectionTimer.isActive())
+ return;
+
+ if (m_scheduledRedirection->type == ScheduledRedirection::redirection && !allAncestorsAreComplete())
+ return;
+
m_redirectionTimer.startOneShot(m_scheduledRedirection->delay);
switch (m_scheduledRedirection->type) {
@@ -2187,7 +2209,7 @@
{
RefPtr<Frame> protect(m_frame);
for (Frame* child = m_frame->tree()->firstChild(); child; child = child->tree()->nextSibling())
- child->loader()->parentCompleted();
+ child->loader()->startRedirectionTimer();
if (Frame* parent = m_frame->tree()->parent())
parent->loader()->checkCompleted();
if (m_frame->view())
@@ -3559,6 +3581,8 @@
{
ASSERT(m_client->hasWebView());
+ m_shouldCallCheckLoadComplete = false;
+
// FIXME: Always traversing the entire frame tree is a bit inefficient, but
// is currently needed in order to null out the previous history item for all frames.
if (Page* page = m_frame->page())
diff --git a/third_party/WebKit/WebCore/loader/FrameLoader.h b/third_party/WebKit/WebCore/loader/FrameLoader.h
index f66e7d2..4b4959b 100644
--- a/third_party/WebKit/WebCore/loader/FrameLoader.h
+++ b/third_party/WebKit/WebCore/loader/FrameLoader.h
@@ -412,15 +412,13 @@
void updateHistoryForAnchorScroll();
void redirectionTimerFired(Timer<FrameLoader>*);
- void checkCompletedTimerFired(Timer<FrameLoader>*);
- void checkLoadCompleteTimerFired(Timer<FrameLoader>*);
+ void checkTimerFired(Timer<FrameLoader>*);
void cancelRedirection(bool newLoadInProgress = false);
void started();
void completed();
- void parentCompleted();
bool shouldUsePlugin(const KURL&, const String& mimeType, bool hasFallback, bool& useFallback);
bool loadPlugin(RenderPart*, const KURL&, const String& mimeType,
@@ -539,6 +537,7 @@
void scheduleCheckCompleted();
void scheduleCheckLoadComplete();
+ void startCheckCompleteTimer();
KURL originalRequestURL() const;
@@ -546,6 +545,7 @@
void saveScrollPositionAndViewStateToItem(HistoryItem*);
+ bool allAncestorsAreComplete() const; // including this
bool allChildrenAreComplete() const; // immediate children, not all descendants
Frame* m_frame;
@@ -612,8 +612,9 @@
KURL m_submittedFormURL;
Timer<FrameLoader> m_redirectionTimer;
- Timer<FrameLoader> m_checkCompletedTimer;
- Timer<FrameLoader> m_checkLoadCompleteTimer;
+ Timer<FrameLoader> m_checkTimer;
+ bool m_shouldCallCheckCompleted;
+ bool m_shouldCallCheckLoadComplete;
Frame* m_opener;
HashSet<Frame*> m_openedFrames;
diff --git a/third_party/WebKit/WebCore/manual-tests/go-back-after-alert.html b/third_party/WebKit/WebCore/manual-tests/go-back-after-alert.html
new file mode 100644
index 0000000..b3a1e1a5
--- /dev/null
+++ b/third_party/WebKit/WebCore/manual-tests/go-back-after-alert.html
@@ -0,0 +1,7 @@
+<html>
+<body>
+This tests a bug where going back just before putting up an alert can lead to a crash.
+<hr>
+<a href="resources/alert-and-go-back.html">Click this link to run the test.</a>
+</body>
+</html>
diff --git a/third_party/WebKit/WebCore/manual-tests/resources/alert-and-go-back.html b/third_party/WebKit/WebCore/manual-tests/resources/alert-and-go-back.html
new file mode 100644
index 0000000..55a5b50
--- /dev/null
+++ b/third_party/WebKit/WebCore/manual-tests/resources/alert-and-go-back.html
@@ -0,0 +1,4 @@
+<script>
+history.back();
+alert("Wait a moment and then dismiss this alert. If there is no crash, the test succeeded.");
+</script>
