500328 - Add support for HTML5 History.pushState(), History.replaceState() methods

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement)

Description

[Justin Lebar (not reading bugmail)]

This enhancement introduces the following new methods from HTML5 to the History interface:

* History.pushState(data, title, [url]) adds a new history entry associated with the given data, changes the window title, and optionally changes the page URL.

* History.clearState() clears all history associated with the current Document.

Additionally, the enhancement adds the onpopstate event to Window.

See the following links for details:

* pushState: http://www.whatwg.org/specs/web-apps/current-work/?slow-browser#dom-history-pushstate

* clearState: http://www.whatwg.org/specs/web-apps/current-work/?slow-browser#dom-history-clearstate

* popstate: http://www.whatwg.org/specs/web-apps/current-work/?slow-browser#activating-state-object-entries

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Before implementing this, it might be good to get HTML5's
session history fixed.
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-July/020719.html

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Comment 3

[Justin Lebar (not reading bugmail)]

Some notes about the above patch:

* We added a replaceState function, with the same semantics as pushState, except that it modifies the current state instead of creating a new state.  This is useful in a number of cases, such as setting the state object for the initial page visited.  We intend to get this function added to the spec.

* Under normal circumstances, if you start at page A, pushstate to page B, refresh, and go back to A, we don't change the document -- the document you got from refreshing B gets a popstate.  But if you get a response code of 400 or greater when refreshing B, we consider it to be a different document.  If the refresh of B returned, for instance, a 404, then we'd actually load A's document when we go back.

* State objects are serialized to JSON.  Since we save these objects to disk (for session restore), we have a length limit on them.  The pref is set to 640k characters by default, which ought to be enough for anybody.

* If you're at A, push/replace state to B, and then navigate to C (by setting document.location, clicking a link, etc), we send B as the HTTP referer.  But if you're at A, navigate to B, pushstate to C, pushstate to D, then refresh D, we send B as the HTTP referer, because pushstates aren't really navigations.

* If you push/replace state to a new directory, we re-resolve all the link hrefs on the page (this functionality was added in bug 209275), but we don't re-resolve any other elements on the page.

Comment 4

[Justin Lebar (not reading bugmail)]

Attachment: Patvh v1.0.1

Small revision to previous patch; removed some unnecessary code.

Comment 5

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1.2

Rebased atop second patch for checkin in bug 209275 and rev 06c77b52f492.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 402482 [details] [diff] [review]
Patch v1.2


