History is broken by reviewboard

RESOLVED FIXED in Firefox 52

Status

()

Core
Document Navigation
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: billm, Assigned: freesamael)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

a year ago
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.
(Reporter)

Comment 1

a year ago
Happens in e10s and non-e10s. I tried to bisect, but it reproduces back to 2013-01-01, so I gave up.
Component: DOM → Document Navigation
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?
Flags: needinfo?(sawang)
(Assignee)

Comment 3

a year ago
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.
Flags: needinfo?(sawang)
(Assignee)

Updated

a year ago
Assignee: nobody → sawang
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.
(Assignee)

Comment 5

a year ago
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.
(Assignee)

Comment 6

a year ago
Created attachment 8788833 [details] [diff] [review]
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.
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.
(Assignee)

Comment 8

a year ago
Created attachment 8790201 [details] [diff] [review]
UpdateIndex on LOAD_NORMAL_REPLACE, ReplaceEntry at requestedIndex when available
(Assignee)

Updated

a year ago
Attachment #8788833 - Attachment is obsolete: true
(Assignee)

Comment 9

a year ago
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?
Attachment #8790201 - Flags: review?(bugs)
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.
(Assignee)

Comment 11

a year ago
(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.
Duplicate of this bug: 1245704
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.
Attachment #8790201 - Flags: feedback?(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_...');
Attachment #8790201 - Flags: feedback?(mconley) → feedback+
Although note that it'd probably be good to see a green try run as well.
(Assignee)

Comment 16

a year ago
(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 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.
Attachment #8790201 - Flags: review?(bugs) → review+
(Assignee)

Comment 18

a year ago
Created attachment 8796410 [details] [diff] [review]
UpdateIndex on LOAD_NORMAL_REPLACE, ReplaceEntry at requestedIndex when available. r=smaug
(Assignee)

Comment 19

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=75725f3cd4bc
(Assignee)

Updated

a year ago
Attachment #8790201 - Attachment is obsolete: true
(Assignee)

Comment 20

a year ago
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.
Attachment #8796410 - Attachment description: UpdateIndex on LOAD_NORMAL_REPLACE, ReplaceEntry at requestedIndex when available → UpdateIndex on LOAD_NORMAL_REPLACE, ReplaceEntry at requestedIndex when available. r=smaug
Attachment #8796410 - Flags: review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 21

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b80f3b6f48b8
UpdateIndex on LOAD_NORMAL_REPLACE, ReplaceEntry at requestedIndex when available. r=smaug
Keywords: checkin-needed

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b80f3b6f48b8
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.