Created attachment 46382 [details] Test case push/replaceState() during load followed by a history.back() doesn't work as expected. According to 10.5.3 of the spec (http://dev.w3.org/html5/spec/Overview.html#activating-state-object-entries), if a state object is popped before the document has loaded, that popped state object becomes the "pending" state object and the event is to-be-fired after the load event fires. I thought I had this working originally when working on the ajaxy history stuff, but apparently missed it in the end. See the attached test case. The output is: Pushing state - document.readyState is loading Popping state - document.readyState is loading State popped - null (type object) - document.readyState is complete And I'd expect it to be: Pushing state - document.readyState is loading Popping state - document.readyState is loading Load event fired - document.readyState is complete State popped - New state (type string) - document.readyState is complete
<rdar://problem/7535011>
The history.back() scheduling cancels the current load, putting the document into the complete state but never firing the load event. Either we need to do the history.back() synchronously or somehow remember to fire the load event along with it.
In the case of following a fragment scroll link, the load is performed synchronously via the "loadInSameDocument" call. I bet that this works in the "before the document has loaded" case just the same as what we're trying to achieve here with the history navigation to a state entry. It should be pretty easy to bottleneck this new interesting case in HistoryController.
> Either we need to do the history.back() synchronously or somehow remember to > fire the load event along with it. It is important that history.back() be performed asynchronously.
(In reply to comment #4) > > Either we need to do the history.back() synchronously or somehow remember to > > fire the load event along with it. > > It is important that history.back() be performed asynchronously. I'm curious why you say this. 6.10.2's description of history.go() makes no mention of queueing any part of the operation or making it be asynchronous. Step's 1-5 choose the specified entry then step 6 hands it off to 6.11.9. 6.11.9's description of history traversal makes no mention of queueing any of the operations (except for 6.11.9.4.5 which is only about queueing the pageshow event) or making it be asynchronous. What am I missing?
Since Darin (Fisher) hasn't elaborated on why he thinks this history.back() needs to be asynchronous and there's already a pretty solid example of a synchronous back/forward navigation that we already do (back/forward amongst fragment scrolls), I'm going to start on a patch that does this type of history.go() synchronously similar to how fragment scrolls work.
Created attachment 46793 [details] Slightly updated test case
Created attachment 46794 [details] WIP patch Current WIP. This turned out to be surprisingly easy to get ALMOST right. This patch makes the above test case work as well as others that are a mix of fragment navs and fragment/state object combinations. The only think this patch obviously gets wrong is the back-forward cursor is not set to the right place after the history.back(). Weeding out why might be easy.
Gosh I hope the try-bots don't actually try to run on this patch, I sure don't want them to waste their time.
Sorry for the slow response. I missed the bugmail. Please see https://bugs.webkit.org/show_bug.cgi?id=25570 for reference. For compat with IE, FF and Chrome, I changed WebKit to make history.{back,forward,go} work asynchronously. It turns out that Chrome has to perform these asynchronously since the back forward list is stored in another process. In general, I also believe we should favor making DOM methods that trigger DOM events function asynchronously whenever possible. It helps reduce complexity by avoiding re-entrancy issues. Stepping back, please note that history.back cannot complete synchronously in all cases. Often, it requires loading data from the network or from a disk cache, which happens asynchronously. In the page cache case or in the fragment scroll case, you could perform history.back() synchronously, but I'd argue that it is a bad idea to make the behavior differ from a normal back navigation.
I think we need to get clarity from the working groups to understand what is supposed to happen in this case. Since history.back() cancels the page load, and prevents the load event from firing, it is not possible for the "pending" state object to-be-fired after the load event fires. Maybe the spec should be amended to say that it can also fire after a load error?
Let me preface my reply by saying that I'm not trying to have an academic discussion about whether this should or should not be synchronous. What I am attempting to bring together is: 1 - How WebKit actually behaves today in some cases. 2 - How other browsers actually behave today in the same cases. 3 - Exactly what the specified behavior is. (In reply to comment #10) > Sorry for the slow response. I missed the bugmail. > Please see https://bugs.webkit.org/show_bug.cgi?id=25570 for reference. > For compat with IE, FF and Chrome, I changed WebKit to make > history.{back,forward,go} work asynchronously In your first comment in 25570, you say: "None of the other browsers behave this way. They always dispatch history.{back,forward,go} asynchronously." Are you sure they dispatch the back/forward asynchronously, or is it simply possible that they fired the scroll event asynchronously? > In general, I also believe we should favor making DOM methods that trigger DOM > events function asynchronously whenever possible. It helps reduce complexity > by avoiding re-entrancy issues. In general I agree with this philosophy. But there's also something to be said for not changing long-since-implemented, well-tested behavior for idealistic reasons. In this context I'm referring to fragment scroll, both in the sense of a fragment navigation (which is often/always synchronous in WebKit) as well as fragment history traversals. That the state object navigation/traversal model matches so well with the one for fragment navigation/traversal is no accident. The spec seems to quite purposefully treat them similarly or even identically. > Stepping back, please note that history.back cannot complete synchronously in > all cases. > Often, it requires loading data from the network or from a disk > cache, which happens asynchronously. Well of course! This is why the spec is so very explicit about what is synchronous and what is not. According to 6.11.9.1, if the specified entry has no document then the traversal algorithm is aborted and the navigation algorithm (6.11.1) is invoked. And at 6.11.1.12, the navigation algorithm specifically calls out converting to asynchronous operation. But just-as-explicitly in the spec is traversals and navigations that *do* happen synchronously. 6.11.1, for example, points out that navigations to about:blank or to certain types of inline content *must* be synchronous. And back in 6.11.9.1, if the specific entry has a Document object then 6.11.1 is skipped altogether. Cases where the target entry might have a document are if it represents a fragment difference from the current document's URL, if it represents a state object entry for the current document, or if its document is suspended in a page cache. For these cases, the entire remainder of 6.11.9 completes synchronously. > In the page cache case or in the fragment scroll case, you could perform > history.back() synchronously, but I'd argue that it is a bad idea to make the > behavior differ from a normal back navigation. You were quite prescient in calling out those two cases, as I just did above, and as the spec already has. :) (In reply to comment #11) > I think we need to get clarity from the working groups to understand what is > supposed to happen in this case. I think raising the conversation with the working groups is needed at this point in the discussion. But my understanding of 6.11.9 leading into 6.11.1 involves little ambiguity - I'm pretty sure the expected behavior I've laid out *is* what is specified, even if it wasn't necessarily the intentions of the editors. > Since history.back() cancels the page load, and prevents the load event from > firing, it is not possible for the "pending" state object to-be-fired after the > load event fires. This might be the difference in our understandings that is causing the confusion. history.back() does not actually cancel the page load. history.back() simply finds the target item from the session history then hands it off to the History traversal algorithm (6.11.9). If the specified item fails the check in 6.11.9.1 and is handed off to the Navigation algorithm (6.11.1), then yes - 6.11.1.5 or 6.11.1.7 might cause the current load to be stopped. Please note that even the Navigation algorithm can be aborted *without* the current load being stopped (e.g., 6.11.1.4). In the interesting cases being discussed - state objects entries and fragment scroll entries - the current load is not necessarily stopped.
As far as I can tell, Brady is right. In my testing, I found IE to be synchronous in the same way the spec describes. If you think this should change, I recommend creating a tiny test case demonstrating the difference between the spec and what you think should happen, and e-mailing the WG with the URL to that test case.
(In reply to comment #12) > Let me preface my reply by saying that I'm not trying to have an academic > discussion about whether this should or should not be synchronous. > > What I am attempting to bring together is: > 1 - How WebKit actually behaves today in some cases. > 2 - How other browsers actually behave today in the same cases. > 3 - Exactly what the specified behavior is. That's good, but I think we should also consider how things should be, especially at this stage of the game where the spec for pushState is still in flux. > (In reply to comment #10) > > Sorry for the slow response. I missed the bugmail. > > Please see https://bugs.webkit.org/show_bug.cgi?id=25570 for reference. > > For compat with IE, FF and Chrome, I changed WebKit to make > > history.{back,forward,go} work asynchronously > > In your first comment in 25570, you say: > "None of the other browsers behave this way. They always dispatch > history.{back,forward,go} asynchronously." > > Are you sure they dispatch the back/forward asynchronously, or is it simply > possible that they fired the scroll event asynchronously? I think I observed the scroll event in my testing, so it could be either. Digging through the Mozilla code, I believe it is indeed the latter. (Perhaps we should consider dispatching the scroll event asynchronously as well.) > > In general, I also believe we should favor making DOM methods that trigger DOM > > events function asynchronously whenever possible. It helps reduce complexity > > by avoiding re-entrancy issues. > > In general I agree with this philosophy. But there's also something to be said > for not changing long-since-implemented, well-tested behavior for idealistic > reasons. In this context I'm referring to fragment scroll, both in the sense > of a fragment navigation (which is often/always synchronous in WebKit) as well > as fragment history traversals. > > That the state object navigation/traversal model matches so well with the one > for fragment navigation/traversal is no accident. The spec seems to quite > purposefully treat them similarly or even identically. Yeah, that makes sense. > > Since history.back() cancels the page load, and prevents the load event from > > firing, it is not possible for the "pending" state object to-be-fired after the > > load event fires. > > This might be the difference in our understandings that is causing the > confusion. history.back() does not actually cancel the page load. > > history.back() simply finds the target item from the session history then hands > it off to the History traversal algorithm (6.11.9). > > If the specified item fails the check in 6.11.9.1 and is handed off to the > Navigation algorithm (6.11.1), then yes - 6.11.1.5 or 6.11.1.7 might cause the > current load to be stopped. Please note that even the Navigation algorithm can > be aborted *without* the current load being stopped (e.g., 6.11.1.4). > > In the interesting cases being discussed - state objects entries and fragment > scroll entries - the current load is not necessarily stopped. This indeed clears up some confusion on my part. Now, I'm confused about why history.back() scheduling cancels the load. Perhaps that is not necessary, and if we fix that, we'll have fixed this bug? I'm also a bit confounded by what it *should* mean to call pushState during page load. replaceState makes sense. Navigating to a reference fragment during page load is a lot like replaceState. It seems a bit strange and potentially problematic to be changing the current HistoryItem during page load without also stopping page load. Just imagine what happens if the BODY includes an IFRAME after some inline script has called pushState. Do we have to go back to the previous HistoryItem and append a HistoryItem for the IFRAME?
> > > (In reply to comment #10) > > > Sorry for the slow response. I missed the bugmail. > > > Please see https://bugs.webkit.org/show_bug.cgi?id=25570 for reference. > > > For compat with IE, FF and Chrome, I changed WebKit to make > > > history.{back,forward,go} work asynchronously > > > > In your first comment in 25570, you say: > > "None of the other browsers behave this way. They always dispatch > > history.{back,forward,go} asynchronously." > > > > Are you sure they dispatch the back/forward asynchronously, or is it simply > > possible that they fired the scroll event asynchronously? > > I think I observed the scroll event in my testing, so it could be either. > Digging through the Mozilla code, I believe it is indeed the latter. > > (Perhaps we should consider dispatching the scroll event asynchronously as > well.) I scoured the text of the spec' to see how dispatching the scroll event is defined, but came up short. Is it in there at all? > This indeed clears up some confusion on my part. > > > Now, I'm confused about why history.back() scheduling cancels the load. > Perhaps that is not necessary, and if we fix that, we'll have fixed this bug? We simply don't match the spec yet. The idea of creating a cohesive spec of navigation and history traversal by looking at all the major browsers had never been done before HTML5 and it seems highly unlikely we would've already matched it without fixing some bugs. :) That said, this bug isn't so much about fixing this one specific test case in any particular manner. Rather it's about changing us to match the spec in a way this test case uncovered. If we pushed back canceling the load until the scheduled redirect occurred, but still kept the history.back() asynchronous, then yes that would fix the symptom in this bug. But in exploring this I think we've uncovered an understanding that this test case is meant to involve a synchronous history traversal. > I'm also a bit confounded by what it *should* mean to call pushState during > page load. replaceState makes sense. Navigating to a reference fragment > during page load is a lot like replaceState. It seems a bit strange and > potentially problematic to be changing the current HistoryItem during page load > without also stopping page load. Just imagine what happens if the BODY > includes an IFRAME after some inline script has called pushState. Do we have > to go back to the previous HistoryItem and append a HistoryItem for the IFRAME? I don't see why you would. As long as the document is live and shared between the two history entries, then returning to the original HistoryItem in your scenario would simply be an inter-document history traversal and not depend on knowing anything about the frame tree. Additionally, if the document was navigated away from and cleared from the history entries, then the URLs in those HistoryItems are the one true representation of their state - frames or no frames, state objects or no state objects.
Updating the title to reflect the final understanding of what needs to be changed.
Created attachment 47095 [details] Patch and layout test updates After a few back and forths here and with Hixie's guidance, I think we're all in agreement with what the spec says. That said, I think there's still disagreement on what the spec *should* be. Personally, I don't see any problem with the spec as-is and we've even analyzed how the synchronicity here might affect WebKit in a multi-process future, and we're pretty convinced it's not a huge problem. This same consensus was reached by at least one non-WebKit contributor who works on the spec. If there are changes to be made to the spec in the future, that's fine - the process will work its magic and we'll make WebKit compliant. Until then, this patch brings WebKit inline with the spec we have.
Comment on attachment 47095 [details] Patch and layout test updates What happens if i now do: history.back(); // going to a different document, so it's asynchronous history.forward(); // going to the same document which will be synchronous My reading say that the history.back() call will make an asynchronous load going back 1 step, history.forward will now happen synchronously so will be done before the back load starts. When back starts there has now been an additional state put into history, so stepping back 1 entry will take us back to the original point. This seems odd to me.
Comment on attachment 47095 [details] Patch and layout test updates r=me
Please hold the presses! This behavior change is not something we can easily support in Chrome, so by making this change, you are introducing a compatibility difference between Chrome and Safari. At the very least, this change is going to break Chrome in a serious way until I have a chance to compensate for it. I think that warrants further discussion (or at least some temporary #ifdefs if you are going to land it now). > If we pushed back canceling the load until the scheduled redirect occurred, but > still kept the history.back() asynchronous, then yes that would fix the symptom > in this bug. But in exploring this I think we've uncovered an understanding > that this test case is meant to involve a synchronous history traversal. I think we should explore this solution further. I understand your desire to match the spec, but I also think that the spec can and should change without breaking anything. It is clear that IE does not implement history.back() in a synchronous manner, so we should not need to make it synchronous for web compat reasons.
Landed in http://trac.webkit.org/changeset/53672 with the new behavior disabled for Chromium.
(In reply to comment #18) > (From update of attachment 47095 [details]) > What happens if i now do: > history.back(); // going to a different document, so it's asynchronous > history.forward(); // going to the same document which will be synchronous > > My reading say that the history.back() call will make an asynchronous load > going back 1 step, history.forward will now happen synchronously so will be > done before the back load starts. When back starts there has now been an > additional state put into history, so stepping back 1 entry will take us back > to the original point. > > This seems odd to me. This is a good question. Brady, did you and Oliver discuss this issue? Would either of you mind sharing the resolution? Thank you for the ENABLE(HISTORY_ALWAYS_ASYNC) define! (I still hope to convince you that all WebKit ports should enable that flag.)