WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
33224
Chains of history items representing same-document navigation need to always remember that association
https://bugs.webkit.org/show_bug.cgi?id=33224
Summary
Chains of history items representing same-document navigation need to always ...
Darin Fisher (:fishd, Google)
Reported
2010-01-05 11:39:47 PST
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. fast/loader/stateobjects/document-destroyed-navigate-back.html seems to assert this "incorrect" behavior. 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.
Attachments
work in progress
(25.57 KB, patch)
2010-01-18 23:29 PST
,
Darin Fisher (:fishd, Google)
fishd
: review-
fishd
: commit-queue-
Details
Formatted Diff
Diff
v1 patch
(18.25 KB, patch)
2010-01-25 22:42 PST
,
Darin Fisher (:fishd, Google)
beidson
: review+
Details
Formatted Diff
Diff
v2 patch
(24.10 KB, patch)
2010-01-26 22:06 PST
,
Darin Fisher (:fishd, Google)
levin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Darin Fisher (:fishd, Google)
Comment 1
2010-01-05 21:32:56 PST
whatwg discussion here:
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2010-January/024675.html
Brady Eidson
Comment 2
2010-01-12 10:50:19 PST
(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.
Darin Fisher (:fishd, Google)
Comment 3
2010-01-12 11:32:00 PST
> 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?
Brady Eidson
Comment 4
2010-01-12 13:19:06 PST
(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. :)
Ian 'Hixie' Hickson
Comment 5
2010-01-14 23:11:49 PST
I've updated the spec.
Darin Fisher (:fishd, Google)
Comment 6
2010-01-18 23:29:14 PST
Created
attachment 46891
[details]
work in progress
Brady Eidson
Comment 7
2010-01-20 19:33:01 PST
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.
Darin Fisher (:fishd, Google)
Comment 8
2010-01-21 00:20:44 PST
(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.
Darin Fisher (:fishd, Google)
Comment 9
2010-01-21 00:45:01 PST
>> > 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.
Darin Fisher (:fishd, Google)
Comment 10
2010-01-21 14:26:45 PST
(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
.
Brady Eidson
Comment 11
2010-01-22 10:10:41 PST
***
Bug 33823
has been marked as a duplicate of this bug. ***
Brady Eidson
Comment 12
2010-01-22 10:11:40 PST
Retitling to reflect the problem this bug is now actually trying to solve.
Darin Fisher (:fishd, Google)
Comment 13
2010-01-25 22:42:10 PST
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.
Brady Eidson
Comment 14
2010-01-25 23:12:15 PST
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+
Brady Eidson
Comment 15
2010-01-25 23:13:09 PST
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
Darin Fisher (:fishd, Google)
Comment 16
2010-01-26 11:25:56 PST
(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.
Darin Fisher (:fishd, Google)
Comment 17
2010-01-26 11:42:22 PST
Landed as
http://trac.webkit.org/changeset/53861
Adam Roben (:aroben)
Comment 18
2010-01-26 15:00:47 PST
Reverted
r53861
for reason: Caused 2 regression tests to fail. Committed
r53869
: <
http://trac.webkit.org/changeset/53869
>
Darin Fisher (:fishd, Google)
Comment 19
2010-01-26 15:06:49 PST
The tests that failed: fast/dom/location-hash.html fast/history/back-forward-is-asynchronous.html
Darin Fisher (:fishd, Google)
Comment 20
2010-01-26 15:17:48 PST
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
.
Darin Fisher (:fishd, Google)
Comment 21
2010-01-26 22:06:33 PST
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
.
David Levin
Comment 22
2010-01-27 11:59:14 PST
Comment on
attachment 47494
[details]
v2 patch Based on previous r+ and the minor diff from them that looked reasonable.
Darin Fisher (:fishd, Google)
Comment 23
2010-01-27 13:19:00 PST
Landed as
http://trac.webkit.org/changeset/53950
Gustavo Noronha (kov)
Comment 24
2010-02-01 12:15:30 PST
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
Evan Martin
Comment 25
2010-02-19 03:39:25 PST
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
Brady Eidson
Comment 26
2010-02-19 09:15:54 PST
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.
Evan Martin
Comment 27
2010-02-19 09:35:36 PST
Darin said this commit fixed it:
https://bugs.webkit.org/show_bug.cgi?id=35063
Brady Eidson
Comment 28
2010-02-19 10:11:00 PST
Ahh - I thought you were implying *behavorial* regression, not a *crashing* regression. Big difference ;)
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug