Bug 33224

whatwg discussion here: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2010-January/024675.html

(In reply to comment #0) > popstate event is only fired when navigating back to a live document > > according to > http://dev.w3.org/html5/spec/Overview.html#activating-state-object-entries, the > popstate event should be fired after the load event. the implication is that > it should be possible to reload the URL, and then fire the popstate event. 5.10.1 states: "User agents may discard the Document objects of entries other than the current entry that are not referenced from any script, reloading the pages afresh when the user or script navigates back to such pages. This specification does not specify when user agents should discard Document objects and when they should cache them." 5.11.9.1 then states: "If there is no longer a Document object for the entry in question, the user agent must navigate the browsing context to the location for that entry to perform an entry update of that entry, and abort these steps." So your suggestion would *only* work in the case of a reload, *if* reload behavior is specified to reuse the same entry and same document. I'm looking through the spec to find out if this is the case but am so far coming up blank. Do you have a reference? I disagree that the language is an implication for the behavior you describe for another reason - there's another possibility where the language makes sense. If - while a document is loading - a script does a pushstate() followed by a history.back(), all before the document is loaded, then the popped state object will become the pending state object and the event will be fired after the load event. I coded for this case when developing the initial feature and thought I had a layout test that covered it. In experimenting before making this comment, I have proven that this appears to be broken and I've filed https://bugs.webkit.org/show_bug.cgi?id=33538 to cover fixing it. > fast/loader/stateobjects/document-destroyed-navigate-back.html seems to assert > this "incorrect" behavior. This seems *not* true, as this layout test does not describe the reload case, and therefore definitely plays along with the different-document-allowed-to-clear-at-any-time model described in the spec. > I think this bug results from having HistoryItem objects store a pointer to a > Document. Instead, I think they should probably have a "group id" or a > "document id" that can be used to know that they are all associated with the > same document even if the actual Document object changes. For the reasons I lay out above, I don't agree. The interesting thing I think we need an answer on is what the spec intends for the reload case. WebCore certainly uses different documents internally for a reload and adopting different behavior would have wide-ranging implications.

> 5.11.9.1 then states: > "If there is no longer a Document object for the entry in question, the user > agent must navigate the browsing context to the location for that entry to > perform an entry update of that entry, and abort these steps." The navigate algorithm (section 5.11.1) ends with "Some of the sections below, to which the above algorithm defers in certain cases, require the user agent to update the session history with the new page. When a user agent is required to do this, it must queue a task to run the following steps: 1) Unload the Document object of the current entry, with the recycle parameter set to false. 2) If the navigation was initiated for entry update of an entry 1) Replace the entry being updated with a new entry representing the new resource and its Document object and related state. The user agent may propagate state from the old entry to the new entry (e.g. scroll position). 2) Traverse the history to the new entry." Notice step 2.2 there. It says to repeat the steps at 5.11.9.1, given that we now have a live document associated with the session history entry, we can proceed with the history traversal algorithm. The reload case is a compelling argument for this bug. It seems pretty lame if all of the pushed state gets discarded by the act of reloading the document. > If - while a document is loading - a script does a pushstate() followed by a > history.back(), all before the document is loaded, then the popped state object > will become the pending state object and the event will be fired after the load > event. Good point. > > fast/loader/stateobjects/document-destroyed-navigate-back.html seems to assert > > this "incorrect" behavior. > > This seems *not* true, as this layout test does not describe the reload case, > and therefore definitely plays along with the > different-document-allowed-to-clear-at-any-time model described in the spec. That layout test asserts that navigating back to a session history entry for which there is no live document causes the pushed state to be discarded. Am I really misunderstanding?

(In reply to comment #3) > > 5.11.9.1 then states: > > "If there is no longer a Document object for the entry in question, the user > > agent must navigate the browsing context to the location for that entry to > > perform an entry update of that entry, and abort these steps." > > The navigate algorithm (section 5.11.1) ends with > > "Some of the sections below, to which the above algorithm defers in > certain cases, require the user agent to update the session history > with the new page. When a user agent is required to do this, it must > queue a task to run the following steps: > > 1) Unload the Document object of the current entry, with the recycle > parameter set to false. > > 2) If the navigation was initiated for entry update of an entry > > 1) Replace the entry being updated with a new entry representing > the new resource and its Document object and related state. The > user agent may propagate state from the old entry to the new > entry (e.g. scroll position). Step 2.1 states "replace the entry being updated with a new entry representing the new resource and its Document object and related state." It doesn't say the new entry is somehow a clone of the old entry, but rather an original entity representing the new Document object and its resource. I believe "state" in this context is not referring to "state objects", but rather "persisted user state" as set forth in 5.11.9 step 3: "For example, some user agents might want to persist the scroll position, or the values of form controls." This is supported by the second sentence of step 2.1: "The user agent may propagate state from the old entry to the new entry (e.g. scroll position)." We should probably file a bug on the docs to standardize on the term "persisted user state" in these situations. > 2) Traverse the history to the new entry." > > Notice step 2.2 there. It says to repeat the steps at 5.11.9.1, given > that we now have a live document associated with the session history > entry, we can proceed with the history traversal algorithm. Indeed. But this is a new live document, associated with a *NEW* session history entry, per step 2.1 which says we're now working with a new replacement entry. > The reload case is a compelling argument for this bug. It seems pretty > lame if all of the pushed state gets discarded by the act of reloading > the document. Users commonly hit the reload button in 2 cases: 1 - When they want to refresh the page to check for new content. 2 - When they think the current state of the page is broken and they want to "start over" In neither of those cases does the user care about preserving the state of the old page. > > > fast/loader/stateobjects/document-destroyed-navigate-back.html seems to assert > > > this "incorrect" behavior. > > > > This seems *not* true, as this layout test does not describe the reload case, > > and therefore definitely plays along with the > > different-document-allowed-to-clear-at-any-time model described in the spec. > > That layout test asserts that navigating back to a session history entry for > which there is no live document causes the pushed state to be discarded. For clarification, the state object is discarded not when navigating back to the old document's URL, but when the navigating *away* from the old document and everything associated with it is destroyed. > Am I really misunderstanding? I could've been more clear - That layout test *does* indeed assert that in WebCore, right now, navigating back to a session history entry for which there is no live document does not result in any popstate event being fired. We both agree on that. I'm saying that this is compatible with the spec, and you disagree. :)

I've updated the spec.

Created attachment 46891 [details] work in progress

Comment on attachment 46891 [details] work in progress > Index: WebCore/history/BackForwardListChromium.cpp > =================================================================== > --- WebCore/history/BackForwardListChromium.cpp (revision 53388) > +++ WebCore/history/BackForwardListChromium.cpp (working copy) > @@ -50,13 +50,13 @@ BackForwardList::~BackForwardList() > ASSERT(m_closed); > } > > -void BackForwardList::addItem(PassRefPtr<HistoryItem> prpItem) > +void BackForwardList::addItem(PassRefPtr<HistoryItem> newItem) > { > - ASSERT(prpItem); > + ASSERT(newItem); > if (m_capacity == 0 || !m_enabled) > return; > > - m_client->addItem(prpItem); > + m_client->addItem(newItem); > } These changes seem unnecessary - the prp idiom is common within WebCore/Kit and this seems to just kludge the patch. > void BackForwardList::pushStateItem(PassRefPtr<HistoryItem> newItem) > { > - // FIXME: Need to implement state support for chromium. > + RefPtr<HistoryItem> current = m_client->currentItem(); > + > + addItem(newItem); > + > + if (!current->stateObject()) > + current->setStateObject(SerializedScriptValue::create()); > } I know this patch isn't the right time to address this, but I feel it would be a better design if the BackForwardList organization was with a class of shared functionality that split off the truly different methods into their own platform specific files. Like, if "getCurrentItem()" had a base implementation that everyone shared, but Chromium implemented their own with the client call. Then addItem() had a normal impl. and a Chromium impl, then this "pushStateItem()" method could be 100% shared and its logic could not possibly get out of sync between the ports. Like I said, something to be addressed another time... > > HistoryItemVector& BackForwardList::entries() > Index: WebCore/history/HistoryItem.cpp > =================================================================== > --- WebCore/history/HistoryItem.cpp (revision 53388) > +++ WebCore/history/HistoryItem.cpp (working copy) > @@ -33,9 +33,18 @@ > #include "PageCache.h" > #include "ResourceRequest.h" > #include <stdio.h> > +#include <wtf/CurrentTime.h> > > namespace WebCore { > > +static long long generateDocumentSequenceNumber() > +{ > + // Initialize to the current time to reduce the likelihood of generating > + // identifiers that overlap with those from past/future browser sessions. > + static long long next = static_cast<long long>(currentTime() * 1000000.0); > + return ++next; > +} *every* platform might want to get a full serialization of the session history in the future, that's for sure. But I don't like this. Can't we come up with a better way to get unique IDs across launches? And have them truly be unique and not just "probably" be unique? > + > Index: WebCore/loader/FrameLoader.cpp > =================================================================== > --- WebCore/loader/FrameLoader.cpp (revision 53388) > +++ WebCore/loader/FrameLoader.cpp (working copy) > @@ -3686,7 +3688,7 @@ Frame* FrameLoader::findFrameForNavigati > > void FrameLoader::navigateWithinDocument(HistoryItem* item) > { > - ASSERT(!item->document() || item->document() == m_frame->document()); > + ASSERT(item->documentSequenceNumber() == history()->currentItem()->documentSequenceNumber()); It's easy for logic to recognize that a sequence of historyitems with "0" as their sequence number are actually unrelated documents. It's harder to make sure that every history item has the right sequence number relationship with its neighbors before entering "navigateWithinDocument." I ran into at least a few edge cases in my initial implementation where that problem was the case, hence the allowance for a null document before your patch. I see you attempted to get at least one of these cases, but I remember (vaguely) that there were a handful. It might be better to retain the "null sequence number" that was representable by the null document pointer, and retain the full assertion here. > Index: WebCore/loader/FrameLoaderClient.h > =================================================================== > --- WebCore/loader/FrameLoaderClient.h (revision 53388) > +++ WebCore/loader/FrameLoaderClient.h (working copy) > @@ -116,6 +116,7 @@ namespace WebCore { > virtual void dispatchDidReceiveServerRedirectForProvisionalLoad() = 0; > virtual void dispatchDidCancelClientRedirect() = 0; > virtual void dispatchWillPerformClientRedirect(const KURL&, double interval, double fireDate) = 0; > + virtual void dispatchDidNavigateWithinPage(bool isHashChange) = 0; A "Page" in WebCore nomenclature represents a top-level browsing context. I think this should be "didNavigateWithinDocument" Also, I don't like the bool. A within-document-navigation can be one of the following: -A state object navigation -A hash change navigation -Both I think an enum is more appropriate and allows for easier correction if more cases are added in the future. > void HistoryController::saveDocumentState() > @@ -629,18 +633,20 @@ void HistoryController::pushState(PassRe > { > Page* page = m_frame->page(); > ASSERT(page); > - > + > // Get a HistoryItem tree for the current frame tree. > RefPtr<HistoryItem> item = createItemTree(m_frame, false); > + ASSERT(item->isTargetItem()); > > - // Override data in the target item to reflect the pushState() arguments. > - HistoryItem* targetItem = item->targetItem(); > - ASSERT(targetItem->isTargetItem()); > - targetItem->setDocument(m_frame->document()); > - targetItem->setTitle(title); > - targetItem->setStateObject(stateObject); > - targetItem->setURLString(urlString); > - > + // Override data in the new item to reflect the pushState() arguments. > + item->setTitle(title); > + item->setStateObject(stateObject); > + item->setURLString(urlString); > + > + // Since the document isn't changed as a result of a fragment scroll, we should > + // preserve the DocumentSequenceNumber of the previous item. > + item->setDocumentSequenceNumber(m_previousItem->documentSequenceNumber()); > + > page->backForwardList()->pushStateItem(item.release()); > } Is this change of extracting the TargetItem necessary? I saw your FIXME in Page::goToItem(), and it seems related to this. And it's an interesting question. But this change seems unrelated to this patch and should probably be separate. I did not review the Chromium WebKit changes.

(In reply to comment #7) ... > > - m_client->addItem(prpItem); > > + m_client->addItem(newItem); > > } > > These changes seem unnecessary - the prp idiom is common within WebCore/Kit and > this seems to just kludge the patch. The prp idiom seemed like some ugly "hungarian notation" thing, and I didn't realize it was accepted *good* practice. I don't see any mention of this in the webkit style guide. > > > void BackForwardList::pushStateItem(PassRefPtr<HistoryItem> newItem) > > { > > - // FIXME: Need to implement state support for chromium. > > + RefPtr<HistoryItem> current = m_client->currentItem(); > > + > > + addItem(newItem); > > + > > + if (!current->stateObject()) > > + current->setStateObject(SerializedScriptValue::create()); > > } > > I know this patch isn't the right time to address this, but I feel it would be > a better design if the BackForwardList organization was with a class of shared > functionality that split off the truly different methods into their own > platform specific files. Like, if "getCurrentItem()" had a base implementation > that everyone shared, but Chromium implemented their own with the client call. > Then addItem() had a normal impl. and a Chromium impl, then this > "pushStateItem()" method could be 100% shared and its logic could not possibly > get out of sync between the ports. > > Like I said, something to be addressed another time... I totally agree. I had a similar idea in fact, but it definitely should happen as a separate patch. > > Index: WebCore/history/HistoryItem.cpp ... > > +static long long generateDocumentSequenceNumber() > > +{ > > + // Initialize to the current time to reduce the likelihood of generating > > + // identifiers that overlap with those from past/future browser sessions. > > + static long long next = static_cast<long long>(currentTime() * 1000000.0); > > + return ++next; > > +} > > *every* platform might want to get a full serialization of the session history > in the future, that's for sure. But I don't like this. Can't we come up with > a better way to get unique IDs across launches? And have them truly be unique > and not just "probably" be unique? Suggestions? I used this same technique in HTMLFormElement.cpp. > > Index: WebCore/loader/FrameLoader.cpp > > =================================================================== > > --- WebCore/loader/FrameLoader.cpp (revision 53388) > > +++ WebCore/loader/FrameLoader.cpp (working copy) > > @@ -3686,7 +3688,7 @@ Frame* FrameLoader::findFrameForNavigati > > > > void FrameLoader::navigateWithinDocument(HistoryItem* item) > > { > > - ASSERT(!item->document() || item->document() == m_frame->document()); > > + ASSERT(item->documentSequenceNumber() == history()->currentItem()->documentSequenceNumber()); > > It's easy for logic to recognize that a sequence of historyitems with "0" as > their sequence number are actually unrelated documents. It's harder to make > sure that every history item has the right sequence number relationship with > its neighbors before entering "navigateWithinDocument." I ran into at least a > few edge cases in my initial implementation where that problem was the case, > hence the allowance for a null document before your patch. > > I see you attempted to get at least one of these cases, but I remember > (vaguely) that there were a handful. > > It might be better to retain the "null sequence number" that was representable > by the null document pointer, and retain the full assertion here. I disagree. It seems like the common case is for each HistoryItem to correspond to a unique document. This is why I initialize each HistoryItem with a unique document sequence number. Only in the cases where we have a same document load do I then go back and set the sequence number of the newly created HistoryItem to the same value as the existing HistoryItem. This is done for state object navigation and hash change navigation, which are the only cases in which a new document isn't created. Either way we have to identify these cases. With the solution I've proposed, I don't have to go back and edit the previous HistoryItem when I realize that the newly created HistoryItem is going to refer to the same document. I just have to edit the newly created HistoryItem. I think that's cleaner. > > Index: WebCore/loader/FrameLoaderClient.h > > =================================================================== > > --- WebCore/loader/FrameLoaderClient.h (revision 53388) > > +++ WebCore/loader/FrameLoaderClient.h (working copy) > > @@ -116,6 +116,7 @@ namespace WebCore { > > virtual void dispatchDidReceiveServerRedirectForProvisionalLoad() = 0; > > virtual void dispatchDidCancelClientRedirect() = 0; > > virtual void dispatchWillPerformClientRedirect(const KURL&, double interval, double fireDate) = 0; > > + virtual void dispatchDidNavigateWithinPage(bool isHashChange) = 0; > > A "Page" in WebCore nomenclature represents a top-level browsing context. I > think this should be "didNavigateWithinDocument" Yeah, I went back and forth on this. I chose WithinPage for consistency with existing methods. Perhaps those should also change to WithinDocument, or perhaps it is still reasonable to say WithinPage since the BackForwardList is a property of the Page. > Also, I don't like the bool. A within-document-navigation can be one of the > following: > -A state object navigation > -A hash change navigation > -Both Good idea. Like this? enum WithinDocumentNavigationType { StateObjectNavigation, HashChangeNavigation }; > > I think an enum is more appropriate and allows for easier correction if more > cases are added in the future. > > > void HistoryController::saveDocumentState() > > @@ -629,18 +633,20 @@ void HistoryController::pushState(PassRe > > { > > Page* page = m_frame->page(); > > ASSERT(page); > > - > > + > > // Get a HistoryItem tree for the current frame tree. > > RefPtr<HistoryItem> item = createItemTree(m_frame, false); > > + ASSERT(item->isTargetItem()); > > > > - // Override data in the target item to reflect the pushState() arguments. > > - HistoryItem* targetItem = item->targetItem(); > > - ASSERT(targetItem->isTargetItem()); > > - targetItem->setDocument(m_frame->document()); > > - targetItem->setTitle(title); > > - targetItem->setStateObject(stateObject); > > - targetItem->setURLString(urlString); > > - > > + // Override data in the new item to reflect the pushState() arguments. > > + item->setTitle(title); > > + item->setStateObject(stateObject); > > + item->setURLString(urlString); > > + > > + // Since the document isn't changed as a result of a fragment scroll, we should > > + // preserve the DocumentSequenceNumber of the previous item. > > + item->setDocumentSequenceNumber(m_previousItem->documentSequenceNumber()); > > + > > page->backForwardList()->pushStateItem(item.release()); > > } > > Is this change of extracting the TargetItem necessary? No, it is not strictly necessary. I just deleted it because I could prove that it was unnecessary. Hence, the assertion. Passing m_frame to createItemTree makes it true that the resulting HistoryItem is the target item. If you prefer it as a separate patch, I can certainly break it out, but it seemed trivial enough to include here. > I saw your FIXME in > Page::goToItem(), and it seems related to this. And it's an interesting > question. But this change seems unrelated to this patch and should probably be > separate. Yeah, I haven't fully tested subframe cases yet. I want to make sure I'm not regressing something. (There may already be layout tests for this.) > > I did not review the Chromium WebKit changes. Indeed, I plan to break them out as a separate patch.

>> > Index: WebCore/history/HistoryItem.cpp >... >> > +static long long generateDocumentSequenceNumber() >> > +{ >> > + // Initialize to the current time to reduce the likelihood of generating >> > + // identifiers that overlap with those from past/future browser >sessions. >> > + static long long next = static_cast<long long>(currentTime() * >1000000.0); >> > + return ++next; >> > +} >> >> *every* platform might want to get a full serialization of the session history >> in the future, that's for sure. But I don't like this. Can't we come up with >> a better way to get unique IDs across launches? And have them truly be unique >> and not just "probably" be unique? > > Suggestions? I used this same technique in HTMLFormElement.cpp. Note, this approach does a pretty good job of avoiding collisions. Notice that the counter is initialized to the current time in microseconds. To have collisions across browser sessions, you'd have to create more than 1 million page navigations per second. That seems extremely unlikely, and so I really question the need to do anything fancier.

(In reply to comment #8) ... > > Is this change of extracting the TargetItem necessary? > > No, it is not strictly necessary. I just deleted it because I could prove > that it was unnecessary. Hence, the assertion. Passing m_frame to > createItemTree makes it true that the resulting HistoryItem is the target > item. > > If you prefer it as a separate patch, I can certainly break it out, but it > seemed trivial enough to include here. I went ahead and broke this out as a separate patch. See https://bugs.webkit.org/show_bug.cgi?id=33969.

*** Bug 33823 has been marked as a duplicate of this bug. ***

Retitling to reflect the problem this bug is now actually trying to solve.

Created attachment 47388 [details] v1 patch Unlike the previous WIP, this version excludes the Chromium WebKit API bits. It also excludes the new FrameLoaderClient method. I'll come back to those in a follow-up. This patch just focuses on fixing this bug. I'll punt the other stuff to separate bugs.

Comment on attachment 47388 [details] v1 patch In reviewing this, I recalled a certain detail of the spec and dug in to it to remind myself. The spec states: "If the specified entry is a state object or the first entry for a Document, the user agent must activate that entry." I wonder if the document-destroyed-navigate-back test actually covers this. For example, if the sequence of entries for a document was: 1 - www.example.com 2 - www.example.com#someHash 3 - www.example.com#someHash (with a state object, via pushState()) Then going back from #3 to #2 should not generate a popstate because entry #2 is not a state object entry, and is also not the first entry for the document. Going back from #2 to #1 *should* generate a popstate event because entry #1 is the first entry, even though it is not a state object entry. Note that this is unrelated to your patch, and I don't think your patch changes WebKit's behavior whether we had it right or wrong. I'm just curious, and thought I'd mention it. Your patch looks fine. r+

Actually, now that I think about it, the final history.back() might've been related to testing this edge case. My r+ is now conditional, based on you exploring that. =D

(In reply to comment #15) > Actually, now that I think about it, the final history.back() might've been > related to testing this edge case. My r+ is now conditional, based on you > exploring that. =D This particular layout test doesn't cover the scenario you described above. It does look like we have some issues to resolve w/ mixed hash changes and push states though. I'll file a bug.

Landed as http://trac.webkit.org/changeset/53861

Reverted r53861 for reason: Caused 2 regression tests to fail. Committed r53869: <http://trac.webkit.org/changeset/53869>

The tests that failed: fast/dom/location-hash.html fast/history/back-forward-is-asynchronous.html

It turns that those test regressions result because the tests are verifying that history.back() runs asynchronously even when navigating back to a reference fragment change. What this indicates is that r53672 did not actually change history.back() to work synchronously in the case of reference fragments. I think this has to do with the null check for HistoryItem::document(). Unless a state object was pushed, that would remain null. In the v1 patch above, I changed the code to just compare documentSequenceNumbers. As a result, the code now does synchronous history.back(). So, the tests just need to be adjusted to match the intent of r53672.

Created attachment 47494 [details] v2 patch Identical to v1 patch except that I reverted the layout test changes from http://trac.webkit.org/changeset/43274.

Comment on attachment 47494 [details] v2 patch Based on previous r+ and the minor diff from them that looked reasonable.

Landed as http://trac.webkit.org/changeset/53950

This change causes a crash in one of our unit tests. It looks like it's just a missing null-check. I am posting a patch to fix it here: https://bugs.webkit.org/show_bug.cgi?id=34443

I think this caused a regression in an internal Google app. I don't have a reduction handy so I'm just linking to the internal bug. http://b/issue?id=2456001

Note that if this caused a regression in a test, then it is a strong possiblity that the test was operating on assumptions from the old and busted past, and is wrong in this new and hot future. ;) Get a reduction here and we can help explore.

Darin said this commit fixed it: https://bugs.webkit.org/show_bug.cgi?id=35063

Ahh - I thought you were implying *behavorial* regression, not a *crashing* regression. Big difference ;)