1300461 - History is broken by reviewboard

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

STR (in new profile):
1. Open a new tab.
2. Visit https://bugzilla.mozilla.org/show_bug.cgi?id=1288618
3. Click on the "Review" link for part 14.
4. Go back.
5. Go forward.

ER:
The back button is not grayed out. I should be able to go back to bugzilla.

AR:
The back button is grayed out.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Happens in e10s and non-e10s. I tried to bisect, but it reproduces back to 2013-01-01, so I gave up.

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

I tend to thinking this is something close to P3 in consideration of the specific use case (reviewboard) and the age. 

Samael, do you think about something suspicious?

Comment 3

[Samael Wang [:freesamael] (away for now)]

It seems the reviewboard page wasn't in bfcache. On history navigation it tried to reload the URL. 
Somehow nsDocShell::OnRedirectStateChange() was invoked and nsDocShell::mLoadInfo was changed 
from LOAD_CMD_HISTORY to LOAD_NORMAL_REPLACE, so nsSHistory::UpdateIndex() was never called afterward.
Leaving the session history index being incorrect.

It brings me these questions:
1. Why wasn't the reviewboard page cacheable? I found both Safari and Chrome cache the page.
2. Why was there a redirection? I used a proxy to record the response and there wasn't a HTTP redirect.
   Besides both old channel and new channel have the same URI 
   "https://reviewboard.mozilla.org/r/76614/diff/2/#index_header"
3. Can a redirection really happen on history navigation? If so how should it be handled?

Anyway if it's not in urgent I can take some time to try digging deeper.

Comment 4

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

