Previously redirected page incorrectly displayed after reload

VERIFIED FIXED in Firefox 43

Status

()

VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: hooloovoo79, Assigned: dragana)

Tracking

({regression})

44 Branch
mozilla44
regression
Points:
---

Firefox Tracking Flags

(firefox43 verified, firefox44+ fixed, firefox-esr3845+ fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
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.
Confirmed - Setting to NEW
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → All
Keywords: regression, regressionwindow-wanted

Updated

3 years ago
Flags: needinfo?(dd.mozilla)
Flags: needinfo?(bzbarsky)

Updated

3 years ago
status-firefox44: --- → affected
tracking-firefox44: --- → ?
Component: Layout → Document Navigation
Version: Trunk → 44 Branch

Comment 2

3 years ago
Via local build
Last Good: c55a9670de0f
First Bad: c634c30551b0

Regressed by: c634c30551b0	Dragana Damjanovic — Bug 1185256 - Save originURI to the history. r=b
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.
Blocks: 1185256
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 4

3 years ago
Created attachment 8669734 [details] [diff] [review]
bug_1211269.patch
Flags: needinfo?(dd.mozilla)
Attachment #8669734 - Flags: review?(bzbarsky)
(Assignee)

Updated

3 years ago
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
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....
Attachment #8669734 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 7

3 years ago
Created attachment 8669868 [details] [diff] [review]
bug_1211269.patch
Attachment #8669734 - Attachment is obsolete: true
Attachment #8669868 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ec8a30fd8ebb
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Keywords: regressionwindow-wanted
(Assignee)

Comment 10

3 years ago
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.
Attachment #8669868 - Flags: approval-mozilla-esr38?
Attachment #8669868 - Flags: approval-mozilla-beta?
status-firefox43: --- → affected
Comment on attachment 8669868 [details] [diff] [review]
bug_1211269.patch

Approved for uplift to beta, includes a test, needed to go with bug 1185256.
Attachment #8669868 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8669868 [details] [diff] [review]
bug_1211269.patch

Same rational as in bug 1185256
Attachment #8669868 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38-

Comment 13

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/23d25d9a94d7
status-firefox43: affected → fixed
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
status-firefox43: fixed → verified
Flags: qe-verify+
Even though this is fixed I am tracking it for 44 as it's a pretty basic scenario.
tracking-firefox44: ? → +
(Assignee)

Comment 16

3 years ago
Created attachment 8723474 [details] [diff] [review]
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.
Attachment #8723474 - Flags: approval-mozilla-esr38?
status-firefox-esr38: --- → affected
tracking-firefox-esr38: --- → 45+
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)
Attachment #8723474 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
(Assignee)

Comment 18

3 years ago
Created attachment 8724427 [details] [diff] [review]
bug_1211269_esr38.patch
Attachment #8723474 - Attachment is obsolete: true
Attachment #8724427 - Flags: review?(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
Attachment #8724427 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 20

3 years ago
Created attachment 8725820 [details] [diff] [review]
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.
Attachment #8724427 - Attachment is obsolete: true
Attachment #8725820 - Flags: review+
(Assignee)

Comment 21

3 years ago
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.
Attachment #8725820 - Flags: approval-mozilla-esr38?
(Assignee)

Comment 22

3 years ago
Created attachment 8725856 [details] [diff] [review]
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.
Attachment #8725820 - Attachment is obsolete: true
Attachment #8725820 - Flags: approval-mozilla-esr38?
Attachment #8725856 - Flags: review+
Attachment #8725856 - Flags: approval-mozilla-esr38?
Comment on attachment 8725856 [details] [diff] [review]
bug_1211269_esr38.patch

This is also needed in esr38.7
Attachment #8725856 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
https://hg.mozilla.org/releases/mozilla-esr38/rev/b3b151a6e087
status-firefox-esr38: affected → fixed
You need to log in before you can comment on or make changes to this bug.