>+++ b/dom/interfaces/events/nsIDOMPopStateEvent.idl
>+[scriptable, uuid(f3834fd5-0ef5-4ccd-a741-0b6685414342)]
>+interface nsIDOMPopStateEvent : nsIDOMEvent
>+{
>+  /**
>+   * The state associated with this popstate event
>+   */
>+  attribute nsIVariant state;
state should be readonly, at least per HTML5 draft.

Comment 7

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1.2.1

In response to comment #6:

State variable is now read-only.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 404116 [details] [diff] [review]
Patch v1.2.1

@@ -1160,16 +1159,26 @@ public:
>    * Returns true if the locale used for the document specifies a direction of
>    * right to left. For chrome documents, this comes from the chrome registry.
>    * This is used to determine the current state for the :-moz-locale-dir pseudoclass
>    * so once can know whether a document is expected to be rendered left-to-right
>    * or right-to-left.
>    */
>   virtual PRBool IsDocumentRightToLeft() { return PR_FALSE; }
> 
>+  nsAString& GetPendingStateObject()
>+  {
>+    return mPendingStateObject;
>+  }
>+
>+  void SetPendingStateObject(nsAString &obj)
>+  {
>+    mPendingStateObject.Assign(obj);
>+  }
Add some comments here.


>+nsresult NS_NewDOMPopStateEvent(nsIDOMEvent** aInstancePtrResult,
>+                            nsPresContext* aPresContext,
>+                            nsEvent* aEvent);
Fix alignment of the parameters.


>-    // Notify the ContentViewer that the Document has finished loading...
>-    //
>-    // This will cause any OnLoad(...) handlers to fire, if it is a HTML
>-    // document...
>+    // Notify the ContentViewer that the Document has finished loading.  This
>+    // will cause any OnLoad(...) and PopState(...) handlers to fire, if it is
>+    // an HTML document.
Could you fix the comment; Notifying content viewer causes load and popstate to be
dispatched, even if the document isn't HTML document.

>+            SetDocPendingStateObj(mOSHE);
>+
>+            // Dispatch the popstate and hashchange events, as appropriate
>+            nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(mScriptGlobal);
>+            if (window) {
>+                window->DispatchSyncPopState();
>+
>+                if (doHashchange)
>+                  window->DispatchAsyncHashchange();
>+            }
>+
>+            if (sameDocIdent) {
>+                // Set the doc's URI according to the new history entry's URI
>+                nsCOMPtr<nsIURI> newURI;
>+                mOSHE->GetURI(getter_AddRefs(newURI));
>+                NS_ENSURE_TRUE(newURI, NS_ERROR_FAILURE);
>+
>+                nsCOMPtr<nsIDocumentViewer>
>+                    docv(do_QueryInterface(mContentViewer));
>+                NS_ENSURE_TRUE(docv, NS_ERROR_FAILURE);
>+                nsCOMPtr<nsIDocument> doc;
>+                docv->GetDocument(getter_AddRefs(doc));
>+                NS_ENSURE_TRUE(doc, NS_ERROR_FAILURE);
>+
>+                doc->SetDocumentURI(newURI);
>             }
Should document URI be set before dispatching popstate (some event listener
may change document uri again)?
And do we want to set the document URI always, even if it wasn't used in
pushState? Or do we just use the old URI in that case?

>+nsDocShell::StringifyVariant(nsIVariant *aData, nsAString &aResult)
>+{
>+    nsresult rv;
>+    aResult.Truncate();
>+
>+    // First, try to extract a jsval from the variant |aData|.  This works only
>+    // if the variant is an XPCVariant, which is normally created only by
>+    // XPCOM.
>+    jsval jsData;
>+    rv = aData->GetAsJSVal(&jsData);
>+    NS_ENSURE_SUCCESS(rv, NS_ERROR_UNEXPECTED);
Since this method works only with 'jsval_variant'->string, the
method name should be a bit different.
Maybe StringifyJSValVariant?

>+
>+    nsCOMPtr<nsIDocumentViewer> docViewer =
>+        do_QueryInterface(mContentViewer, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsCOMPtr<nsIDocument> document;
>+    docViewer->GetDocument(getter_AddRefs(document));
>+    NS_ENSURE_TRUE(document, NS_ERROR_FAILURE);
Since you're doing this in a few places, perhaps you could
add a helper method for this.
nsIDocument* nsDocShell::GetCurrentDocument();


>+    // Check that the state object isn't too long.
>+    NS_ENSURE_TRUE(dataStr.Length() <= maxStateObjSize,
>+                   NS_ERROR_OUT_OF_MEMORY);
NS_ERROR_OUT_OF_MEMORY doesn't seem like the right error message here.
Maybe NS_ERROR_ILLEGAL_VALUE?


>+    // Try to set the title the current history element
>+    PRUnichar *oldTitle;
>+    mOSHE->GetTitle(&oldTitle);
>+    if (mOSHE)
>+      mOSHE->SetTitle(aTitle); // Set the title, ignoring any errors
>+
>+    return NS_OK;
>+}
I don't understand this.
You get the title from mOSHE, then null check mOSHE and set title.
And btw, you leak oldTitle here.


>+   * If the JSON representation of aData has more than maxStateObjSize
>+   * characters, this function returns an out of memory error.
>+   */
>+  void addState(in nsIVariant aData, in DOMString aTitle,
>+                in DOMString aURL, in boolean replace,
>+                in long maxStateObjSize);
Do we really want maxStateObjSize as a parameter here?
Just get the pref in nsDocShell, it has mPrefs member variable.

>+++ b/docshell/shistory/public/nsISHistoryInternal.idl
...
>-[scriptable, uuid(7ca0fd71-437c-48ad-985d-11ce9e2429b4)]
>+[scriptable, uuid(a6261f4d-775b-4d6e-8052-3b592af3c5ed)]
> interface nsISHistoryInternal: nsISupports
> {
>   /**
>    * Add a new Entry to the History List
>    * @param aEntry - The entry to add
>    * @param aPersist - If true this specifies that the entry should persist
>    * in the list.  If false, this means that when new entries are added
>    * this element will not appear in the session history list.
Why are you changing uuid here?


>+   nsresult   LoadEntry(PRInt32 aIndex, nsISHEntry* prevEntry,
>+                        nsISHEntry* nextEntry, long aLoadType,
>+                        PRUint32 histCmd);
Add some comment here that how does this method differ from the other
LoadEntry, and when should one use this and when the other.


>+  nsCOMPtr<nsIDOMDocumentEvent> docEvent = do_QueryInterface(mDoc);
>+  NS_ENSURE_TRUE(docEvent, NS_ERROR_FAILURE);
>+
>+  nsCOMPtr<nsIDOMEvent> domEvent;
>+  rv = docEvent->CreateEvent(NS_LITERAL_STRING("popstate"),
>+                             getter_AddRefs(domEvent));
>+  NS_ENSURE_SUCCESS(rv, rv);
You could use nsEventDispatcher::CreateEvent here

>+  nsCOMPtr<nsIDOMEventTarget> outerWindow
>+    = do_QueryInterface(GetOuterWindow());
'=' should be in the previous line.

>+[scriptable, uuid(f3834fd5-0ef5-4ccd-a741-0b6685414342)]
>+interface nsIDOMPopStateEvent : nsIDOMEvent
>+{
>+  /**
>+   * The state associated with this popstate event
>+   */
>+  readonly attribute nsIVariant state;
>+
>+  void initPopStateEvent(in DOMString typeArg,
>+                         in boolean canBubbleArg,
>+                         in boolean cancelableArg,
>+                         in nsIVariant stateArg);
Could you add a comment here or somewhere else that what kind of
variant we're expecting as a parameter.

>+nsJSON::DecodeInternal(const nsAString &str, JSContext *cx, jsval *result)
Should the caller of this GCroot *result? If so, please add some comment to the .idl.
> 
>+  // We need to unsuppress painting before we dispatch the popstate event
>+  // because we need the window's document to be set in case the onpopstate
>+  // handler navigates the window.
>+  if (!mStopped) {
>+    window->DispatchSyncPopState();
>+  }
So um, where do you unsuppress painting here, if that is really needed?
Or is the comment just wrong.

@@ -602,16 +603,19 @@ nsDOMEvent::SetEventType(const nsAString
...
+  } else if (mEvent->eventStructType == NS_POPSTATE_EVENT) {
+    if (atom == nsGkAtoms::onpopstate)
+      mEvent->message = NS_POPSTATE;
So where do you use NS_POPSTATE_EVENT?
I don't see nsPopstateEvent or anything similar anywhere.
Seems like you're just using normal NS_EVENT event struct for
popstate events.


diff --git a/widget/public/nsGUIEvent.h b/widget/public/nsGUIEvent.h
...
>+#define NS_POPSTATE_EVENT                 26
So remove this.

xpconnect parts need review from mrbkap.

Comment 9

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1.3

> >+            SetDocPendingStateObj(mOSHE);
> >+
> >+            // Dispatch the popstate and hashchange events, as appropriate
> >+            nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(mScriptGlobal);
> >+            if (window) {
> >+                window->DispatchSyncPopState();
> >+
> >+                if (doHashchange)
> >+                  window->DispatchAsyncHashchange();
> >+            }
> >+
> >+            if (sameDocIdent) {
> >+                // Set the doc's URI according to the new history entry's URI
> >+                nsCOMPtr<nsIURI> newURI;
> >+                mOSHE->GetURI(getter_AddRefs(newURI));
> >+                NS_ENSURE_TRUE(newURI, NS_ERROR_FAILURE);
> >+
> >+                nsCOMPtr<nsIDocumentViewer>
> >+                    docv(do_QueryInterface(mContentViewer));
> >+                NS_ENSURE_TRUE(docv, NS_ERROR_FAILURE);
> >+                nsCOMPtr<nsIDocument> doc;
> >+                docv->GetDocument(getter_AddRefs(doc));
> >+                NS_ENSURE_TRUE(doc, NS_ERROR_FAILURE);
> >+
> >+                doc->SetDocumentURI(newURI);
> >             }
> [snip]
> And do we want to set the document URI always, even if it wasn't used in
> pushState? Or do we just use the old URI in that case?

We could compare the document's current URI to the history entry's current URI
here, but I don't see why that would be advantageous to calling SetDocumentURI
without such a check.  The |if (sameDocIdent)| branch is only entered if we're
navigating between two history entries which are related to each other by a
popState.

> 
> >+
> >+    nsCOMPtr<nsIDocumentViewer> docViewer =
> >+        do_QueryInterface(mContentViewer, &rv);
> >+    NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+    nsCOMPtr<nsIDocument> document;
> >+    docViewer->GetDocument(getter_AddRefs(document));
> >+    NS_ENSURE_TRUE(document, NS_ERROR_FAILURE);
> Since you're doing this in a few places, perhaps you could
> add a helper method for this.
> nsIDocument* nsDocShell::GetCurrentDocument();

We can already get the nsIDOMDocument by calling
  do_GetInterface(GetAsSupports(this))
so I added another case in nsDocShell::GetInterface for casting to an
nsIDocument.  Is this OK?

> >+[scriptable, uuid(f3834fd5-0ef5-4ccd-a741-0b6685414342)]
> >+interface nsIDOMPopStateEvent : nsIDOMEvent
> >+{
> >+  /**
> >+   * The state associated with this popstate event
> >+   */
> >+  readonly attribute nsIVariant state;
> >+
> >+  void initPopStateEvent(in DOMString typeArg,
> >+                         in boolean canBubbleArg,
> >+                         in boolean cancelableArg,
> >+                         in nsIVariant stateArg);
> Could you add a comment here or somewhere else that what kind of
> variant we're expecting as a parameter.

We never stringify a popstate event's state argument; if there are restrictions
on the nsIVariant we pass, presumably they'd be the same as for any other
nsIVariants we want to pass through XPCOM?

> >+nsJSON::DecodeInternal(const nsAString &str, JSContext *cx, jsval *result)
> Should the caller of this GCroot *result? If so, please add some comment to the
> .idl.

Comment added.  I know nothing about the JS GC, but I presume I don't need to
worry about GCrooting the state object myself after I decode it because I add
it to the PopState event?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

But you call DecodeInternal before you add anything to PopState.
You don't gcroot jsStateObj. So if DecodeInternal calls gc, something bad 
may happen.

Btw, perhaps decodeInternal could be called something else.
There is already such method, which is really "internal", but you're adding
a method to the interface.
decodeToJSVal?

Will review the rest asap.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 413971 [details] [diff] [review]
Patch v1.3

>diff --git a/content/base/public/nsIDocument.h b/content/base/public/nsIDocument.h
You need to update NS_IDOCUMENT_IID

>+class nsDOMPopStateEvent : public nsDOMEvent,
>+                           public nsIDOMPopStateEvent
>+{
>+public:
>+  nsDOMPopStateEvent(nsPresContext* aPresContext, nsEvent* aEvent)
>+    : nsDOMEvent(aPresContext, aEvent)  // state
>+  {
>+  }
>+
>+  virtual ~nsDOMPopStateEvent();
>+
>+  NS_DECL_ISUPPORTS_INHERITED
>+
>+  NS_DECL_NSIDOMPOPSTATEEVENT
>+
>+  NS_FORWARD_TO_NSDOMEVENT
>+
>+protected:
>+  nsCOMPtr<nsIVariant> mState;
Didn't notice this when reviewing first time; add mState to cycle collection.
Though, I'm not sure if that will really help, since variant isn't cycle collected.

Perhaps file a followup bug to make a main thread only cycle-collectable variant.
But still, add mState to cycle collection.

Could you test whether the following leaks.
var evt = document.createEvent("popupstateevent");
evt.initEvent("foo", false, false, evt);


>   if (aEventType.LowerCaseEqualsLiteral("scrollareaevent"))
>     return NS_NewDOMScrollAreaEvent(aDOMEvent, aPresContext, nsnull);
>+  if (aEventType.LowerCaseEqualsLiteral("popstate"))
>+    return NS_NewDOMPopStateEvent(aDOMEvent, aPresContext, nsnull);
This should be "popupstateevent".

>+    else if (aIID.Equals(NS_GET_IID(nsIDocument)) &&
>+             NS_SUCCEEDED(EnsureContentViewer())) {
>+        nsCOMPtr<nsIDOMDocument> domDoc;
>+        mContentViewer->GetDOMDocument(getter_AddRefs(domDoc));
>+        if (!domDoc)
>+            return NS_NOINTERFACE;
>+        return domDoc->QueryInterface(aIID, aSink);
>+    }
So the caller is always ok that this may actually create a new about:blank document? But yeah, I've been thinking about adding this to GetInterface.
Could you file a followup to simplify code which takes nsIDOMDocument and then
QIs that to nsIDocument.

>+nsresult
>+nsGlobalWindow::DispatchSyncPopState()
>+{
>+  FORWARD_TO_INNER(DispatchSyncPopState, (), NS_OK);
Could you add an assertion here that nsContentUtils::IsSafeToRunScript() returns
true.


>+nsHistory::PushState(nsIVariant *aData, const nsAString& aTitle,
>+                     const nsAString& aURL)
...
>+  // Notify the session history that it might need to evict a content viewer
>+  nsCOMPtr<nsISHistory> session_history;
>+  GetSessionHistoryFromDocShell(mDocShell, getter_AddRefs(session_history));
>+  // From the page's perspective, this isn't a failure.
>+  NS_ENSURE_TRUE(session_history, NS_OK);
>+
>+  nsCOMPtr<nsISHistoryInternal> session_history_internal =
>+    do_QueryInterface(session_history);
>+  NS_ENSURE_TRUE(session_history_internal, NS_OK);
>+
>+  PRInt32 curIndex = -1;
>+  rv = session_history->GetIndex(&curIndex);
>+  if (NS_SUCCEEDED(rv) && curIndex > -1) {
>+    session_history_internal->EvictContentViewers(curIndex - 1, curIndex);
>+  }
Could you explain this?

I could look at this still once more, so r-.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #11)
> Perhaps file a followup bug to make a main thread only cycle-collectable
> variant.

Ok, seems like we do have cycle-collectable variant "XPCVariant".
So adding mState to cycle collection should be enough.
But please add a test like 
var evt = document.createEvent("popupstateevent");
evt.initEvent("foo", false, false, evt);
to the testcases. That should hopefully capture possible memory leaks when
mochitest runs.

Comment 13

[Justin Lebar (not reading bugmail)]

(In reply to comment #12)
> But please add a test like 
> var evt = document.createEvent("popupstateevent");
> evt.initEvent("foo", false, false, evt);
> to the testcases.
I added this to the testcase, and mochitest didn't complain about any leaks.  Does this mean that the cycle collection in nsDOMEvent is working on nsDOMPopStateEvent?

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Note that you have to enable leak testing in order for us to detect leaks. Set the environment variable XPCOM_MEM_LEAK_LOG to 1. For more details see
https://wiki.mozilla.org/Performance:Leak_Tools#trace-refcnt_and_the_refcount_balancer

Comment 15

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1.4

(In reply to comment #11)
>>+ nsHistory::PushState(nsIVariant *aData, const nsAString& aTitle,
>>+                      const nsAString& aURL)
>>...
>>+  // Notify the session history that it might need to evict a content viewer
>>...
>>    session_history_internal->EvictContentViewers(curIndex - 1, curIndex);
>
>Could you explain this?

This is to notify the bfcache that we've added a new session history entry so it can evict viewers as appropriate.  I added a comment to that effect to the patch.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 414389 [details] [diff] [review]
Patch v1.4

>diff --git a/browser/components/sessionstore/src/nsSessionStore.js b/browser/components/sessionstore/src/nsSessionStore.js
Someone else should probably review these changes.

>+NS_IMETHODIMP
>+nsDOMPopStateEvent::GetState(nsIVariant **aState)
>+{
>+  NS_PRECONDITION(aState, "null state arg");
>+  *aState = mState;
>+  NS_IF_ADDREF(*aState);
>+  return NS_OK;
>+}
Nit, I prefer NS_IF_ADDREF(*aState = mState);

>+    jsval jsStateObj;
>+    nsCOMPtr<nsIJSON> json = do_GetService("@mozilla.org/dom/json;1");
>+    NS_ENSURE_TRUE(cxPusher.Push(cx), NS_ERROR_FAILURE);
>+    rv = json->DecodeToJSVal(stateObjJSON, cx, &jsStateObj);
>+    cxPusher.Pop();
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // Root the jsStateObj until we wrap it in a variant.
>+    nsAutoGCRoot(jsStateObj, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
Shouldn't you autogcroot before you use jsStateObj in DecodeToJSVal.

>+nsHistory::PushState(nsIVariant *aData, const nsAString& aTitle,
>+                     const nsAString& aURL)
...
>+  // To play nice with bfcache, notify the session history that it might need
>+  // to evict a content viewer
>+  nsCOMPtr<nsISHistory> session_history;
>+  GetSessionHistoryFromDocShell(mDocShell, getter_AddRefs(session_history));
>+  // From the page's perspective, this isn't a failure.
>+  NS_ENSURE_TRUE(session_history, NS_OK);
>+
>+  nsCOMPtr<nsISHistoryInternal> session_history_internal =
>+    do_QueryInterface(session_history);
>+  NS_ENSURE_TRUE(session_history_internal, NS_OK);
>+
>+  PRInt32 curIndex = -1;
>+  rv = session_history->GetIndex(&curIndex);
>+  if (NS_SUCCEEDED(rv) && curIndex > -1) {
>+    session_history_internal->EvictContentViewers(curIndex - 1, curIndex);
>+  }
Btw, I was mainly wondering the parameters to the EvictContentViewers.
That method is a bit strange.

>+nsIURI*
>+nsLocation::GetSourceURI(nsIPrincipal* principal,
>+                         nsIScriptSecurityManager* secMan) {
Could you explain why this method is doing what it does.
Especially, why http(s) is handled differently than other schemes?
How should file: work? What about ftp: ? jar: ?

>+  [noscript] AString  encodeInternal(in JSValPtr value, in JSContext cx);
Could you change also this method name. Something similar what you did too decodeXXX

>diff --git a/js/src/xpconnect/src/*
mrbkap or someone should review these changes.

With comments addressed and GetSourceURI handling explained (possibly fixed?), r=me

IMO, this needs sr, probably from jonas or jst.

Comment 17

[Justin Lebar (not reading bugmail)]

Attachment: Patch 1.4.1

(In reply to comment #16)
> (From update of attachment 414389 [details] [diff] [review])

> NS_IMETHODIMP
> nsJSON::DecodeToJSVal(const nsAString &str, JSContext *cx, jsval *result)
> {
>   JSONParser *parser = JS_BeginJSONParse(cx, result);
>   NS_ENSURE_TRUE(parser, NS_ERROR_UNEXPECTED);
> 
>   JSBool ok = JS_ConsumeJSONText(cx, parser,
>                                  (jschar*)PromiseFlatString(str).get(),
>                                  (uint32)str.Length());
> 
>   // Since we've called JS_BeginJSONParse, we have to call JS_FinishJSONParse,
>   // even if JS_ConsumeJSONText fails.  But if either fails, we'll report an
>   // error.
>   ok = ok && JS_FinishJSONParse(cx, parser, JSVAL_NULL);
> 
>   if (!ok) {
>     return NS_ERROR_UNEXPECTED;
>   }
> 
>   return NS_OK;
> }
> Shouldn't you autogcroot before you use jsStateObj in DecodeToJSVal.

I think this is OK because JS_BeginJSONParse calls js_BeginJSONParse (json.cpp:644), which calls js_AddRoot.  JS_FinishJSONParse similarly removes the root.

> >+nsIURI*
> >+nsLocation::GetSourceURI(nsIPrincipal* principal,
> >+                         nsIScriptSecurityManager* secMan) {
> Could you explain why this method is doing what it does.
> Especially, why http(s) is handled differently than other schemes?
> How should file: work? What about ftp: ? jar: ?

I'm a bit flummoxed by this code as well.  AFAICT, it's there to make HTTP referer work properly after you've pushState'd to a new URL.  But I agree that we don't need special handling of http(s), so I took that out.

> >diff --git a/js/src/xpconnect/src/*
> mrbkap or someone should review these changes.
> 
> IMO, this needs sr, probably from jonas or jst.

> >diff --git a/browser/components/sessionstore/src/nsSessionStore.js b/browser/components/sessionstore/src/nsSessionStore.js
> Someone else should probably review these changes.

Who wants to be next?  :)

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 414572 [details] [diff] [review]
Patch 1.4.1

Asking Paul for review on the session restore parts.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #17)
> > Shouldn't you autogcroot before you use jsStateObj in DecodeToJSVal.
> 
> I think this is OK because JS_BeginJSONParse calls js_BeginJSONParse
> (json.cpp:644), which calls js_AddRoot.  JS_FinishJSONParse similarly removes
> the root.
Well, still, is there any reason not to move the nsAutoGCroot few lines up and
be safe for sure?

Comment 20

[Justin Lebar (not reading bugmail)]

(In reply to comment #19)
> Well, still, is there any reason not to move the nsAutoGCroot few lines up and
> be safe for sure?

Here's the relevant code in nsGlobalWindow::DispatchSyncPopState():

    jsval jsStateObj;
    ...
    rv = json->DecodeToJSVal(stateObjJSON, cx, &jsStateObj);
    cxPusher.Pop();
    NS_ENSURE_SUCCESS(rv, rv);

    // Root the jsStateObj until we wrap it in a variant.
    nsAutoGCRoot(&jsStateObj, &rv);
    NS_ENSURE_SUCCESS(rv, rv);

The question is whether we should gcroot immediately after we get the jsStateObj from DecodeToJSVal.

I don't mind making this change, but I'm not sure it makes us any safer.  We still unroot the object before returning from DecodeToJSVal, so there's still a period of time where the object is hanging around unrooted.  I don't know what the semantics of rooting jsvals are, so I don't know whether this is OK.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #20)
> I don't mind making this change, but I'm not sure it makes us any safer.  We
> still unroot the object before returning from DecodeToJSVal,
Where? GCRoot operates on the address of the variable.
Btw, I think jsStateObj should be initialized.

Comment 22

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 414572 [details] [diff] [review]
Patch 1.4.1

Historically session restore is not as well tested as it should be and I'd like to change that. So unless you have a compelling reason otherwise, I'd feel much better if there was a test for the session restore part.

Comment 23

[Justin Lebar (not reading bugmail)]

Attachment: Patch 1.4.2

Added session restore test, and moved gcroot per earlier discussion.  Olli, could you check that the gcroot change looks good?

> (In reply to comment 21)
> Btw, I think jsStateObj [in nsGlobalWindow] should be initialized.
Even though it is read only if DecodeToJSVal returns success?  If so, is 0 a meaningful null value to stick in there?

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 416147 [details] [diff] [review]
Patch 1.4.2

Initialize it to JSVAL_NULL?
And still, why nsAutoGCRoot isn't before DecodeToJSVal?
Or does JSAutoRequest prevent GC? I think it does. (Sorry, I think I missed JSAutoRequest before)
(I wish I knew our strange JS API a bit better.)

mrbkap knows this stuff.

Comment 25

[Justin Lebar (not reading bugmail)]

Attachment: Patch 1.4.3

Ah, I think I understand now.  Rooting before the decode seems much safer to me than what I was doing before.

Comment 26

[Justin Lebar (not reading bugmail)]

Attachment: Patch 1.4.4

Oops; I pulled that last attachment from an old cache.  This one should have my latest changes.  Sorry for the bugspam.

Comment 27

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 416172 [details] [diff] [review]
Patch 1.4.4

>diff --git a/browser/components/sessionstore/test/browser/Makefile.in b/browser/components/sessionstore/test/browser/Makefile.in
>+	browser_500328.js \
>+	browser_500328_sample.html \

It looks like you didn't add browser_500328_sample.html... Is that page doing anything or is the real work all being done in the test? If it's all being done in the test, then you can use a generic page (eg. example.com) instead of creating an html file.

>diff --git a/browser/components/sessionstore/test/browser/browser_500328.js b/browser/components/sessionstore/test/browser/browser_500328.js

I know it's just test code, but please include the license info here. And please move checkState up here too.

>+function test() {
>+  // Tests session restore functionality of history.pushState and
>+  // history.replaceState().  (Bug 500328)
>+
>+  let ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
>+  waitForExplicitFinish();
>+
>+  // Force the session store into existence.
>+  gPrefService.setIntPref("browser.sessionstore.interval", 0);

You don't need to do that. In fact it's better if you don't.

>+
>+  const testURL = "http://localhost:8888/browser/" +
>+    "browser/components/sessionstore/test/browser/browser_500328_sample.html";
>+
>+  // Open a new blank tab.
>+  let newWin = openDialog(location, "_blank", "chrome,all,dialog=no", "about:blank");

This is actually opening a new window, which you don't need to do. I would avoid doing it if you can. Instead you could do this all in a new tab. And then since this is run in the scope of the window, you can then just access gBrowser directly.
> let newTab = gBrowser.addTab(testURL);

>+      let contentWindow = newWin.gBrowser.contentWindow;

Not much advantage gained from doing this, but since you have, make sure you use it - see checkState below.

>+      let history = contentWindow.history;
>+      history.pushState({obj1:1}, "title-obj1");
>+      history.pushState({obj2:2}, "title-obj2", "page2");
>+      history.replaceState({obj3:3}, "title-obj3");
>+
>+      // Save the current state, make sure it appears as we expect it to.
>+      let state = ss.getWindowState(newWin);
>+      checkState(contentWindow);

This check is less of sessionstore & more of how push/replaceState work. Ok to have in here though.

>+
>+      // In order to make sure that setWindowState actually modifies the
>+      // window's state, we modify the state here.  checkState will fail if
>+      // this change isn't overwritten by setWindowState.
>+      history.replaceState({should_be_overwritten:true}, "title-overwritten");
>+
>+      // Restore the state and make sure it looks right.
>+      ss.setWindowState(newWin, state, true);
>+      checkState(newWin.gBrowser.contentWindow);

I was expecting a different way of testing this (looking at the actual state returned by ss.getWindowState) but this works too and is actually good to do.

>+      // clean up after ourselves
>+      if (gPrefService.prefHasUserValue("browser.sessionstore.interval"))
>+        gPrefService.clearUserPref("browser.sessionstore.interval");
>+      newWin.close();

Won't need to do either of these.

>+
>+      finish();
>+    }, true);
>+  }, true);
>+}
>+
>+function checkState(contentWindow) {
...
>+    else if (popStateCount == 1) {
>+      popStateCount++;
>+      is(JSON.stringify(aEvent.state), JSON.stringify({obj3:3}),
>+         "second popstate object.");
>+
>+      // Make sure that the new-elem node is present in the document.  If it's
>+      // not, then this history entry has a different doc identifier than the
>+      // previous entry, which is bad.
>+      let doc = contentWindow.document;
>+      let newElem = doc.getElementById("new-elem");
>+      ok(newElem, "doc should contain new-elem.");
>+      newElem.parentNode.removeChild(newElem);
>+      ok(!doc.getElementById("new-elem"), "new-elem should be removed.");

You should be removing the event listener in here.

>+    }
>+  }, true);
>+
>+  contentWindow.history.back();
>+  // Back and popstate happen synchronously, so by the time the call above
>+  // returns, we should have seen our two popstate events.
>+  is(popStateCount, 2, "Going back should generate 2 popstate events.");
>+}

Otherwise it looks good. Thanks for writing the test.

Comment 28

[Justin Lebar (not reading bugmail)]

When I try to run the test in a tab, pushState aborts because it sees nsDocShell::mOSHE == null.  This happens even when I load about:blank into the tab before loading http://example.com and doing my pushStates.

If it's valid for mOSHE to be null, then this is a bug in pushState.  But I can't seem to reproduce this outside the browser chrome test -- i.e. if I open a regular browser and load into a new tab a page which in its onload handler calls pushState, that works fine.

Any thoughts on what might be going on here?

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #28)
> When I try to run the test in a tab, pushState aborts because it sees
> nsDocShell::mOSHE == null.  This happens even when I load about:blank into the
> tab before loading http://example.com and doing my pushStates.
> 
> If it's valid for mOSHE to be null
It is valid. mOSHE has some value when something real has been loaded to the docshell.
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp?mark=6203-6204#6173

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 416172 [details] [diff] [review]
Patch 1.4.4

>diff --git a/content/base/public/nsIDocument.h b/content/base/public/nsIDocument.h
>   /**
>+   * Returns the document's pending state object (serialized to JSON), or the
>+   * empty string if one doesn't exist.  The deserialized object is returned to
>+   * the document in the PopState event.
>+   */
>+  nsAString& GetPendingStateObject()
>+  {
>+    return mPendingStateObject;
>+  }
>+
>+  /**
>+   * Set the document's pending state object (as serialized to JSON).
>+   */
>+  void SetPendingStateObject(nsAString &obj)
>+  {
>+    mPendingStateObject.Assign(obj);
>+  }

Can you document what you mean by "pending"

>+nsDocShell::StringifyJSValVariant(nsIVariant *aData, nsAString &aResult)
>+{
>+    nsresult rv;
>+    aResult.Truncate();
>+
>+    // First, try to extract a jsval from the variant |aData|.  This works only
>+    // if the variant is an XPCVariant, which is normally created only by
>+    // XPCOM.
>+    jsval jsData;
>+    rv = aData->GetAsJSVal(&jsData);
>+    NS_ENSURE_SUCCESS(rv, NS_ERROR_UNEXPECTED);
>+
>+    // Now get the JSContext associated with the current document.
>+    // First get the current document.
>+    nsCOMPtr<nsIDocument> document = do_GetInterface(GetAsSupports(this));
>+    NS_ENSURE_TRUE(document, NS_ERROR_FAILURE);
>+
>+    // Get the JSContext from the document, like we do in
>+    // nsContentUtils::GetContextFromDocument().
>+    nsIScriptGlobalObject *sgo = document->GetScopeObject();
>+    NS_ENSURE_TRUE(sgo, NS_ERROR_FAILURE);
>+
>+    nsIScriptContext *scx = sgo->GetContext();
>+    NS_ENSURE_TRUE(scx, NS_ERROR_FAILURE);
>+
>+    JSContext *cx = (JSContext *)scx->GetNativeContext();
>+
>+    // Begin a new request
>+    JSAutoRequest ar(cx);

Johnny says to move this request into the EncodeFromJSVal function. Same in nsGlobalWindow::DispatchSyncPopState / DecodeToJSVal.

>+nsDocShell::AddState(nsIVariant *aData, const nsAString& aTitle,
>+                     const nsAString& aURL, PRBool aReplace)
>+{
>+    // Implements History.pushState and History.replaceState
>+
>+    // Here's what we do, roughly in the order specified by HTML5:
>+    // 1. Serialize aData to JSON.
>+    // 2. If the third argument is present,
>+    //     a. Resolve the url, relative to the first script's base URL
>+    //     b. If (a) fails, raise a SECURITY_ERR
>+    //     c. Compare the resulting absolute URL to the document's address.  If
>+    //        any part of the URLs difer other than the <path>, <query>, and
>+    //        <fragment> components, raise a SECURITY_ERR and abort.
>+    // 3. If !aReplace:
>+    //     Remove from the session history any entries for the Document from
>+    //     the entry after the current entry up to the last entry in the
>+    //     session history that references the same Document object.
>+    // 4.  As apropriate, either add a state object entry to the session
>+    //     history after the current entry with the following properties, or
>+    //     modify the current session history entry to set
>+    //      a. cloned data as the state object,
>+    //      b. the given title as the title, and,
>+    //      c. if the third argument was present, the absolute URL found in
>+    //         step 2
>+    // 5. If the third argument is present, set the document's current address
>+    //    to the absolute URL found in step 2.
>+    // 6. Update the current entry to be this newly-added entry.

Nice comment!


>+    // Check that the state object isn't too long.
>+    PRInt32 maxStateObjSize = 0;
>+    if (mPrefs) {
>+      mPrefs->GetIntPref("browser.history.maxStateObjectSize",
>+                         &maxStateObjSize);
>+    }
>+    if (maxStateObjSize < 0)
>+      maxStateObjSize = 0;

IMHO a bit of overkill to use a preference here. I'd just hard-code something. But if you wanna keep the pref I think you should at least use a reasonable default size.

>+        // 2c: Same-origin check.
...
>+            if (NS_FAILED(secMan->CheckSameOriginURI(mCurrentURI,
>+                                                     newURI, PR_FALSE)) ||

You should pass PR_TRUE as last argument here so that we log this. PR_FALSE should only be passed if failing the same-origin check is not really an error and some sort of fallback mechanism is used. Which isn't the case here.

>+            if (!principal ||
>+                NS_FAILED(principal->CheckMayLoad(newURI, PR_FALSE))) {

Same here

>+        nsCAutoString currentSpec, newSpec;
>+        NS_ENSURE_SUCCESS(mCurrentURI->GetSpec(currentSpec), NS_ERROR_FAILURE);
>+        NS_ENSURE_SUCCESS(newURI->GetSpec(newSpec), NS_ERROR_FAILURE);
>+        changedURI = !currentSpec.Equals(newSpec);

Any reason not to simply use mCurrentURI->Equals(newSpec) here?

>+    // Create a new entry in the session history.  This may modify mOSHE, which
>+    // we need later, so we keep a reference here.  Adding the entry will erase
>+    // all SHEntries after the new entry and make this entry the current one,
>+    // thus satisfying step 3 of the algorithm.

Nit: Can you start this comment with "Step 3:" like the other comments. It makes scanning the function visually much easier.

>diff --git a/docshell/base/nsDocShell.h b/docshell/base/nsDocShell.h
>--- a/docshell/base/nsDocShell.h
>+++ b/docshell/base/nsDocShell.h
>@@ -365,16 +365,20 @@ protected:
>                             PRBool * aDoHashchange);
> 
>     // Dispatches the hashchange event to the current thread, if the document's
>     // readystate is "complete".
>     nsresult DispatchAsyncHashchange();
> 
>     nsresult FireHashchange();
> 
>+    // Tries to stringify a given variant by converting it to JSON.  This only
>+    // works if the variant is backed by a JSVal.
>+    nsresult StringifyJSValVariant(nsIVariant *aData, nsAString &aResult);

Hmm.. does this mean that history.pushState(3, "") won't work? Since I think that'll be converted to a different type of nsIVariant.


>diff --git a/docshell/shistory/src/nsSHistory.cpp b/docshell/shistory/src/nsSHistory.cpp
>@@ -473,35 +473,41 @@ nsSHistory::PrintHistory()
>     nsCOMPtr<nsISHEntry>  entry;
>     rv = txn->GetSHEntry(getter_AddRefs(entry));
>     if (NS_FAILED(rv) && !entry)
>       return NS_ERROR_FAILURE;
> 
>     nsCOMPtr<nsILayoutHistoryState> layoutHistoryState;
>     nsCOMPtr<nsIURI>  uri;
>     nsXPIDLString title;
>+
>+    nsCOMPtr<nsIContentViewer> viewer;
>+    nsCOMPtr<nsISHEntry> ownerEntry;
>+    entry->GetAnyContentViewer(getter_AddRefs(ownerEntry),
>+                               getter_AddRefs(viewer));

Why this change?


>diff --git a/dom/base/nsHistory.cpp b/dom/base/nsHistory.cpp

>+nsHistory::PushState(nsIVariant *aData, const nsAString& aTitle,
>+                     const nsAString& aURL)
>+{
>+  // Check that PushState hasn't been pref'ed off.
>+  if (!nsContentUtils::GetBoolPref(sAllowPushStatePrefStr, PR_FALSE))
>+    return NS_OK;
>+
>+  NS_ENSURE_TRUE(mDocShell, NS_ERROR_FAILURE);
>+
>+  // PR_FALSE tells the docshell to add a new history entry instead of
>+  // modifying the current one.
>+  nsresult rv = mDocShell->AddState(aData, aTitle, aURL, PR_FALSE);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // To play nice with bfcache, notify the session history that it might need
>+  // to evict a content viewer
>+  nsCOMPtr<nsISHistory> session_history;
>+  GetSessionHistoryFromDocShell(mDocShell, getter_AddRefs(session_history));
>+  // From the page's perspective, this isn't a failure.
>+  NS_ENSURE_TRUE(session_history, NS_OK);
>+
>+  nsCOMPtr<nsISHistoryInternal> session_history_internal =
>+    do_QueryInterface(session_history);
>+  NS_ENSURE_TRUE(session_history_internal, NS_OK);
>+
>+  PRInt32 curIndex = -1;
>+  rv = session_history->GetIndex(&curIndex);
>+  if (NS_SUCCEEDED(rv) && curIndex > -1) {
>+    session_history_internal->EvictContentViewers(curIndex - 1, curIndex);
>+  }

Can the call to AddState run any script synchronously? Such as hashchange event listeners? If so, do you need to make the evict call above before such script runs? So that things behave correctly if that script calls history.forward() for example?

>+nsHistory::ReplaceState(nsIVariant *aData, const nsAString& aTitle,
>+                        const nsAString& aURL)
>+{
>+  // Check that ReplaceState hasn't been pref'ed off
>+  if (!nsContentUtils::GetBoolPref(sAllowReplaceStatePrefStr, PR_FALSE))
>+    return NS_OK;
>+
>+  NS_ENSURE_TRUE(mDocShell, NS_ERROR_FAILURE);
>+
>+  // PR_TRUE tells the docshell to modify the current SHEntry, rather than
>+  // create a new one.
>+  nsresult rv = mDocShell->AddState(aData, aTitle, aURL, PR_TRUE);
>+  return rv;

Just do |return mDocShel->...|

>diff --git a/dom/base/nsLocation.cpp b/dom/base/nsLocation.cpp
>+nsLocation::GetSourceURI(nsIPrincipal* principal,
>+                         nsIScriptSecurityManager* secMan) {
>+  nsresult rv;
>+
>+  nsCOMPtr<nsIURI> principalURI;
>+  principal->GetURI(getter_AddRefs(principalURI));
>+
>+  if (!principalURI)
>+    return nsnull;
>+
>+  // Try to get the document of our caller.  If we can't, perhaps because our
>+  // caller doesn't have a document, just return the principal's URI.
>+  nsCOMPtr<nsIDOMDocument> domDoc = nsContentUtils::GetDocumentFromCaller();

Won't this return the wrong value if someone does:

myIframe.contentWindow.location

Won't that get the location of the caller, rather than the location of the window inside the iframe?

>+  nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);
>+  if (!doc)
>+    return principalURI;
>+
>+  // Finally, get the document's URI.  As a sanity check, make sure that it's
>+  // of the same origin as the principal's URI.  If for some reason they're of
>+  // different origins (maybe someone initiated a location change from C++
>+  // instead of JS and our call to GetDocumentFromCaller gave us a different
>+  // document), we'll just use the principal's URI and ignore the document's.
>+  nsCOMPtr<nsIURI> docURI = doc->GetDocumentURI();
>+  rv = secMan->CheckSameOriginURI(principalURI, docURI, PR_FALSE);

This check *should* never fail right? If so, it'd be a good idea to add an assertion here.


> XPCVariant::VariantDataToJS(XPCLazyCallContext& lccx, 
>                             nsIVariant* variant,
>                             JSObject* scope, nsresult* pErr,
>                             jsval* pJSVal)
> {
>     // Get the type early because we might need to spoof it below.
>     PRUint16 type;
>-    if(NS_FAILED(variant->GetDataType(&type)))
>+    if (NS_FAILED(variant->GetDataType(&type)))
>         return JS_FALSE;
> 
>+    jsval realVal;
>+    nsresult rv = variant->GetAsJSVal(&realVal);
>+
>+    if (NS_SUCCEEDED(rv) &&
>+       (JSVAL_IS_PRIMITIVE(realVal) ||
>+         type == nsIDataType::VTYPE_ARRAY ||
>+         type == nsIDataType::VTYPE_EMPTY_ARRAY ||
>+         type == nsIDataType::VTYPE_ID))
>+    {
>+        // It's not a JSObject (or it's a JSArray or a JSObject representing an
>+        // nsID).  Just pass through the underlying data.
>+        *pJSVal = realVal;
>+        return JS_TRUE;
>+    }
>+
>     nsCOMPtr<XPCVariant> xpcvariant = do_QueryInterface(variant);
>-    if(xpcvariant)
>+    if (xpcvariant && xpcvariant->mReturnRawObject)
>     {
>-        jsval realVal = xpcvariant->GetJSVal();
>-        if(JSVAL_IS_PRIMITIVE(realVal) || 
>-           type == nsIDataType::VTYPE_ARRAY ||
>-           type == nsIDataType::VTYPE_EMPTY_ARRAY ||
>-           type == nsIDataType::VTYPE_ID)
>-        {
>-            // Not a JSObject (or is a JSArray or is a JSObject representing 
>-            // an nsID),.
>-            // So, just pass through the underlying data.
>-            *pJSVal = realVal;
>-            return JS_TRUE;
>-        }
>-
>-        if(xpcvariant->mReturnRawObject)
>-        {
>-            NS_ASSERTION(type == nsIDataType::VTYPE_INTERFACE ||
>-                         type == nsIDataType::VTYPE_INTERFACE_IS,
>-                         "Weird variant");
>-            *pJSVal = realVal;
>-            return JS_TRUE;
>-        }
>+        NS_ASSERTION(type == nsIDataType::VTYPE_INTERFACE ||
>+                     type == nsIDataType::VTYPE_INTERFACE_IS,
>+                     "Weird variant");
>+        *pJSVal = realVal;
>+        return JS_TRUE;


Why do you need these changes? Would like to have an xpconnect guy review this.



>@@ -1074,34 +1072,39 @@ DocumentViewerImpl::LoadComplete(nsresul
>   }
> 
>   // Now that the document has loaded, we can tell the presshell
>   // to unsuppress painting.
>   if (mPresShell && !mStopped) {
>     nsCOMPtr<nsIPresShell> shellDeathGrip(mPresShell);
>     mPresShell->UnsuppressPainting();
>     // mPresShell could have been removed now, see bug 378682/421432
>-    if (mPresShell) {
>-      mPresShell->ScrollToAnchor();
>-    }
>+  }
>+
>+  if (mPresShell && !mStopped) {
>+    mPresShell->ScrollToAnchor();
>   }

Why this change? (Not that it seems wrong necessarily)

sr=me, except would like to see someone xpconnectish person review the xpconnect changes.

Comment 31

[Justin Lebar (not reading bugmail)]

(In reply to comment #29)
> (In reply to comment #28)
> > When I try to run the test in a tab, pushState aborts because it sees
> > nsDocShell::mOSHE == null.  This happens even when I load about:blank into the
> > tab before loading http://example.com and doing my pushStates.
> > 
> > If it's valid for mOSHE to be null
> It is valid. mOSHE has some value when something real has been loaded to the
> docshell.
> http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp?mark=6203-6204#6173

Okay; that makes sense.  But this should only happen if we try to call pushState on about:blank, right?  I think perhaps we should disallow pushState on about:blank and enforce it by not running the function if mOSHE is null.

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That makes sense to me.

Or can mOSHE be null if the first page after about:blank is in the process of being loaded, but we're still running scripts etc in it?

Comment 33

[Justin Lebar (not reading bugmail)]

Well, pushState works fine when it's called from onload.  Should I try to call it from earlier in the page's lifetime?  I'm not sure if it's valid to pushState before onload fires.

Comment 34

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, I was wondering what happens if it's called before onload.

I think ultimately we'll want pushState to work even before onload fires, since normal navigation works fine then. Also it's quite likely that users will interact with a page before onload fires in such a way that pushState gets called.

However I agree that there's likely weird edge cases that needs to be handled in that situation. Which likely needs to be specced and handled. So maybe for now not support pushState before onload, and file a separate bug on that? Up to you.

Comment 35

[Justin Lebar (not reading bugmail)]

Attachment: Patch 1.4.5

Addresses Jonas and Paul's review comments.  Also fixes a bug in which the back button wouldn't always be enabled after a call to pushState.

Comment 36

[Justin Lebar (not reading bugmail)]

Comments on Jonas's review:

>diff --git a/docshell/base/nsDocShell.h b/docshell/base/nsDocShell.h
>--- a/docshell/base/nsDocShell.h
>+++ b/docshell/base/nsDocShell.h
>@@ -365,16 +365,20 @@ protected:
>                             PRBool * aDoHashchange);
> 
>     // Dispatches the hashchange event to the current thread, if the document's
>     // readystate is "complete".
>     nsresult DispatchAsyncHashchange();
> 
>     nsresult FireHashchange();
> 
>+    // Tries to stringify a given variant by converting it to JSON.  This only
>+    // works if the variant is backed by a JSVal.
>+    nsresult StringifyJSValVariant(nsIVariant *aData, nsAString &aResult);

> Hmm.. does this mean that history.pushState(3, "") won't work? Since I think
> that'll be converted to a different type of nsIVariant.

I modified the test so that it pushes an integer and a floating-point number,
and it appears to work fine.  I checked and I think the comment is correct too;
StringifyJSValVariant requires nsIVariant::GetAsJSVal to be implemented, and
only XPCVariant has a real implementation of that method.

>diff --git a/dom/base/nsHistory.cpp b/dom/base/nsHistory.cpp

>+nsHistory::PushState(nsIVariant *aData, const nsAString& aTitle,
>+                     const nsAString& aURL)
>+{
>+  // Check that PushState hasn't been pref'ed off.
>+  if (!nsContentUtils::GetBoolPref(sAllowPushStatePrefStr, PR_FALSE))
>+    return NS_OK;
>+
>+  NS_ENSURE_TRUE(mDocShell, NS_ERROR_FAILURE);
>+
>+  // PR_FALSE tells the docshell to add a new history entry instead of
>+  // modifying the current one.
>+  nsresult rv = mDocShell->AddState(aData, aTitle, aURL, PR_FALSE);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // To play nice with bfcache, notify the session history that it might need
>+  // to evict a content viewer
>+  nsCOMPtr<nsISHistory> session_history;
>+  GetSessionHistoryFromDocShell(mDocShell, getter_AddRefs(session_history));
>+  // From the page's perspective, this isn't a failure.
>+  NS_ENSURE_TRUE(session_history, NS_OK);
>+
>+  nsCOMPtr<nsISHistoryInternal> session_history_internal =
>+    do_QueryInterface(session_history);
>+  NS_ENSURE_TRUE(session_history_internal, NS_OK);
>+
>+  PRInt32 curIndex = -1;
>+  rv = session_history->GetIndex(&curIndex);
>+  if (NS_SUCCEEDED(rv) && curIndex > -1) {
>+    session_history_internal->EvictContentViewers(curIndex - 1, curIndex);
>+  }

> Can the call to AddState run any script synchronously? Such as hashchange event
> listeners? If so, do you need to make the evict call above before such script
> runs? So that things behave correctly if that script calls history.forward()
> for example?

PushState and ReplaceState don't trigger a hashchange, so I think we're safe,
unless we can think of some other event which might fire.

I could evict before the pushState call just to be safe, but then we'd end up
evicting even when pushState failed and we didn't actually add a new entry;
that doesn't seem right.

>diff --git a/dom/base/nsLocation.cpp b/dom/base/nsLocation.cpp
>+nsLocation::GetSourceURI(nsIPrincipal* principal,
>+                         nsIScriptSecurityManager* secMan) {
>+  nsresult rv;
>+
>+  nsCOMPtr<nsIURI> principalURI;
>+  principal->GetURI(getter_AddRefs(principalURI));
>+
>+  if (!principalURI)
>+    return nsnull;
>+
>+  // Try to get the document of our caller.  If we can't, perhaps because our
>+  // caller doesn't have a document, just return the principal's URI.
>+  nsCOMPtr<nsIDOMDocument> domDoc = nsContentUtils::GetDocumentFromCaller();
>
> Won't this return the wrong value if someone does:
>
> myIframe.contentWindow.location
>
> Won't that get the location of the caller, rather than the location of the
> window inside the iframe?
>
> [...]
>
>+  rv = secMan->CheckSameOriginURI(principalURI, docURI, PR_FALSE);
> This check *should* never fail right? If so, it'd be a good idea to add an
> assertion here.

Actually, it turns out that the document is sometimes at about:blank, and in at
least that case, the check will fail.  Looking over this again, I think I can
just use nsLocation::GetURI and nix the whole nsLocation::GetSourceURI method.

> XPCVariant::VariantDataToJS(XPCLazyCallContext& lccx, 
>                             nsIVariant* variant,
>                             JSObject* scope, nsresult* pErr,
>                             jsval* pJSVal)
> [Snip]

> Why do you need these changes? Would like to have an xpconnect guy review this.

I added this GetAsJSVal function to variants which tries to unwrap variants
backed by JSVals.  The code here was unwrapping the JSVal manually, but I
reworked it to use the new function.

It took me a few tries to get this even somewhat right, so I'd definitely
appreciate a review from someone who knows the code better.

>@@ -1074,34 +1072,39 @@ DocumentViewerImpl::LoadComplete(nsresul
>   }
> 
>   // Now that the document has loaded, we can tell the presshell
>   // to unsuppress painting.
>   if (mPresShell && !mStopped) {
>     nsCOMPtr<nsIPresShell> shellDeathGrip(mPresShell);
>     mPresShell->UnsuppressPainting();
>     // mPresShell could have been removed now, see bug 378682/421432
>-    if (mPresShell) {
>-      mPresShell->ScrollToAnchor();
>-    }
>+  }
>+
>+  if (mPresShell && !mStopped) {
>+    mPresShell->ScrollToAnchor();
>   }

> Why this change? (Not that it seems wrong necessarily)

I'm not quite sure why I made that change.  It may be necessary to make some
edge case work, but I've been banging on the patch minus this change for a few
days now, and I haven't been able to get it to misbehave. Since everything
seems to work without it, I took the change out of the patch.

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #36)
> >+    // Tries to stringify a given variant by converting it to JSON.  This only
> >+    // works if the variant is backed by a JSVal.
> >+    nsresult StringifyJSValVariant(nsIVariant *aData, nsAString &aResult);
> 
> > Hmm.. does this mean that history.pushState(3, "") won't work? Since I think
> > that'll be converted to a different type of nsIVariant.
> 
> I modified the test so that it pushes an integer and a floating-point number,
> and it appears to work fine.  I checked and I think the comment is correct too;
> StringifyJSValVariant requires nsIVariant::GetAsJSVal to be implemented, and
> only XPCVariant has a real implementation of that method.

I think it might be more correct to say "this only works if the variant implements GetAsJSVal". Since I imagine that you can implement that function without backing the variant with a JS val (convert lazily).

> >diff --git a/dom/base/nsHistory.cpp b/dom/base/nsHistory.cpp
> 
> >+nsHistory::PushState(nsIVariant *aData, const nsAString& aTitle,
> >+                     const nsAString& aURL)
> >+{
> >+  // Check that PushState hasn't been pref'ed off.
> >+  if (!nsContentUtils::GetBoolPref(sAllowPushStatePrefStr, PR_FALSE))
> >+    return NS_OK;
> >+
> >+  NS_ENSURE_TRUE(mDocShell, NS_ERROR_FAILURE);
> >+
> >+  // PR_FALSE tells the docshell to add a new history entry instead of
> >+  // modifying the current one.
> >+  nsresult rv = mDocShell->AddState(aData, aTitle, aURL, PR_FALSE);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+  // To play nice with bfcache, notify the session history that it might need
> >+  // to evict a content viewer
> >+  nsCOMPtr<nsISHistory> session_history;
> >+  GetSessionHistoryFromDocShell(mDocShell, getter_AddRefs(session_history));
> >+  // From the page's perspective, this isn't a failure.
> >+  NS_ENSURE_TRUE(session_history, NS_OK);
> >+
> >+  nsCOMPtr<nsISHistoryInternal> session_history_internal =
> >+    do_QueryInterface(session_history);
> >+  NS_ENSURE_TRUE(session_history_internal, NS_OK);
> >+
> >+  PRInt32 curIndex = -1;
> >+  rv = session_history->GetIndex(&curIndex);
> >+  if (NS_SUCCEEDED(rv) && curIndex > -1) {
> >+    session_history_internal->EvictContentViewers(curIndex - 1, curIndex);
> >+  }
> 
> > Can the call to AddState run any script synchronously? Such as hashchange event
> > listeners? If so, do you need to make the evict call above before such script
> > runs? So that things behave correctly if that script calls history.forward()
> > for example?
> 
> PushState and ReplaceState don't trigger a hashchange, so I think we're safe,
> unless we can think of some other event which might fire.

One solution is to add a nsAutoScriptBlocker around these calls. While that is in scope no events will fire and you'll just get an assertion instead.

> >diff --git a/dom/base/nsLocation.cpp b/dom/base/nsLocation.cpp
> >+nsLocation::GetSourceURI(nsIPrincipal* principal,
> >+                         nsIScriptSecurityManager* secMan) {
> >+  nsresult rv;
> >+
> >+  nsCOMPtr<nsIURI> principalURI;
> >+  principal->GetURI(getter_AddRefs(principalURI));
> >+
> >+  if (!principalURI)
> >+    return nsnull;
> >+
> >+  // Try to get the document of our caller.  If we can't, perhaps because our
> >+  // caller doesn't have a document, just return the principal's URI.
> >+  nsCOMPtr<nsIDOMDocument> domDoc = nsContentUtils::GetDocumentFromCaller();
> >
> > Won't this return the wrong value if someone does:
> >
> > myIframe.contentWindow.location
> >
> > Won't that get the location of the caller, rather than the location of the
> > window inside the iframe?
> >
> > [...]
> >
> >+  rv = secMan->CheckSameOriginURI(principalURI, docURI, PR_FALSE);
> > This check *should* never fail right? If so, it'd be a good idea to add an
> > assertion here.
> 
> Actually, it turns out that the document is sometimes at about:blank, and in
> at least that case, the check will fail.  Looking over this again, I think I
> can just use nsLocation::GetURI and nix the whole nsLocation::GetSourceURI
> method.

Note to self, re-review this part.

Comment 38

[Boris Zbarsky [:bzbarsky]]

> unless we can think of some other event which might fire.

OnLocationChange?

> Actually, it turns out that the document is sometimes at about:blank

Why exactly are we comparing URIs?

> It may be necessary to make some edge case work

We should have a test for said edge case, then.

> since normal navigation works fine then

Normal navigation before onload replaces the history entry for the thing before whose onload it happens, no?

Comment 39

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 418525 [details] [diff] [review]
Patch 1.4.5

>diff --git a/js/src/xpconnect/idl/nsIXPConnect.idl b/js/src/xpconnect/idl/nsIXPConnect.idl
>+//    native JSVal(jsval); // declared in nsIVariant.idl

I'd prefer you take this out entirely or replace it with just:

// NB: JSVal is declared in nsIVariant.idl.

>+    nsIVariant
>+    jSValToVariant(in JSValPtr aJSVal, in JSContextPtr cx);

Nit: Please reverse the order of the parameters to match the JS engine style (cx, rest args).

>diff --git a/js/src/xpconnect/src/xpcvariant.cpp b/js/src/xpconnect/src/xpcvariant.cpp
>-    if(NS_FAILED(variant->GetDataType(&type)))
>+    if (NS_FAILED(variant->GetDataType(&type)))
>         return JS_FALSE;

Nit: Here and below, style in XPConnect is |if(...)|.

> 
>+    jsval realVal;
>+    nsresult rv = variant->GetAsJSVal(&realVal);
>+
>+    if (NS_SUCCEEDED(rv) &&
>+       (JSVAL_IS_PRIMITIVE(realVal) ||
>+         type == nsIDataType::VTYPE_ARRAY ||
>+         type == nsIDataType::VTYPE_EMPTY_ARRAY ||
>+         type == nsIDataType::VTYPE_ID))

Über-nit: the 3 last cases here are overindented by one space.

Comment 40

[Justin Lebar (not reading bugmail)]

(In response to comment 38)
> > Actually, it turns out that the document is sometimes at about:blank
> 
> Why exactly are we comparing URIs?

I had code to check that the document's URI matched the principal's URI in nsLocation::CheckURI.  It was overkill, and I think the check failed when a document opened a new window; the new location would be about:blank, but the principal would be the old URI.  I've taken it out in the latest version of the patch.

Comment 41

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1.4.6

Addresses Blake's xpcom review, Jonas's comment 37.

Comment 42

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 418555 [details] [diff] [review]
Patch v1.4.6

Only one nit!

>diff --git a/content/base/public/nsIDocument.h b/content/base/public/nsIDocument.h
>@@ -1179,16 +1179,38 @@ public:
>     Doc_Theme_Uninitialized, // not determined yet
>     Doc_Theme_None,
>     Doc_Theme_Neutral,
>     Doc_Theme_Dark,
>     Doc_Theme_Bright
>   };
> 
>   /**
>+   * Returns the document's pending state object (serialized to JSON), or the
>+   * empty string if one doesn't exist.
>+   *
>+   * This field serves as a waiting place for the history entry's state object:
>+   * We set the field's value to the history entry's state object early on in
>+   * the load, then after we fire onload we deserialize the field's value and
>+   * return the resulting object to the document in the PopState event.

"return" isn't really technically correct (though arguably conceptually correct). I'd replace the last line with something like

"fire a PopState event containing the resulting object."


Does this still need Pauls review on the latest round of session history stuff? Or is this ready to land?

Comment 43

[Justin Lebar (not reading bugmail)]

Attachment: Last one?

The changes I made to the session restore test after Paul's review were pretty minor, but it's also the first chrome test I've ever written, so perhaps a final once-over would be in order.

Comment 44

[Justin Lebar (not reading bugmail)]

Tryserver is complaining a bit, so we need to hold off pushing until I can investigate tomorrow.

Comment 45

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 418914 [details] [diff] [review]
Last one?

Looks good, r=zpao. Thanks Justin!

Comment 46

[Justin Lebar (not reading bugmail)]

Tryserver identified a Windows build error (apparently due to moving the declaration of jsval into nsIVariant.idl) and some unit test failures (http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1261522757.1261531195.21495.gz).  I'm working on ironing these issues out ASAP.

Additionally, I'm hitting bug 531176's new assert when I call pushState from within a popState handler.  The issue is that nsDocShell::AddState calls FireOnLocationChange to enable the back button if necessary.  This is triggering an event which trips the assert.  I may be misreading the stack traces, but it appears that the URLBarSetURI call at browser.js:4132 is the culprit.

Is this event actually unsafe?  I'm having trouble telling exactly what event is being dispatched (it has message 2000, NS_USER_DEFINED_EVENT and userType string "o"), but it appears to be directed at chrome, not the page.

If the event is unsafe, we can probably run the FireOnLocationChange asynchronously, or I could perhaps figure out another way to get the back button to work.  But I'd like to be sure that we really need the extra complexity before I add it.

Comment 47

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It's definitely not safe to run scripts when there are scriptblockers on the stack. So the assertion from bug 531176 is correct.

However it could be that the scriptblocker that's currently active might not be needed. Do you know what scriptblocker is currently active? The one added in this bug perhaps?

Can you look up the callstack when the assertion fires and see who adds the scriptblocker?

Comment 48

[Boris Zbarsky [:bzbarsky]]

userType string "o" sounds unlikely.  What do the following bytes look like?

Comment 49

[Justin Lebar (not reading bugmail)]

(In reply to comment #48)
> userType string "o" sounds unlikely.  What do the following bytes look like?
Indeed.  It's actually "onValueChange".

(In reply to comment #47)
> Do you know what scriptblocker is currently active? The one added in
> this bug perhaps?
It's indeed the one added in this bug that's causing the assertion.  I guess I'll move the script blocker into the docshell code, unless we want to dispatch FireOnLocationChange asynchronously.

In other news, the patch is breaking some session restore tests (e.g. browser_491168.js).  The tests load a page into a tab, save the tab's state, and then restore it.  The restore triggers a load of type LOAD_HISTORY.  My code sees that the current page (before the restore) has the same document identifier as the new page (after the restore) and so treats the load like navigation between two pushState'd entries.  For some reason, this prevents the SSTabRestored event from firing (it might be waiting for the page to fire an onload), and the test times out.

I think my code is doing the right thing here; I just need to figure out how to get that SSTabRestored event to fire (perhaps by firing it when the page fires its popstate event).  I'll continue to look into this over the next few days.

Comment 50

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #49)
> In other news, the patch is breaking some session restore tests (e.g.
> browser_491168.js).  The tests load a page into a tab, save the tab's state,
> and then restore it.  The restore triggers a load of type LOAD_HISTORY.  My
> code sees that the current page (before the restore) has the same document
> identifier as the new page (after the restore) and so treats the load like
> navigation between two pushState'd entries.  For some reason, this prevents the
> SSTabRestored event from firing (it might be waiting for the page to fire an
> onload), and the test times out.
> 
> I think my code is doing the right thing here; I just need to figure out how to
> get that SSTabRestored event to fire (perhaps by firing it when the page fires
> its popstate event).  I'll continue to look into this over the next few days.

SSTabRestored is fired after a load event. The listener is attached here (restoreDocument_proxy fires SSTabRestored): http://hg.mozilla.org/mozilla-central/file/b7ee0f1acfef/browser/components/sessionstore/src/nsSessionStore.js#l2158

Comment 51

[Justin Lebar (not reading bugmail)]

(In reply to comment #50)
> SSTabRestored is fired after a load event. The listener is attached here
> (restoreDocument_proxy fires SSTabRestored):
> http://hg.mozilla.org/mozilla-central/file/b7ee0f1acfef/browser/components/sessionstore/src/nsSessionStore.js#l2158

Thanks, Paul.  I hacked it last night to fire SSTabRestored after popstate instead of after load.  This fixed the hang, but the tab's data still wasn't being restored properly, due to the fact that the loads were being short-circuited by my code.

I've modified the sessionstore's loads to have type nsIDocShellLoadInfo::loadReloadNormal instead of loadHistory.  This prevents the short-circuiting, although it feels like a hack and I'm not sure if it'll introduce additional problems.  I'm testing on try right now.

Comment 52

[Justin Lebar (not reading bugmail)]

Attachment: Patch v.1.4.8

Three modifications in this patch:

 * Per comment 51, the loads generated by session restore now have type reloadNormal instead of loadHistory.

 * If during a load the new history entry has the same id as the current entry (this id is a separate value from the session history entry's document identifier), we don't do a short-circuited pushState-type load.  This is to allow history.go(0) to perform a full-fledged load.

 * I moved the FireOnLocationChange call out of the docshell and into nsHistory so the script blocker could sit around the call to nsDocShell::AddState.  (I was going to move the script blocker into AddState, but the docshell can't access nsContentUtils, where all the script blocking code lives.)

This patch is (finally) green on Linux and OSX try, but Windows try is giving me compile errors.  It builds fine on my Windows box, so I suspect the problem may be that try isn't doing a sufficiently-clean rebuild.  (See http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1262025022.1262026903.29606.gz)

Paul, could you have a look and tell me if the session history change I made looks safe?

Comment 53

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 419387 [details] [diff] [review]
Patch v.1.4.8

>-        browser.webNavigation.gotoIndex(activeIndex);
>+        browser.webNavigation.gotoIndexForceLoad(activeIndex);

I don't see gotoIndexForceLoad anywhere in our code or your patch... I think you meant gotoIndexAsRefresh?

Will this have any effect on loading pages from bfcache? I'm pretty sure we'll reload from bfcache right now and don't want to break that.

Are there any other consumers of gotoIndex that might have depended on the load event being triggered?

Comment 54

[Justin Lebar (not reading bugmail)]

(In reply to comment #53)
> I don't see gotoIndexForceLoad anywhere in our code or your patch... I think
> you meant gotoIndexAsRefresh?
Argh; find/replace fail.  Yes, that's what I meant.

> Will this have any effect on loading pages from bfcache? I'm pretty sure we'll
> reload from bfcache right now and don't want to break that.
I think the code as it was would reload from bfcache, but I dunno if bfcache normally gets cleared before a restore.  If I open a tab to http://stanford.edu/~jlebar/moz/history/onload.html , close it, and then press ctrl+shift+t to restore the tab, I get an onload event.  Does that mean we're not restoring from bfcache?

> Are there any other consumers of gotoIndex that might have depended on the 
> load event being triggered?
Possibly.  I think tryserver stops after some number of test failures, so I wouldn't necessarily have seen them.

An alternative to this approach is to change the document IDs of the session history entries when we restore them.  We actually already to this for the session history entries' regular IDs: When a SHEntry is restored, it gets a new ID, chosen to be unique within its frame.  (See http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#2203)

I thought about doing something like this for the document IDs.  It might be less disruptive.  The only trick is, it's a security issue for two history entries to incorrectly have the same document ID (an attacker could load bank.com and evil.com with the same document ID and then navigation forward from evil.com's history entry to bank.com's history entry wouldn't change the document, and evil.com's script would continue to run, but the URL would be bank.com).

I'm not sure if the document IDs need to be globally unique or just unique within a tab.  It might be tricky to guarantee that they're always globally unique if we're changing them on session restore.

Comment 55

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #54)
> > Will this have any effect on loading pages from bfcache? I'm pretty sure we'll
> > reload from bfcache right now and don't want to break that.
> I think the code as it was would reload from bfcache, but I dunno if bfcache
> normally gets cleared before a restore.  If I open a tab to
> http://stanford.edu/~jlebar/moz/history/onload.html , close it, and then press
> ctrl+shift+t to restore the tab, I get an onload event.  Does that mean we're
> not restoring from bfcache?

I think it is. I usually just disconnect from the Internet, then restart Firefox. I think that forces bfcache... (or is it just normal cache?).

I just opened that link after going offline and I got the onload event. I don't get it if I press back in a browser to go to the page though. So maybe it's not getting restored from bfcache (I honestly don't know - perhaps somebody like Jonas or Boris can say better).

> > Are there any other consumers of gotoIndex that might have depended on the 
> > load event being triggered?
> Possibly.  I think tryserver stops after some number of test failures, so I
> wouldn't necessarily have seen them.

I just want to make sure. It seems like behavior that some other things might depend on.

> An alternative to this approach is to change the document IDs of the session
> history entries when we restore them.  We actually already to this for the
> session history entries' regular IDs: When a SHEntry is restored, it gets a new
> ID, chosen to be unique within its frame.  (See
> http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#2203)
> 
> I thought about doing something like this for the document IDs.  It might be
> less disruptive.  The only trick is, it's a security issue for two history
> entries to incorrectly have the same document ID (an attacker could load
> bank.com and evil.com with the same document ID and then navigation forward
> from evil.com's history entry to bank.com's history entry wouldn't change the
> document, and evil.com's script would continue to run, but the URL would be
> bank.com).

Yea, we don't want that to happen.

> I'm not sure if the document IDs need to be globally unique or just unique
> within a tab.  It might be tricky to guarantee that they're always globally
> unique if we're changing them on session restore.

That's something I'd trust your input on (or somebody else who's familiar with this code). I'm not super familiar with the workings of content/docshell internals. Simon wrote the sessionstore code for setting the shEntry.id, perhaps he'd have some insight.

Comment 56

[Boris Zbarsky [:bzbarsky]]

> Does that mean we're not restoring from bfcache?

In _Firefox_, undo tab close restores from the session store stuff, not from bfcache.  In Seamonkey, I believe it uses bfcache.

Restarting Firefox always drops all information from bfcache: bfcache is a purely in-memory data structure.

> It might be tricky to guarantee that they're always globally unique if we're
> changing them on session restore.

Can't you just give documents sequential 64-bit integer ids?  They'd be unique during any particular Firefox process execution and not persist across session restore.  Is that what we want?

Comment 57

[Justin Lebar (not reading bugmail)]

(In reply to comment #56)
> > It might be tricky to guarantee that they're always globally unique if we're
> > changing them on session restore.
> 
> Can't you just give documents sequential 64-bit integer ids?  They'd be unique
> during any particular Firefox process execution and not persist across session
> restore.  Is that what we want?

To a first approximation, we want the invariant that session history entries X and Y have the same document identifier iff X and Y correspond to the same document to persist across session restore.

Actually, it's good enough if the invariant only applies to X and Y such that the user may navigate from X to Y.  I think this is equivalent to saying that X and Y belong to the same window (tab?), but I'm not sure.

I've modified the patch under the assumption that uniqueness within a window is sufficient to reassign IDs.  If the assumption is correct, I think this is a safer approach than my previous approach of forcing reloads on session restore, since it doesn't modify existing functionality.  I hope to post it soon; I have a few unrelated issues to work out.

Comment 58

[Simon Bünzli]

(In reply to comment #55)
> Simon wrote the sessionstore code for setting the shEntry.id,
> perhaps he'd have some insight.

IIRC, shEntry.ID is used to determine which frames to reload during subframe navigation. It's - as Boris suggested - a sequential integer id, so the reasonable thing seemed to reset them to an arbitrarily high value when we needed them to be identical (which in retrospective seems like a fragile hack; Paul: if these IDs are indeed sequential, a mapping from old-session-ID to current-session-ID for just resetting the IDs of subsequent frames would have been better).

Comment 59

[Boris Zbarsky [:bzbarsky]]

> we want the invariant that session history entries X and Y have the same
> document identifier iff X and Y correspond to the same document to persist
> across session restore.

OK...  In that case I'm not quite sure what the problem is.

Comment 60

[Justin Lebar (not reading bugmail)]

(In reply to comment #59)
> > we want the invariant that session history entries X and Y have the same
> > document identifier iff X and Y correspond to the same document to persist
> > across session restore.
> 
> OK...  In that case I'm not quite sure what the problem is.

Suppose we have a tab with one history entry, X.  We then serialize the tab's history, deserialize it, and restore the history into the tab.  This triggers a load in the docshell, where the current history entry is X, and the new history entry is, say, X'.  If X and X' have the same doc identifier, then we won't dispatch an onload event after we load X', since navigation between X and X' is like navigation between foo.html and foo.html#bar.  (We would fire a popstate event in this case.)  A number of session history tests rely on this load event being dispatched.

One solution is to have the session history notify the docshell that the current load shouldn't short-circuit like a navigation from foo.html and foo.html#bar.  That's what I do in the current patch, by having the session history trigger a load of type nsIDocShellLoadInfo::loadReloadNormal.  But Paul and I are concerned that this might break existing functionality.

An alternative is to make the doc identifier of X' different from that of X.  But now the question is, how unique does X''s doc identifier need to be?  Is it OK if it's unique only within a tab, or does it need to be globally unique?  It's bad if a page could force a doc identifier collision and then navigate from one of the affected history entries to another of the affected entries, because then one of the pages could masquerade as another, potentially in another origin.  But if you can never navigate between them, I don't think it matters if two history entries have the same doc identifier.

Comment 61

[Boris Zbarsky [:bzbarsky]]

> If X and X' have the same doc identifier

Why would they, though?  How does session restore handle this for shentry ids right now?  Why wouldn't we handle it the same way for doc identifiers?

> Is it OK if it's unique only within a tab, or does it need to be globally
> unique? 

The latter, I would think.  Various extensions move shentries around between docshells, if nothing else.

Comment 62

[Justin Lebar (not reading bugmail)]

(In reply to comment #61)
> > If X and X' have the same doc identifier
> 
> Why would they, though?  How does session restore handle this for shentry ids
> right now?  Why wouldn't we handle it the same way for doc identifiers?

X and X' would have the same doc identifier if we set X''s doc identifier to that of X on session restore.  That's what I do in the patch that's currently posted.

Here's the relevant code for how session restore handles shentry ids: http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#2203  I don't think this guarantees global uniqueness, only uniqueness within a frame.

> > Is it OK if it's unique only within a tab, or does it need to be globally
> > unique? 
> 
> The latter, I would think.  Various extensions move shentries around between
> docshells, if nothing else.

Okay.  I'll see if I can come up with something which gives us global uniqueness.

Comment 63

[Boris Zbarsky [:bzbarsky]]

> I don't think this guarantees global uniqueness,

Depends on what the scope of aIdMap is (but ideally it would ask shistory for the id anyway), no?

Comment 64

[Justin Lebar (not reading bugmail)]

(In reply to comment #63)
> > I don't think this guarantees global uniqueness,
> 
> Depends on what the scope of aIdMap is (but ideally it would ask shistory for
> the id anyway), no?

I think the shentry ids are unique within one call to restoreHistoryPrecursor.  e.g. duplicateTab [1] calls restoreHistoryPrecursor which creates an empty idMap [2].

But yes, I think we should just ask the shistory to give us a globally unique doc identifier.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#987
[2] http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#2057

Comment 65

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #55)
> (In reply to comment #54)
> > > Will this have any effect on loading pages from bfcache? I'm pretty sure > > > we'll reload from bfcache right now and don't want to break that.
> > I think the code as it was would reload from bfcache, but I dunno if
> > bfcache normally gets cleared before a restore.  If I open a tab to
> > http://stanford.edu/~jlebar/moz/history/onload.html , close it, and then
> > press ctrl+shift+t to restore the tab, I get an onload event.  Does that
> > mean we're not restoring from bfcache?
> 
> I think it is.

That is correct.

> I usually just disconnect from the Internet, then restart
> Firefox. I think that forces bfcache... (or is it just normal cache?).

If you're disconnected from the internet but can still access a resource it could be coming from either the normal cache, or from the bfcache.

> I just opened that link after going offline and I got the onload event. I
> don't get it if I press back in a browser to go to the page though. So maybe
> it's not getting restored from bfcache (I honestly don't know - perhaps
> somebody like Jonas or Boris can say better).

If you get an onload event then the page was not loaded from bfcache. Note that we're only loading things from the bfcache if you navigate to it using the back or forward buttons. We don't use it if you navigate using a link or by typing an address.

My recollection is that we don't use it if you use the "undo close tab" feature. This seems to match what bz is saying.

Comment 66

[Simon Bünzli]

(In reply to comment #62)
> I don't think this guarantees global uniqueness, only uniqueness within a
> frame.

It doesn't guarantee it, but at least makes it highly probable, since we start counting at Date.now() which will be a unique number unless you use the same API twice in very quick succession (less milliseconds than we had shEntries to restore). Of course, this hack should be fixed sooner or later...

Comment 67

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1.5

This one gives globally unique doc identifiers on tab restore.  It also fixes some intermittent test failures and moves the script blocker into the docshell.

I got errors on try, but completely different errors on each OS (none of which I can reproduce locally), so I'm hoping they're unrelated.  I'll try again once the tree is green.

Paul, could you have one last look at the shistory code?  Jonas, could you check nsDocShell::AddState now that I moved the script blocker there?

Comment 68

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 420086 [details] [diff] [review]
Patch v1.5

Mostly good. Just a few things.

>     var idMap = { used: {} };
>-    this.restoreHistory(aWindow, aTabs, aTabData, idMap);
>+    var docIdentMap = { used: {} };

You aren't using .used at all (since you're generating the identifier outside of sessionstore), so you don't need that in the object.

>-  restoreHistory: function sss_restoreHistory(aWindow, aTabs, aTabData, aIdMap) {
>+  restoreHistory: function sss_restoreHistory(aWindow, aTabs, aTabData, aIdMap, aDocIdentMap) {

Nit: Please break this up (see _serializeSessionStorage).

>-  _deserializeHistoryEntry: function sss_deserializeHistoryEntry(aEntry, aIdMap) {
>+  _deserializeHistoryEntry: function sss_deserializeHistoryEntry(aEntry, aIdMap, aDocIdentMap) {

Nit: break this up too.

>+    if (aEntry.docIdentifier) {
>+      // Get a new document identifier for this entry to ensure that history
>+      // entries after a session restore are considered to have different
>+      // documents from the history entries before the session restore.
>+      //
>+      // It's a potential security issue if before a session restore, two
>+      // history entries have two different doc identifiers and after the
>+      // restore, those history entries have the same doc identifier, so we
>+      // need to be careful here.

We are being careful, so I don't think we need that 2nd half of the comment. You could reduce it to mentioning that we generate a unique identifier. (and get rid of the blank comment line)

>+      var ident = aDocIdentMap[aEntry.docIdentifier] || 0;

Don't need the  || 0 here. It would be undefined (falsy) if not found.
Also, s/var/let/

>+      if (ident) {
>+        shEntry.docIdentifier = ident;
>+      }
>+      if (!ident) {

Use else {

>+        shEntry.setUniqueDocIdentifier();
>+        aDocIdentMap[aEntry.docIdentifier] = shEntry.docIdentifier;

I imagine it should be very difficult to hit this case, but JavaScript doesn't properly handle 64bit values. So we'll lose precision at some large doc ident and then we won't be setting things properly (I think we'd be setting the same doc ident for everything after that point). So I would at the very least add a comment about that with the other comments, in case we start seeing bugs that might be caused by that.

>+      }
>+    }
>+
>+    if (aEntry.stateData) {
>+      shEntry.stateData = aEntry.stateData;
>+    }
>+

Nit: please move this whole block up with the aEntry.id block (since they are similar).

r+ with these fixes

Comment 69

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If something is important for security, but it's obvious that it is, I would really appreciate a comment indicating so. These things tend to be forgotten over time.

However I'm definitely not married to any particular wording.

Comment 70

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #69)
> If something is important for security, but it's obvious that it is, I would
> really appreciate a comment indicating so. These things tend to be forgotten
> over time.

True. I guess I'm just not a fan of the wording - it seems to imply that there is a security issue. Justin - just say something more along the lines of "we're doing X to mitigate security issue Y". Jonas is right - better to have a comment than none at all.

Comment 71

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1.5.1

Addressing Paul's last review (comment 68).

Comment 72

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

How does this build? nsAutoScriptBlocker lives in gklayout and nsDocShell does not. This should generate a linker error at least on some platforms. Did you try to push this to tryserver?

Comment 73

[Justin Lebar (not reading bugmail)]

(In reply to comment #72)
> How does this build? nsAutoScriptBlocker lives in gklayout and nsDocShell does
> not. This should generate a linker error at least on some platforms. Did you
> try to push this to tryserver?

I was pretty surprised too that it worked.  The last time I tried to use nsContentUtils in the docshell, it definitely didn't link.  I noticed that I now have to link a lot more directories when I change the docshell than I used to, so I figured that might have had something to do with why it worked this time.

The tryserver build did fail on winmo -- I wrote this off as tryserver weirdness, but I'll spin it again and have a closer look at the logs.

Comment 74

[Boris Zbarsky [:bzbarsky]]

It still shouldn't link (except _maybe_ in a libxul build, but even there it's pretty odd).

Comment 75

[Justin Lebar (not reading bugmail)]

I pushed again to try and the patch built on all platforms (including Maemo).  I've filed bug 540235 for this behavior.

Comment 76

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Justin: I'd recommend not depending on having nsDocShell link to nsContentUtils as things are set up now. It might be that this works in all tinderboxes, but it's definitely possible to build locally in such a way that this fails. I do personally and I'd be surprised if not many others do too. And I'd hate to get into arguments about this patch over this.

I think the simplest solution is to add back nsIContentUtils :(

Comment 77

[Justin Lebar (not reading bugmail)]

(In reply to comment #76)
> I think the simplest solution is to add back nsIContentUtils :(
Oh, I'd forgotten about that!  Yes, I think that's probably the simplest thing to do.

It would be good if I could access nsAutoScriptBlocker, not just the add/remove script blocker methods.  Perhaps I could move the nsAutoScriptBlocker declaration out of nsContentUtils.h and into a header file somewhere else?

Comment 78

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Also, I think the scoping of the nsAutoScriptBlocker is wrong. Wasn't the idea to make sure that we don't run script until after |internalSH->EvictContentViewers| is called?

That in order to ensure that we are in the correct state when the script is running, and to make sure that we don't get messed up in case the script calls history.back() or some such.

Comment 79

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I.e. i think the scope needs to be expanded to contain "Step 5" as well.

Comment 80

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #77)
> (In reply to comment #76)
> > I think the simplest solution is to add back nsIContentUtils :(
> Oh, I'd forgotten about that!  Yes, I think that's probably the simplest thing
> to do.
> 
> It would be good if I could access nsAutoScriptBlocker, not just the add/remove
> script blocker methods.  Perhaps I could move the nsAutoScriptBlocker
> declaration out of nsContentUtils.h and into a header file somewhere else?

Hmm.. the tricky part is that we don't want all current nsAutoScriptBlocker users to go through the virtual nsIContentUtils.

Maybe put a nsAutoScriptBlockerExternal class in nsIContentUtils.h

Comment 81

[Justin Lebar (not reading bugmail)]

(In reply to comment #78)
> Also, I think the scoping of the nsAutoScriptBlocker is wrong. Wasn't the idea
> to make sure that we don't run script until after
> |internalSH->EvictContentViewers| is called?

Do we want to not run scripts until after EvictContentViewers, or until right before EvictContentViewers?  It looks like it doesn't make much difference, since EvitctContentViewers doesn't trigger the script blocker's assert if I move it into the blocker's scope, but is it right to assert that EvictContentViewers will never run script?

Perhaps the scope should be right up to but not including the call to EvictContentViewers.  The other step 5 code really doesn't do much, but I suppose the whole point here is better safe than sorry.

Comment 82

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

nsAutoScriptBlocker going out of scope can run script. While the scriptblocker is live we can queue up scriptrunners using nsContentUtils::AddScriptRunner, which will then run as soon as the last script blocker is removed.

If EvictContentViewers can run scripts. And if none of the code that's currently being called during the current nsAutoScriptBlocker needs to call nsContentUtils::AddScriptRunner, then you can use nsContentUtils::AddScriptBlockerAndPreventAddingRunners rather than nsContentUtils::AddScriptBlocker.

Does that make sense?

Does EvictContentViewers run script? I guess it might run "onunload" handlers?

Comment 83

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1.5.2

Adding nsIContentUtilsExternal, nsAutoScriptBlockerExternal, and moving step 5 into the script blocker.

Smaug tells me that EvictContentViewers might run scripts, so we'll have to take another look at this.

Comment 84

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ugh, given that EvictContentViewers might run script, I wonder if using scriptblockers is the right thing to do.

script blockers are really intended when we want to execute something that we know sometimes needs to schedule scripts to run, but we want to delay those scripts until we're past a certain critical section.

However here delaying scripts doesn't really work since we'd need the critical section to span past EvictContentViewers, but since EvictContentViewers runs script it also can't be inside the critical section.

Additionally, some of the code inside the currently marked critical section is the JSON code, which we know can run script, and where we also can't delay the script running.


Looking at the code again I don't see what could actually run script, apart from the JSON serialization. But it seems like it's running before we do any session history stuff so it should be fine if session history changes while that runs. None of the other code seems to risk running script.


So as long as we're fine with AddState running script, I think we're fine just removing the scriptblocker stuff. Might want to add a crashtest where the JSON serialization calls history.back() or sets document.location or something else evil. Just to make sure it doesn't crash.

Comment 85

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ultimately we shouldn't be using JSON serialization, but rather the structured-clone code. At which point there should be no risk of script running. But lets leave that for later.

Comment 86

[Justin Lebar (not reading bugmail)]

Attachment: Patvh v1.5.3

Ugh indeed.

Script blockers are gone.  Added a crashtest which passes to pushState an object which calls history.back() in its toJSON() function.

Comment 87

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Given that AddState can run script, can you at the callsite hold a local strong reference to the docshell (this is actually standard XPCOM rules, though we cheat *a lot*).

So something like:

nsCOMPtr<nsIDocShell> docShell = mDocShell;
docShell->AddState(...);

Also add a comment that you're doing this to prevent the docshell from going away since AddState can run script.

with that, r=me! Woot!!!

Comment 88

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1.5.4

Adding strong references per Jonas's comment.  Pushed (hopefully) one last time to try.  Crossing my fingers.

Comment 89

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1.5.4 rebased, ready for checkin

Wow.  I think this is actually ready.  Tryserver was green.

Comment 90

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Couldn't wait, so I pushed it :). Had to do some trivial merging

http://hg.mozilla.org/mozilla-central/rev/dc7a04be6904

Comment 91

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Adjusting summary since clearState wasn't implemented (and is no longer part of the spec), but replaceState is.

Comment 92

[Daniel Holbert [:dholbert]]

This may have triggered bug 543034, an apparent MSVC compiler bug.  It looks like *some* change (in the 24 hours before today's nightly) has made us start triggering that bug, and this looks like the most likely at the moment.  (see bug 543034 comment 3)

I'm backing this out and re-spinning today's (busted) nightly, to test that theory.

Comment 93

[Daniel Holbert [:dholbert]]

Backed out, per comment 92:
http://hg.mozilla.org/mozilla-central/rev/6d50455cabaa
http://hg.mozilla.org/mozilla-central/rev/b0b9d8dca9d6

joduinn is now respinning the nightly, to see if the backout has any effect on bug 543034... Stay tuned.

Comment 94

[Daniel Holbert [:dholbert]]

The nightly build after the backout was green, as noted in bug 543034 comment 8. So, looks like this was indeed the 'guilty' changeset (guilty only of triggering a fatal bug in Microsoft's compiler).

Can the relevant developers on this bug double-check the nsIVariant-related changes here for anything broken, particularly in any nsIVariant code that might get used by nsAnnotationService::SetItemAnnotation?  (If we're triggering an edge case in MSVC, it makes me think maybe we're doing something unexpected/unintended... though maybe MSVC's just broken.)

Any attempt to re-land this should probably be followed by an immediate kick of the Windows nightly builder (with help from #build), so that we don't have to wait until the next morning to find out whether it triggers bug 543034 again.

Comment 95

[Justin Lebar (not reading bugmail)]

dholbert told me on IRC that this is failing in the PGO step of the build on the nightly box.

Theoretically, the nightly Windows builds should be the same as Windows tryserver opt builds, although the patch builds just fine on try.  But it appears something is different.

Comment 96

[Boris Zbarsky [:bzbarsky]]

try doesn't do pgo.

Comment 97

[Daniel Holbert [:dholbert]]

Then, in comment 95:
  s/tryserver opt builds/tinderbox opt builds/
  s/builds just fine on try/builds just fine on non-nightly opt builds/
(Ted told me in IRC that there's nothing substantially different between the tbox opt vs nightly builders)

Comment 98

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #96)
> try doesn't do pgo.

Are you sure? I recently enquired about why tryserver on windows takes so long and was told it was because of pgo.

Comment 99

[Boris Zbarsky [:bzbarsky]]

I'm not 100% sure, but it didn't use to as of a few weeks ago...

Comment 100

[Daniel Holbert [:dholbert]]

(In reply to comment #94)
> The nightly build after the backout was green, as noted in bug 543034 comment
> 8. So, looks like this was indeed the 'guilty' changeset (guilty only of
> triggering a fatal bug in Microsoft's compiler)

After more investigation, looks like this checkin wasn't guilty after all, but was just unlucky enough to sporadically get a green cycle after its backout.  A day and a half after the backout, a different build machine hit the same compiler bug, and since then we've had some distinct-but-related-looking failures.  (See Bug 543034 Comment 13 thru 18.)

So, as the sheriff responsible for backing out, I'd say this is clear to re-land.

Comment 101

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Pushed to m-c again

http://hg.mozilla.org/mozilla-central/rev/a2f468118868

Comment 102

[Eric Shepherd [:sheppy]]

What is the "data" argument's value intended to be to these methods? The spec is unclear on the matter. Is it arbitrary data or is it intended to have specific content?

I've added these new methods here:

https://developer.mozilla.org/en/DOM/window.history

And wrote a new article about using the history object here:

https://developer.mozilla.org/en/DOM/Manipulating_the_browser_history

The latter won't be considered complete until I know the answer to my question above.

Comment 103

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It is just a opaque object that we store and pass back in the popstate event. So the page can use it for anything it wants.

We do require that the object can be serialized using JSON though. I.e. you can't pass in a DOM object for example.

Comment 104

[Eric Shepherd [:sheppy]]

The documentation has been updated with that information and I now consider it to be complete; please let me know if there are errors (or, better, fix them):

https://developer.mozilla.org/en/DOM/window.history
https://developer.mozilla.org/en/DOM/Manipulating_the_browser_history

Comment 105

[Ed Lee :Mardak]

There should probably be documentation for window.onpopstate to describe how to make use of data added by pushState and replaceState. Just looking at the entries for window.history, I was quite confused what these new functions would be used for.

Comment 106

[Marco Bonardo [:mak]]

any reason for this bug to be still open since the patch has been pushed?

Comment 107

[Eric Shepherd [:sheppy]]

popstate event docs have been added:

https://developer.mozilla.org/en/DOM/window.onpopstate
https://developer.mozilla.org/en/DOM/Manipulating_the_browser_history#State_information_for_history_entries