Bug 669671 - When navigating to a history entry created via pushState or touched by replaceState, we should not force the load from cache. r=bz
authorJustin Lebar <justin.lebar@gmail.com>
Thu, 07 Jul 2011 09:12:14 -0400
changeset 72478 470cfda4dd83e6df5ea40ce283accb51e317afc8
parent 72477 89ef5bf1e3d2f3af936e9a3ba49f95a2f4e26afd
child 72479 bc91728e5a7c0503cf07fc07aa6b2f39bb2e41b7
push id447
push userjlebar@mozilla.com
push dateThu, 07 Jul 2011 22:02:52 +0000
treeherdermozilla-inbound@470cfda4dd83 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz
bugs669671
milestone8.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 669671 - When navigating to a history entry created via pushState or touched by replaceState, we should not force the load from cache. r=bz
docshell/base/nsDocShell.cpp
docshell/shistory/public/nsISHEntry.idl
docshell/shistory/src/nsSHEntry.cpp
docshell/shistory/src/nsSHEntry.h
docshell/test/Makefile.in
docshell/test/file_bug669671.sjs
docshell/test/test_bug669671.html
--- a/docshell/base/nsDocShell.cpp
+++ b/docshell/base/nsDocShell.cpp
@@ -9187,17 +9187,27 @@ nsresult nsDocShell::DoChannelLoad(nsICh
     nsLoadFlags loadFlags = 0;
     (void) aChannel->GetLoadFlags(&loadFlags);
     loadFlags |= nsIChannel::LOAD_DOCUMENT_URI |
                  nsIChannel::LOAD_CALL_CONTENT_SNIFFERS;
 
     // Load attributes depend on load type...
     switch (mLoadType) {
     case LOAD_HISTORY:
-        loadFlags |= nsIRequest::VALIDATE_NEVER;
+        {
+            // Only send VALIDATE_NEVER if mLSHE's URI was never changed via
+            // push/replaceState (bug 669671).
+            PRBool uriModified = PR_FALSE;
+            if (mLSHE) {
+                mLSHE->GetURIWasModified(&uriModified);
+            }
+
+            if (!uriModified)
+                loadFlags |= nsIRequest::VALIDATE_NEVER;
+        }
         break;
 
     case LOAD_RELOAD_CHARSET_CHANGE:
         loadFlags |= nsIRequest::LOAD_FROM_CACHE;
         break;
     
     case LOAD_RELOAD_NORMAL:
     case LOAD_REFRESH:
@@ -9849,16 +9859,25 @@ nsDocShell::AddState(nsIVariant *aData, 
         newSHEntry->SetURI(newURI);
     }
 
     // Step 4: Modify new/original session history entry and clear its POST
     // data, if there is any.
     newSHEntry->SetStateData(scContainer);
     newSHEntry->SetPostData(nsnull);
 
+    // If this push/replaceState changed the document's current URI and the new
+    // URI differs from the old URI in more than the hash, or if the old
+    // SHEntry's URI was modified in this way by a push/replaceState call
+    // set URIWasModified to true for the current SHEntry (bug 669671).
+    PRBool sameExceptHashes = PR_TRUE, oldURIWasModified = PR_FALSE;
+    newURI->EqualsExceptRef(mCurrentURI, &sameExceptHashes);
+    oldOSHE->GetURIWasModified(&oldURIWasModified);
+    newSHEntry->SetURIWasModified(!sameExceptHashes || oldURIWasModified);
+
     // Step 5: If aReplace is false, indicating that we're doing a pushState
     // rather than a replaceState, notify bfcache that we've added a page to
     // the history so it can evict content viewers if appropriate.
     if (!aReplace) {
         nsCOMPtr<nsISHistory> rootSH;
         GetRootSessionHistory(getter_AddRefs(rootSH));
         NS_ENSURE_TRUE(rootSH, NS_ERROR_UNEXPECTED);
 
--- a/docshell/shistory/public/nsISHEntry.idl
+++ b/docshell/shistory/public/nsISHEntry.idl
@@ -54,17 +54,17 @@ interface nsIStructuredCloneContainer;
 %{C++
 struct nsIntRect;
 class nsDocShellEditorData;
 %}
 [ref] native nsIntRect(nsIntRect);
 [ptr] native nsDocShellEditorDataPtr(nsDocShellEditorData);
 
 
-[scriptable, uuid(5f3ebf43-6944-45fb-b1b1-78a05bf9370b)]
+[scriptable, uuid(b92d403e-f5ec-4b81-b0e3-6e6c241cef2d)]
 interface nsISHEntry : nsIHistoryEntry
 {
     /** URI for the document */
     void setURI(in nsIURI aURI);
 
     /** Referrer URI */
     attribute nsIURI referrerURI;
 
@@ -164,16 +164,29 @@ interface nsISHEntry : nsIHistoryEntry
     /** attribute to indicate whether the page is already expired in cache */
     attribute boolean expirationStatus;
 
     /**
      * attribute to indicate the content-type of the document that this
      * is a session history entry for
      */
     attribute ACString contentType; 
+
+    /**
+     * If we created this SHEntry via history.pushState or modified it via
+     * history.replaceState, and if we changed the SHEntry's URI via the
+     * push/replaceState call, and if the SHEntry's new URI differs from its
+     * old URI by more than just the hash, then we set this field to true.
+     *
+     * Additionally, if this SHEntry was created by calling pushState from a
+     * SHEntry whose URI was modified, this SHEntry's URIWasModified field is
+     * true.
+     *
+     */
+    attribute boolean URIWasModified;
  
     /** Set/Get scrollers' positon in anchored pages */
     void setScrollPosition(in long x, in long y);
     void getScrollPosition(out long x, out long y);
 
     /** Additional ways to create an entry */
     [noscript] void create(in nsIURI URI, in AString title,
                            in nsIInputStream inputStream,
--- a/docshell/shistory/src/nsSHEntry.cpp
+++ b/docshell/shistory/src/nsSHEntry.cpp
@@ -103,16 +103,17 @@ static void StopTrackingEntry(nsSHEntry 
 
 
 nsSHEntry::nsSHEntry() 
   : mLoadType(0)
   , mID(gEntryID++)
   , mDocIdentifier(gEntryDocIdentifier++)
   , mScrollPositionX(0)
   , mScrollPositionY(0)
+  , mURIWasModified(PR_FALSE)
   , mIsFrameNavigation(PR_FALSE)
   , mSaveLayoutState(PR_TRUE)
   , mExpired(PR_FALSE)
   , mSticky(PR_TRUE)
   , mDynamicallyCreated(PR_FALSE)
   , mParent(nsnull)
   , mViewerBounds(0, 0, 0, 0)
   , mDocShellID(0)
@@ -127,16 +128,17 @@ nsSHEntry::nsSHEntry(const nsSHEntry &ot
   , mTitle(other.mTitle)
   , mPostData(other.mPostData)
   , mLayoutHistoryState(other.mLayoutHistoryState)
   , mLoadType(0)         // XXX why not copy?
   , mID(other.mID)
   , mDocIdentifier(other.mDocIdentifier)
   , mScrollPositionX(0)  // XXX why not copy?
   , mScrollPositionY(0)  // XXX why not copy?
+  , mURIWasModified(other.mURIWasModified)
   , mIsFrameNavigation(other.mIsFrameNavigation)
   , mSaveLayoutState(other.mSaveLayoutState)
   , mExpired(other.mExpired)
   , mSticky(PR_TRUE)
   , mDynamicallyCreated(other.mDynamicallyCreated)
   // XXX why not copy mContentType?
   , mCacheKey(other.mCacheKey)
   , mParent(other.mParent)
@@ -203,16 +205,28 @@ NS_IMETHODIMP nsSHEntry::SetScrollPositi
 
 NS_IMETHODIMP nsSHEntry::GetScrollPosition(PRInt32 *x, PRInt32 *y)
 {
   *x = mScrollPositionX;
   *y = mScrollPositionY;
   return NS_OK;
 }
 
+NS_IMETHODIMP nsSHEntry::GetURIWasModified(PRBool* aOut)
+{
+  *aOut = mURIWasModified;
+  return NS_OK;
+}
+
+NS_IMETHODIMP nsSHEntry::SetURIWasModified(PRBool aIn)
+{
+  mURIWasModified = aIn;
+  return NS_OK;
+}
+
 NS_IMETHODIMP nsSHEntry::GetURI(nsIURI** aURI)
 {
   *aURI = mURI;
   NS_IF_ADDREF(*aURI);
   return NS_OK;
 }
 
 NS_IMETHODIMP nsSHEntry::SetURI(nsIURI* aURI)
--- a/docshell/shistory/src/nsSHEntry.h
+++ b/docshell/shistory/src/nsSHEntry.h
@@ -97,16 +97,17 @@ private:
   nsCOMPtr<nsIInputStream>        mPostData;
   nsCOMPtr<nsILayoutHistoryState> mLayoutHistoryState;
   nsCOMArray<nsISHEntry>          mChildren;
   PRUint32                        mLoadType;
   PRUint32                        mID;
   PRInt64                         mDocIdentifier;
   PRInt32                         mScrollPositionX;
   PRInt32                         mScrollPositionY;
+  PRPackedBool                    mURIWasModified;
   PRPackedBool                    mIsFrameNavigation;
   PRPackedBool                    mSaveLayoutState;
   PRPackedBool                    mExpired;
   PRPackedBool                    mSticky;
   PRPackedBool                    mDynamicallyCreated;
   nsCString                       mContentType;
   nsCOMPtr<nsISupports>           mCacheKey;
   nsISHEntry *                    mParent;  // weak reference
--- a/docshell/test/Makefile.in
+++ b/docshell/test/Makefile.in
@@ -111,16 +111,18 @@ include $(topsrcdir)/config/rules.mk
 		file_bug660404^headers^ \
 		test_bug662170.html \
 		file_bug662170.html \
 		test_bug570341.html \
 		bug570341_recordevents.html \
 		test_bug668513.html \
 		bug668513_redirect.html \
 		bug668513_redirect.html^headers^ \
+		test_bug669671.html \
+		file_bug669671.sjs \
 		$(NULL)
 
 ifeq ($(MOZ_WIDGET_TOOLKIT),cocoa)
 _TEST_FILES += \
 		test_bug511449.html \
 		file_bug511449.html \
 		$(NULL)
 endif
new file mode 100644
--- /dev/null
+++ b/docshell/test/file_bug669671.sjs
@@ -0,0 +1,13 @@
+function handleRequest(request, response)
+{
+  var count = parseInt(getState('count'));
+  if (!count)
+    count = 0;
+  setState('count', count + 1 + '');
+
+  response.setHeader('Content-Type', 'text/html', false);
+  response.setHeader('Cache-Control', 'max-age=0');
+  response.write('<html><body onload="opener.onChildLoad()" ' +
+                 'onunload="parseInt(\'0\')">' +
+                 count + '</body></html>');
+}
new file mode 100644
--- /dev/null
+++ b/docshell/test/test_bug669671.html
@@ -0,0 +1,140 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=669671
+-->
+<head>
+  <title>Test for Bug 669671</title>
+  <script type="application/javascript" src="/MochiKit/packed.js"></script>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=669671">Mozilla Bug 669671</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+
+</div>
+<pre id="test">
+<script type="application/javascript;version=1.7">
+
+/**
+ * Test for Bug 669671.
+ *
+ * This is a bit complicated.  We have a script, file_bug669671.sjs, which counts
+ * how many times it's loaded and returns that count in the body of an HTML
+ * document.  For brevity, call this page X.
+ *
+ * X is sent with Cache-Control: max-age=0 and can't be bfcached (it has an
+ * onunload handler).  Our test does the following in a popup:
+ *
+ * 1) Load X?pushed, to prime the cache.
+ * 2) Navigate to X.
+ * 3) Call pushState and navigate from X to X?pushed.
+ * 4) Navigate to X?navigated.
+ * 5) Go back (to X?pushed).
+ *
+ * We do all this work so we can check that in step 5, we fetch X?pushed from
+ * the network -- we shouldn't use our cached copy, because of the
+ * cache-control header X sends.
+ *
+ * Then we go back and repeat the whole process but call history.replaceState
+ * instead of pushState.  And for good measure, we test once more, this time
+ * modifying only the hash of the URI using replaceState.  In this case, we
+ * *should* load from the cache.
+ *
+ **/
+SimpleTest.waitForExplicitFinish();
+
+function onChildLoad()
+{
+  SimpleTest.executeSoon(function() { gGen.next() });
+}
+
+var _loadCount = 0;
+function checkPopupLoadCount()
+{
+  is(popup.document.body.innerHTML, _loadCount + '', 'Load count');
+
+  // We normally want to increment _loadCount here.  But if the test fails
+  // because we didn't do a load we should have, let's not cause a cascade of
+  // failures by incrementing _loadCount.
+  var origCount = _loadCount;
+  if (popup.document.body.innerHTML >= _loadCount + '')
+    _loadCount++;
+  return origCount;
+}
+
+function test()
+{
+  // Step 1 - The popup's body counts how many times we've requested the
+  // resource.  This is the first time we've requested it, so it should be '0'.
+  checkPopupLoadCount();
+
+  // Step 2 - We'll get another onChildLoad when this finishes.
+  popup.location = 'file_bug669671.sjs';
+  yield;
+
+  // Step 3 - Call pushState and change the URI back to ?pushed.
+  checkPopupLoadCount();
+  popup.history.pushState('', '', '?pushed');
+
+  // Step 4 - Navigate away.  This should trigger another onChildLoad.
+  popup.location = 'file_bug669671.sjs?navigated-1';
+  yield;
+
+  // Step 5 - Go back.  This should result in another onload (because the file is
+  // not in bfcache) and should be the fourth time we've requested the sjs file.
+  checkPopupLoadCount();
+  popup.back();
+  yield;
+
+  // This is the check which was failing before we fixed the bug.
+  checkPopupLoadCount();
+
+  popup.close();
+
+  // Do the whole thing again, but with replaceState.
+  popup = window.open('file_bug669671.sjs?replaced');
+  yield;
+  checkPopupLoadCount();
+  popup.location = 'file_bug669671.sjs';
+  yield;
+  checkPopupLoadCount();
+  popup.history.replaceState('', '', '?replaced');
+  popup.location = 'file_bug669671.sjs?navigated-2';
+  yield;
+  checkPopupLoadCount();
+  popup.back();
+  yield;
+  checkPopupLoadCount();
+  popup.close();
+
+  // Once more, with feeling.  Notice that we don't have to prime the cache
+  // with an extra load here, because X and X#hash share the same cache entry.
+  popup = window.open('file_bug669671.sjs?hash-test');
+  yield;
+  var initialCount = checkPopupLoadCount();
+  popup.history.replaceState('', '', '#hash');
+  popup.location = 'file_bug669671.sjs?navigated-3';
+  yield;
+  checkPopupLoadCount();
+  popup.back();
+  yield;
+  is(popup.document.body.innerHTML, initialCount + '',
+     'Load count (should be cached)');
+  popup.close();
+
+  SimpleTest.finish();
+  yield;
+}
+
+// This will call into onChildLoad once it loads.
+var popup = window.open('file_bug669671.sjs?pushed');
+
+var gGen = test();
+
+</script>
+</pre>
+</body>
+</html>