Nightly: URL not updated in URL bar after a cross-domain redirect, document.URL neither (session store doesn't contain ResultPrincipalURI)

VERIFIED FIXED in Firefox 56

Status

()

defect
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: jakub.g.opensource, Assigned: mayhemer)

Tracking

Trunk
mozilla56
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(4 attachments)

Reporter

Description

2 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170628075643

Steps to reproduce:

Open URL which is a Facebook's open redirect

https://lm.facebook.com/l.php?u=https%3A%2F%2Fwww.lawlessfrench.com%2Fexpressions%2Ftout-de-suite%2F&h=ATMyA_7d0U0DMiKy_OLrRa2X-o4ptBrufLrzG-TKuTopilBcrCK8czYSZZiNVcVrneKg507jjeY8PKLGVx1lbu46N_QczCk8nX-t0mrDUnQiDInp3PBrcFsvv1iaoskdbzYxSEPe&enc=AZOiKr1Z5AZ1Zeq1_15ZE5ICLyV_uJCOqhtJdXE4tbio2DKXdD58NTSSrVGa98_0sL3bQ-7j_PPei8f-Vhyhv8Sf1sYaziLoDP2LPA5qyZIaiLMESIKrgefUv3Otg9BFehlVaJDyDgWpIoRTadhfbREqM_DSgNr95zHjfCLqfLtQ5-_aZ3eKhY_w3tWmjRDryGVLAFxjhRW2DugSfhM5bo-O0CKHByO6RC726BeVNEt2QkTZHbwRevpnHrlUd3Z9Ob4qVhVfC4F6h5TYsRPgKG_nnvt7EmaXBlPl0u4AibVeGGjtKmG_BM7DEQWcKbEumY1-idwmMxL-XzGEfCo28wga&s=1


Actual results:

The page is loaded, but the URL bar shows the URL from before redirect (different domain)

Also document.URL, document.domain, window.origin point to Facebook

I only managed to reproduce it once, maybe it has something to do with session restore


Expected results:

Expected URL to be displayed in URL bar

https://www.lawlessfrench.com/expressions/tout-de-suite/
Reporter

Comment 1

2 years ago
Info from about:support:

Version: 56.0a1
Build ID: 20170710030203
Update Channel: nightly
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
OS: Windows_NT 6.1
Multiprocess Windows: 1/1 (Enabled by default)
Component: Untriaged → Networking: HTTP
Product: Firefox → Core
Component: Networking: HTTP → Document Navigation
Reporter

Comment 2

2 years ago
Posted image Screenshot
Reporter

Updated

2 years ago
OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64
I can't reproduce with the link in comment 0.

Do you have any extensions or addons?  Is there a service worker installed for facebook?
Flags: needinfo?(jakub.g.opensource)
Reporter

Comment 4

2 years ago
Hello, I don't know how to reliably repro the issue.

I can still reproduce the issue in one of the tabs I have opened earlier today (even if I reboot Nightly), but not when I open a new tab.

What is interesting is that with F5 or CTRL-R:

- the page is reloaded (fine)
- I see a HTTP request to "https://www.lawlessfrench.com/expressions/tout-de-suite/" in Fiddler (fine)
- but still, the old URL (one from Facebook) is still displayed in the URL bar

I have 0 addons, default theme enabled, 2 video plugins which I believe are shipped by default (OpenH264 and Widevine)

Not sure about service workers, I will try to dig into it
Flags: needinfo?(jakub.g.opensource)
Reporter

Comment 5

2 years ago
- about:debugging#workers says no services workers are installed
- CTRL-SHIFT-DELETE > time range: everything, ticked everything > cleared
- CTRL-SHIFT-R in the offending tab

Result:

still reproducible in the offending tab
Reporter

Comment 6

2 years ago
I will try to provide you with a Firefox profile which reproduces the bug later today.
Reporter

Comment 7

2 years ago
I managed to create a MWE with just two files. I can not easily go further because I'm stuck on decompresing jsonlz4 file.

Reproducible on Win7 and Win10 64-bit.

1. Create a new profile in Nightly.
2. Open the profile folder from about:support.
3. Close Nightly.
4. Delete all the files and folders in the profile folder.
5. Copy-paste the 2 files I attached (js and jsonlz4).
6. Open Nightly using given profile.
7. Switch to the first tab.
8. Observe the bug.
Reporter

Comment 8

2 years ago
Posted file sessionstore.jsonlz
Reporter

Comment 9

2 years ago
Posted file sessionstore.js
Can you take a look at this?  Maybe some unexpected fallout from the session store compression?
Flags: needinfo?(mdeboer)
Reporter

Comment 11

2 years ago
There's no issue with decompression, I just didn't know how do it on Windows.

