1367586 - Incorrect URL appears in location bar when going back to redirected page

Status: RESOLVED DUPLICATE of bug 1319111

(Core :: DOM: Navigation, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

A recent change in Nightly seems to have broken session history and / or the location bar when going back.

I am currently using 2017-05-24.

STR:

1. In tab A (Gmail), open a new tab from a link in some email.
  * When you click a link from Gmail, the URL loaded is "https://www.google.com/url?hl=en&q=<ACTUAL_PAGE>", which eventually redirects to <ACTUAL_PAGE>
2. In the new tab B, click on something to navigate forward.
3. Click the back button in tab B.

ER:

I expect the location bar to contain the URL that matches the page I went back to, which is <ACTUAL_PAGE>.

AR:

While I do see the correct page content, the location bar shows "https://www.google.com/url?hl=en&q=<ACTUAL_PAGE>", the value _before_ redirection.

Comment 1

[Alice0775 White]

I can reproduce the problem.

Steps to reproduce:
1. Open https://www.yahoo.co.jp/
2. Click link of a news topic in middle column
   (it will redirect actual url)
3. Click a any link to navigate forward
4. Go back


Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=01ed176087496bf08ba4da9f323a32fe9e281925&tochange=7f28c1084c47d771be342692f37a8b0d575d3264

Regressed by: 7f28c1084c47	Honza Bambas — Bug 1319111 - Expose 'result principal URI' on LoadInfo as a source for NS_GetFinalChannelURI (removes some use of LOAD_REPLACE flag). r=bz

Comment 2

[Boris Zbarsky [:bzbarsky]]

Ugh.  With the new setup, we can't just rely on the protocol handler setting the right things when repeating the navigation (as opposed to the "resultPrincipalURI defaults to URI if not set" case, in which redirects set a _null_ resultPrincipalURI).  Since we now set the resultPrincipalURI during HTTP redirects, we also need to store the resultPrincipalURI in session history, session restore, etc, so we can replay said redirects properly.  Worse yet, it's not immediately obvious what we should store, because we really want to store the thing that was set on the loadInfo _before_ the newChannel call on the post-redirect protocol handler, but that information is no longer available by the time we're creating the session history entry.  I think this is the issue that initially made us go with the "resultPrincipalURI defaults to URI" setup that caused complications with extensions.  :(

This is a critical security issue, because we derive the principal from this URI that we're now getting wrong....

Honza, are you able to take the time to fix this?

Comment 3

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Security regression.

Comment 4

[Honza Bambas (:mayhemer)]

Thanks for the analyzes Boris.  Taking this.  I'll do my best to move on yet today.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

I found a way to (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #0)
> A recent change in Nightly seems to have broken session history and / or the
> location bar when going back.
> 
> I am currently using 2017-05-24.
> 
> STR:
> 
> 1. In tab A (Gmail), open a new tab from a link in some email.
>   * When you click a link from Gmail, the URL loaded is
> "https://www.google.com/url?hl=en&q=<ACTUAL_PAGE>", which eventually
> redirects to <ACTUAL_PAGE>
> 2. In the new tab B, click on something to navigate forward.
> 3. Click the back button in tab B.
> 
> ER:
> 
> I expect the location bar to contain the URL that matches the page I went
> back to, which is <ACTUAL_PAGE>.
> 
> AR:
> 
> While I do see the correct page content, the location bar shows
> "https://www.google.com/url?hl=en&q=<ACTUAL_PAGE>", the value _before_
> redirection.

I started reliably reproducing this as well with the gmail case. It only occurs when the page which I am navigating away from is not in the BF cache.

If you navigate `gmail` -> `bugzilla` -> add onbeforeunload handler to bugzilla page to cause it to not stay in bfcache -> `bugzilla dashboard` -> hit back button the URL in the URL bar shows the google redirect URL, the certificate badge in the URL bar says `Mozilla Foundation (US)`, and `document.location` is the pre-redirect URL.

In addition, the CSS on the loaded page is completely broken. I'm guessing this is because of the incorrect URL issue. I'm using Firefox Nightly 2017-05-24.

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-) from comment #2)
> Ugh.  With the new setup, we can't just rely on the protocol handler setting
> the right things when repeating the navigation (as opposed to the
> "resultPrincipalURI defaults to URI if not set" case, in which redirects set
> a _null_ resultPrincipalURI).  Since we now set the resultPrincipalURI
> during HTTP redirects, we also need to store the resultPrincipalURI in
> session history, session restore, etc, so we can replay said redirects
> properly.  Worse yet, it's not immediately obvious what we should store,
> because we really want to store the thing that was set on the loadInfo
> _before_ the newChannel call on the post-redirect protocol handler, but that
> information is no longer available by the time we're creating the session
> history entry.  I think this is the issue that initially made us go with the
> "resultPrincipalURI defaults to URI" setup that caused complications with
> extensions.  :(

(Haven't confirmed this is really the cause, but highly likely is)

I would do the following:
- store the TP-URI to session store
- we create a channel during the restoration (TP-URI left null)
- when the handler leaves the TP-URI null, set it to whatever we have got in the session store 
- (we could potentially assert when both are non-null, they are also the same)
- let HttpBaseChannel::CloneLoadInfoForRedirect do the same: preset PT-URI to null, when left null by the handler, set to redirect target URI

This is then consistent, in both cases, 1) redirect 2) restore, protocol handlers work under the same initial input conditions.

I think this is even better, since setting the TP-URI in advance in the redirect case could cause some unexpected side effects, like when:

- the protocol handler creates an inner channel with a different URI (setting originalURI to something else after it gets the result, after the following steps were performed)
- the nested protocol handler creates its channel
- does NS_GetFinalChannelURI to decide on whatever conditions it needs to examine
=> gets the redirect target URI, but should get the "different URI" (original URI of the just created channel)

Comment 7

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

Could we please backout bug 1319111 for now? Nightly is rather broken with this.

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Olli Pettay [:smaug] from comment #7)
> Could we please backout bug 1319111 for now? Nightly is rather broken with
> this.

Yes, will take care, because I don't believe the fix here would make it to the next build.

Comment 9

[Marcia Knous [:marcia]]

Tracking 55+ for this regression.

Comment 10

[Honza Bambas (:mayhemer)]

(In reply to Marcia Knous [:marcia - use ni] from comment #9)
> Tracking 55+ for this regression.

I just backed out the offending bug.

Comment 11

[Honza Bambas (:mayhemer)]

Attachment: v1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee89d552fad1eb034e9d2eeedd2d4ae2cf3ec268
https://treeherder.mozilla.org/#/jobs?repo=try&revision=919e33ae03c4f3e8f8c7f3e88084926e0b3b092b

brings back some parts of the v9 patch from bug 1319111, session store compatibility code is updated so that TP-URI is rebuilt as |if LOAD_REPLACE -> TP-URI = URI|.

Verified with STR from comment 1.

Pending on try result will ask for r.

Comment 12

[Honza Bambas (:mayhemer)]

Boris, sorry to bother you again with this.  Thy push seems to be green for this patch.  Can you review it please?  

For details see comment 6, s/TP-URI/result principal URI/ (or rpURI, for short?)

Bug 1319111 has been backed out, so it's not super urgent.  Thanks.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8871337 [details] [diff] [review]
v1

Why do we need NS_EnsureFinalChannelURI instead of just setting that URI on the loadinfo before we create the channel in nsDocShell::DoURILoad?

Similarly, why the changes in HttpBaseChannel?  What was wrong with the old setup of setting up the resultPrincipalURI in CloneLoadInfoForRedirect as opposed to SetupReplacementChannel?

We should really add tests for this, so we don't regress it _again_.  Should have done that in bug 1211269, sorry.  :(

The rest looks good.  I think I buy the sessionstore changes, though it might be good to have one of the sessionstore people look at them too.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Ah, I guess comment 6 answers my "why" questions?

Comment 15

[(no longer active)]

FWIW I got the crash in bug 1367742 today when restoring my session and just now noticed that I have a couple of tabs which show symptoms similar to this bug and bug 1367656.  It could be that crash is an instance of this bug as well...  I'm commenting here to avoid revealing this connection in a non-hidden bug.

Comment 16

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-) from comment #14)
> Ah, I guess comment 6 answers my "why" questions?

Yes.  I think this is more consistent way to do it.  If you believe otherwise, I can leave the behavior as it was in the last landed version (v11) for bug 1319111.  But I think the arguments from comment 6 make sense.

And I'll add a test as well (have to remember how one does it :))

Thanks.

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #15)
> FWIW I got the crash in bug 1367742 today when restoring my session and just
> now noticed that I have a couple of tabs which show symptoms similar to this
> bug and bug 1367656.  It could be that crash is an instance of this bug as
> well...  I'm commenting here to avoid revealing this connection in a
> non-hidden bug.

Could be a different bug - bug 1366224.  But it's under investigation.

Comment 18

[Honza Bambas (:mayhemer)]

Attachment: test v1

according STR comment 1.

mochitest, iframe is loaded a URL that redirects, then it's loaded another URL (not redirecting).  iframe history is navigated back (history.back()), expected redirect target URL is checked in the location.

fails w/o patch v1, passes w/ it.

Comment 19

[Boris Zbarsky [:bzbarsky]]

So the only concern I had is this: what happens if you have a protocol handler that really wants its principal URI to be the URI that's passed to its newChannel method.  How can it do that in the new ordering?  In the old ordering, it could force the principalURI to null and then end up using the originalURI.  But I guess in the new ordering it can just remember the value that was passed to newChannel and force to that non-null value.  I can't think of use cases for a protocol handler to force a principal URI of "whatever originalURI happens to get set to", which is what null really means in the new world.

That said, I'm not sure the docshell behavior of ignoring the session history principalURI if the protocol handler sets a non-null one is correct.  By definition, when the session history load happened it happened with the given URI, originalURI and principalURI.  We should faithfully replay that, I would think, instead of letting the protocol handler for "URI" possibly override the principalURI we used last time.  Otherwise there's a risk of the load happening differently this time and leading to a different principal, which is bad.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8871721 [details] [diff] [review]
test v1

>+<body>Redirect target content</body>

This should probably have a pageshow listener that asserts

  parent.ok(!event.persisted, "We shouldn't load from bfcache");

or so.  Otherwise if we ever did bfcache in subframes we could have this test pass by accident.

r=me

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8871337 [details] [diff] [review]
v1

r=me if you make the set of the resultPrincipalURI unconditional when it's non-null in docshell.  And maybe even if it _is_ null, actually; again the goal is to correctly replay the channel load.

Comment 22

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-) from comment #19)
> So the only concern I had is this: what happens if you have a protocol
> handler that really wants its principal URI to be the URI that's passed to
> its newChannel method.  How can it do that in the new ordering?  

You mean - in the redirect case - handler dropping the rpURI to null so that originalURI will be used?  When rpURI is left null, we set it to what was the original URI (the redirect target URI - NOT the first URI in the chain).

I think (if I understand your concern correctly) this was not possible in the previous ordering!  We rewrote originalURI to the first URI in the redir chain, so, information lost.  It's actually better in the new ordering.

Comment 23

[Honza Bambas (:mayhemer)]

No need to keep this hidden.  This bug has been on Nightly only for a day or two tops.  The offending bug 1319111 has been swiftly backed out days ago already.

I will carry the fix there as a new version of its patch.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> It's actually better in the new ordering.

Yes, I think I agree.