1211269 - Previously redirected page incorrectly displayed after reload

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[John]

STR
1. Load any page with a redirect like http://is.gd/MTmj1o (just takes you to Mozillazine)
2. Wait for page to finish loading and then reload (F5) the page.

The URL bar now has the original redirect (http://is.gd/MTmj1o) in it and the page loads improperly.

Someone else has narrowed the regress range to:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c55a9670de0f69b5b964bba11e8fd0850ac017d9&tochange=b50322abc2863332513aa05e5e53ab3d8278672d

which suggests Bug 1185256 may be the cause.

Comment 1

[Jim Jeffery not reading bug-mail 1/2/11]

Confirmed - Setting to NEW

Comment 2

[Alice0775 White]

Via local build
Last Good: c55a9670de0f
First Bad: c634c30551b0

Regressed by: c634c30551b0	Dragana Damjanovic — Bug 1185256 - Save originURI to the history. r=b

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.  I assumed that this got checked in bug 1185256...

What we probably need to do is not only save the originalURI but also whether the channel has the LOAD_REPLACE flag, and restore it as needed if we're restoring the originalURI.

Comment 4

[Dragana Damjanovic [:dragana]]

Attachment: bug_1211269.patch

Comment 5

[Dragana Damjanovic [:dragana]]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8669734 [details] [diff] [review]
bug_1211269.patch

>+        loadReplace = !!(loadFlags & nsIChannel::LOAD_REPLACE);

Shouldn't need the !! here.

>+   * @param aLoadReplace    - If set LOAD_REPLACE flag will be set on the
>+   *                          channel.


Only if aOriginalURI is non-null, right?  Otherwise this argument is ignored.

>+++ b/docshell/shistory/nsISHEntry.idl

Please add a blank line between the new attribute and the following comment.

This could really use a test....

Comment 7

[Dragana Damjanovic [:dragana]]

Attachment: bug_1211269.patch

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec8a30fd8ebb

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 10

[Dragana Damjanovic [:dragana]]

Comment on attachment 8669868 [details] [diff] [review]
bug_1211269.patch

Approval Request Comment
[Feature/regressing bug #]:1185256
[User impact if declined]:This is regression from a sec-high bug. If a redirected page is reloaded it will be incorrectly displayed.
[Describe test coverage new/current, TreeHerder]:there is a simple test described in comment #0.
[Risks and why]: Low
[String/UUID change made/needed]: 3 uuids
docshell/base/nsIDocShell.idl
docshell/base/nsIDocShellLoadInfo.idl
docshell/shistory/nsISHEntry.idl

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8669868 [details] [diff] [review]
bug_1211269.patch

Approved for uplift to beta, includes a test, needed to go with bug 1185256.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8669868 [details] [diff] [review]
bug_1211269.patch

Same rational as in bug 1185256

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/23d25d9a94d7

Comment 14

[Mihai Boldan, Desktop QA [:mboldan]]

I was able to reproduce this issue with Firefox 44.0a1 (2015-10-03) and on Windows 7 x64.
Verified fixed with latest 43.0b3 (20151112144305), across platforms [1].

[1] Windows 7 x64, Ubuntu 14.04 x 64 and Mac OS X 10.11.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Even though this is fixed I am tracking it for 44 as it's a pretty basic scenario.

Comment 16

[Dragana Damjanovic [:dragana]]

Attachment: bug_1211269_esr38.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a regrestin of bug 1185256 and needs to be uplifted too
User impact if declined: 
Fix Landed on Version:44
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: yes (the same once as bug 1185256)

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8723474 [details] [diff] [review]
bug_1211269_esr38.patch

Need to uplift this regression fix to esr38.7 in order to uplift fix for bug 1185256 (sec-high issue)

Comment 18

[Dragana Damjanovic [:dragana]]

Attachment: bug_1211269_esr38.patch

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8724427 [details] [diff] [review]
bug_1211269_esr38.patch

>+  // If aLoadReplace is true, OLOAD_REPLACE flag will be set to the nsIChannel.

Typo: extra leading "O" in the flag name.  I guess the trunk patch had the same problem....

r=me

Comment 20

[Dragana Damjanovic [:dragana]]

Attachment: bug_1211269_esr38.patch

only difference:

477c477
< +  // If aLoadReplace is true, LOAD_REPLACE flag will be set to the nsIChannel.
---
> +  // If aLoadReplace is true, OLOAD_REPLACE flag will be set to the nsIChannel.

Comment 21

[Dragana Damjanovic [:dragana]]

Comment on attachment 8725820 [details] [diff] [review]
bug_1211269_esr38.patch

Needed for 1246956.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 22

[Dragana Damjanovic [:dragana]]

Attachment: bug_1211269_esr38.patch

Just rebased.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8725856 [details] [diff] [review]
bug_1211269_esr38.patch

This is also needed in esr38.7

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr38/rev/b3b151a6e087