I finally decompressed the file with https://github.com/avih/dejsonlz4

Relevant part of the JSON where probably the problem lies:

                    "entries": [
                        {
                            "url": "https://www.lawlessfrench.com/expressions/tout-de-suite/",
                            "title": "Tout de suite - Lawless French Expression - Essential French",
                            "charset": "UTF-8",
                            "ID": 855308652,
                            "docshellUUID": "{ee241bfc-bebd-41c1-b07a-a460894c22b1}",
                            "referrer": "https://m.facebook.com/",
                            "referrerPolicy": 4,
                            "originalURI": "https://lm.facebook.com/l.php?u....[CUT]",
                            "resultPrincipalURI": null,
                            "loadReplace2": true,
                            "principalToInherit_base64": "vQZuXxRvRHKD...[CUT]",
                            "triggeringPrincipal_base64": "ZT4OTT7kRfqycpf...[CUT]",
                            "docIdentifier": 1,
Well, it's more likely to be _possible_ fallout of bug 1380012... Honza, would you care to glance over this and see if it hits a chord? Thanks!
Flags: needinfo?(mdeboer) → needinfo?(honzab.moz)
I think the bug linked in comment 12 is wrong?
Argh! I meant bug 1319111. Thanks, Ben.
Assignee

Comment 15

2 years ago
Thanks for this report and the files!

Yeah, resultPrincipalURI should be set to https://www.lawlessfrench.com/expressions/tout-de-suite/ as this is a redirect channel, clearly.  It's been lost from some reason before it's been stored to the session store.  I'm suspecting some parts of the docshell code are not setting it when session restores it and it then gets back nullified.

We need steps to reproduce this, to find out how to get a session store that doesn't have the rpURI set (or set to null).

I'll make it my priority.
Assignee: nobody → honzab.moz
Blocks: 1319111
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(honzab.moz)
Assignee

Comment 16

2 years ago
Theory: nsDocShell::AddToSessionHistory is called for a channel that doesn't have load info assigned.  But I'm not sure it's even possible these days...

I'll try to reproduce this locally by fiddling with redirects, session restore, going back and forward etc.

jakub, could you try to remember what have you been doing with that particular page before this started to manifest?  It might help narrowing it down.  Thanks.
Flags: needinfo?(jakub.g.opensource)
Summary: Nightly: URL not updated in URL bar after a cross-domain redirect, document.URL neither → Nightly: URL not updated in URL bar after a cross-domain redirect, document.URL neither (session store doesn't contain ResultPrincipalURI)
Reporter

Comment 17

2 years ago
Hello,

I don't remember what I exactly did which triggered this. I'm not sure also if I noticed it immediately, or with some delay.

Basically, I had a handful of tabs opened long time ago.
Then I kept doing the following yesterday a few times:

- open Nightly
- open m.facebook.com, browse
- open some links from Facebook in the new tab and/or in the same tab (probably always in new tab but not 100% sure)
- close Nightly
- repeat

I will keep using Nightly in this way and if I see that issue again, I will come back with more info.
Flags: needinfo?(jakub.g.opensource)
Reporter

Comment 18

2 years ago
I encountered the issue again 2 more times but didn't found any reliable nor even semi-reliable way to have it happen on demand.

In case you didn't pay attention, the behavior observed from UI side is as follows:

- First I see the URL bar has good URL ("url" from session restore entry)
- After half a second, I see it gets changed to the wrong one ("originalURI" from session restore entry)

AFAIU there are two things here:

1) wrong data is written to session store data => to be investigated why

> resultPrincipalURI should be set (..) as this is a redirect channel, clearly.  It's been lost from some reason before it's been stored to the session store. 

2) "originalURI" makes its way to the UI when "resultPrincipalURI" is null => maybe more checks should be put in place?

In fact I'm really baffled why the "originalURI" ever gets displayed in the UI. Since the redirect from A to B is in progress, shouldn't the browser just assume the URL is B and forget about A altogether, especially when doing session restore? (apart from the back button stack, but this is a separate story)

Probably you have some reasons to do it this way, but it's a bit surprising looking from external world.
Assignee

Comment 19

2 years ago
c18
Flags: needinfo?(honzab.moz)
Assignee

Comment 20

2 years ago
(In reply to jakub.g from comment #18)
> - First I see the URL bar has good URL ("url" from session restore entry)
> - After half a second, I see it gets changed to the wrong one ("originalURI"
> from session restore entry)

Yeah, I missed this point!  This is very interesting.  Seems like we do load the data correctly, but then during the page reconstruction process we loose the result URI and then also rewrite it in the session store.  Thanks!
Flags: needinfo?(honzab.moz)
Assignee

Comment 21

2 years ago
One question: have you reproduced with the same URL or a different one?
Flags: needinfo?(jakub.g.opensource)
Reporter

Comment 22

2 years ago
It was with different URLs but similar scenario, I opened links from m.facebook.com in a new tab.
Flags: needinfo?(jakub.g.opensource)
Assignee

Comment 23

2 years ago
I can reproduce locally:
- new profile
- set it to restore
- delete sessionstore.jsonlz4
- paste [1] decompressed as sessionstore.js with the following modification:
  

{
    "version": ["sessionrestore", 1],
    "windows": [
    {
        "tabs": [
        {
            "entries": [
            {
...
+                "resultPrincipalURI": "https://www.lawlessfrench.com/expressions/tout-de-suite/",

which SHOULD make the session restore load the rpURI correctly.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8885406
Assignee

Comment 24

2 years ago
My favorite mistake!  Add a new class member and forget to update the class copy ctor...  This time nsSHEntry.  Only surprising is the this doesn't manifest massively.
Status: NEW → ASSIGNED
Assignee

Comment 25

2 years ago
Back to the drawing board... I probably made a mistake and copied to the profile the unmodified sessionstore.js with already missing rpURI, still it seems a bit strange to me...

The copy ctor of nsSHEntry is never called, so it's not the cause of this bug at all (tho, worth fixing.)
Status: ASSIGNED → NEW
Assignee

Comment 26

2 years ago
OTOH, I can see there is a usage of the copy ctor, [1].  Not sure what are the circumstances, but it could be the source of this bug.

Let's try if it fixes the problem.

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


[1] https://dxr.mozilla.org/mozilla-central/search?q=path%3AnsDocShell.cpp+%27-%3EClone(&redirect=false
Attachment #8891552 - Flags: review?(bzbarsky)
Assignee

Updated

2 years ago
Status: NEW → ASSIGNED
Comment on attachment 8891552 [details] [diff] [review]
v1 (missing nsSHEntry rpURI copy ctor initiator)

r=me
Attachment #8891552 - Flags: review?(bzbarsky) → review+
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 28

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e89bb12f8ea
Copy nsSHEntry::mResultPrincipalURI in its copy constructor. r=bz
Keywords: checkin-needed

Comment 29

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5e89bb12f8ea
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Is this actually fixed?  The comments above didn't sound like we'd confirmed this patch fixes the bug...

Reopening for now to make sure we don't lose track of this.
Status: RESOLVED → REOPENED
Flags: needinfo?(honzab.moz)
Resolution: FIXED → ---
Assignee

Comment 31

2 years ago
I think we can close this (the patch has landed) and let jakub verify the fix.  If it's found unfixed, I will reopen.

Jakub, can you please monitor this for some time and check if you observe the bug again?  There is no way for us to verify it's fixed.  Thanks.

Leaving ni? on me to not loose the track.
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Flags: needinfo?(honzab.moz) → needinfo?(jakub.g.opensource)
Resolution: --- → FIXED
Assignee

Updated

2 years ago
Flags: needinfo?(honzab.moz)
Depends on: 1388192
Assignee

Comment 32

2 years ago
Jakub, any news?
Reporter

Comment 33

2 years ago
Hi, I was off for a while and didn't use Nightly.

I updated and, using the original profile which I used to report the bug, I still see the wrong URLs in the old tabs (the ones opened few weeks ago). Not sure if this was part of the bugfix or it is assumed it won't happen again?

Regarding the behavior for the new tabs, I didn't observe the bug yet. Will keep you updated if it happens again.
Flags: needinfo?(jakub.g.opensource)
Assignee

Comment 34

2 years ago
(In reply to jakub.g from comment #33)
> Hi, I was off for a while and didn't use Nightly.
> 
> I updated and, using the original profile which I used to report the bug, I
> still see the wrong URLs in the old tabs (the ones opened few weeks ago).
> Not sure if this was part of the bugfix or it is assumed it won't happen
> again?
> 
> Regarding the behavior for the new tabs, I didn't observe the bug yet. Will
> keep you updated if it happens again.

Hi, I completely missed this comment, sorry!

We *have not* fixed this for old tabs - it's wrongly persisted and it's left there broken until you close the tab.

We *have* fixed this for new tabs, so based on that I think this seems to be fixed.  Thanks for keeping an eye on it.

Based on comment 33 I believe we can mark this verified for now.  Jakub, if it happens again *for a new tab*, please reopen this bug (or better file a new one, since here we have landed a fix here for an existing an legit issue already)

Thanks!
Status: RESOLVED → VERIFIED
Flags: needinfo?(honzab.moz)
You need to log in before you can comment on or make changes to this bug.