What you mean with cacheable? (Chrome doesn't have bfcache). Or do you mean http cache?

And Gecko doens't bfcache because mozreview has beforeunload listener.

Redirection can of course happen on history navigation, when not loading from bfcache, but doing a normal http load.

Comment 5

[Samael Wang [:freesamael] (away for now)]

Ahh so that should be HTTP cache in chrome. The navigation was so quick (comparing to firefox) I couldn't tell if it's bfcache or http cache.

Comment 6

[Samael Wang [:freesamael] (away for now)]

Attachment: Update SHistory index when loadtype is LOAD_NORMAL_REPLACE

So there's a straghtforward way to fix this symptom, but I'm not quite sure it's the right approach.

Comment 7

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

This could use then also some tests.
We have some in http://searchfox.org/mozilla-central/source/docshell/test/navigation/test_sessionhistory.html and others in the same directory.

Comment 8

[Samael Wang [:freesamael] (away for now)]

Attachment: UpdateIndex on LOAD_NORMAL_REPLACE, ReplaceEntry at requestedIndex when available

Comment 9

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8790201 [details] [diff] [review]
UpdateIndex on LOAD_NORMAL_REPLACE, ReplaceEntry at requestedIndex when available

Hi Olli,

I've included a test case to cover the symptom, verified it wouldn't pass without patching. I found the ReplaceEntry call was also incorrectly replacing previous entry so I fixed that part as well. 

Would you take a look at it?

Comment 10

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

Sure.
Did you test the behavior in other browsers? Session history is a thing where HTML spec is very broken 
(https://github.com/whatwg/html/issues/1454) and blink/webkit quite broken and IE/Edge and Gecko have then had a bit better implementations.

Comment 11

[Samael Wang [:freesamael] (away for now)]

(In reply to Olli Pettay [:smaug] from comment #10)
> Sure.
> Did you test the behavior in other browsers? Session history is a thing
> where HTML spec is very broken 
> (https://github.com/whatwg/html/issues/1454) and blink/webkit quite broken
> and IE/Edge and Gecko have then had a bit better implementations.

By disabling cache and use builtin developer tools, I can confirm Chrome and Safari behave as expected for comment 0. Edge has no problem either but interestingly Edge didn't encounter HTTP 301 at all. 

The reviewboard path in Chrome, Safari and Firefox all appeared as "/r/76614/diff/4#index_header", and when they try to GET "r/76614/diff/4" the web server says goto "r/76614/diff/4/". Somehow Edge recorded the path as "r/76614/diff/4/" at first place so it never encounter redirection.

I tried a local test to verify Edge still work properly if redirection does happen, through.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8790201 [details] [diff] [review]
UpdateIndex on LOAD_NORMAL_REPLACE, ReplaceEntry at requestedIndex when available

Hoping to get more docshell review requests under my belt, so I hope it's alright that I request feedback from myself.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8790201 [details] [diff] [review]
UpdateIndex on LOAD_NORMAL_REPLACE, ReplaceEntry at requestedIndex when available

Review of attachment 8790201 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Samael - this looks okay to me!

::: docshell/base/nsDocShell.cpp
@@ +11644,5 @@
>  
>      AddURIVisit(aURI, referrer, previousURI, previousFlags, responseStatus);
>    }
>  
>    // If this was a history load or a refresh,

I guess this documentation should be updated too.

@@ +12279,5 @@
>      // This is the root docshell
>      bool addToSHistory = !LOAD_TYPE_HAS_FLAGS(mLoadType, LOAD_FLAGS_REPLACE_HISTORY);
>      if (!addToSHistory) {
>        // Replace current entry in session history.
> +      // Note: If the requested index is valid, it indicats the loading was

Typo: "indicats" -> "indicates"

::: docshell/test/navigation/file_bug1300461.html
@@ +4,5 @@
> +    <meta http-equiv="content-type" content="text/html; charset=utf-8">
> +    <title>Bug 1300461</title>
> +  </head>
> +  <body onload="test();">
> +    <script>

Great to see a test! Would you mind documenting the scenario this test is testing somewhere in here?

::: docshell/test/navigation/file_bug1300461_back.html
@@ +3,5 @@
> +  <head>
> +    <meta http-equiv="content-type" content="text/html; charset=utf-8">
> +    <title>Bug 1300461</title>
> +  </head>
> +  <body onload="test();" onunload="">

I guess we can get rid of the empty onunload attribute? Or am I missing something about this docshell testing infrastructure?

@@ +17,5 @@
> +          opener.is(shistory.index, 1, 'file_bug1300461_back.html: check history index == 1');
> +          opener.is(shistory.requestedIndex, -1, 'file_bug1300461_back.html: check requestedIndex == -1');
> +          opener.ok(webNav.canGoBack, 'file_bug1300461_back.html: check canGoBack == true');
> +          if (opener.testCount == 1) {
> +            opener.ok(true, 'file_bug1300461_back.html: replaceState to redirect.html');

Is this just informational? If so, I guess you could just do opener.info('file_...');

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Although note that it'd probably be good to see a green try run as well.

Comment 16

[Samael Wang [:freesamael] (away for now)]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #14)
> Comment on attachment 8790201 [details] [diff] [review]
> UpdateIndex on LOAD_NORMAL_REPLACE, ReplaceEntry at requestedIndex when
> available
> 
> Review of attachment 8790201 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: docshell/test/navigation/file_bug1300461_back.html
> @@ +3,5 @@
> > +  <head>
> > +    <meta http-equiv="content-type" content="text/html; charset=utf-8">
> > +    <title>Bug 1300461</title>
> > +  </head>
> > +  <body onload="test();" onunload="">
> 
> I guess we can get rid of the empty onunload attribute? Or am I missing
> something about this docshell testing infrastructure?

It was to avoid this page being in bfcache. I think test_sessionhistory.html uses the same trick:
http://searchfox.org/mozilla-central/rev/f6c298b36db67a7109079c0dd7755f329c1d58e2/docshell/test/navigation/test_sessionhistory.html#45

But yes I should have added some document.

> @@ +17,5 @@
> > +          opener.is(shistory.index, 1, 'file_bug1300461_back.html: check history index == 1');
> > +          opener.is(shistory.requestedIndex, -1, 'file_bug1300461_back.html: check requestedIndex == -1');
> > +          opener.ok(webNav.canGoBack, 'file_bug1300461_back.html: check canGoBack == true');
> > +          if (opener.testCount == 1) {
> > +            opener.ok(true, 'file_bug1300461_back.html: replaceState to redirect.html');
> 
> Is this just informational? If so, I guess you could just do
> opener.info('file_...');

Yup only to make the testing message more informative. I'll try opener.info.

Glad to get your feedback! I'll update and trigger a try later.

Comment 17

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

Comment on attachment 8790201 [details] [diff] [review]
UpdateIndex on LOAD_NORMAL_REPLACE, ReplaceEntry at requestedIndex when available

As all session history changes, this is regression risky.
But looks good (assuming the comments mconley gave are fixed)

And yes, onunload="" there is to prevent bfcache.

Comment 18

[Samael Wang [:freesamael] (away for now)]

Attachment: UpdateIndex on LOAD_NORMAL_REPLACE, ReplaceEntry at requestedIndex when available. r=smaug

Comment 19

[Samael Wang [:freesamael] (away for now)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75725f3cd4bc

Comment 20

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8796410 [details] [diff] [review]
UpdateIndex on LOAD_NORMAL_REPLACE, ReplaceEntry at requestedIndex when available. r=smaug

let's wait for the try run.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b80f3b6f48b8
UpdateIndex on LOAD_NORMAL_REPLACE, ReplaceEntry at requestedIndex when available. r=smaug

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b80f3b6f48b8