Closed Bug 1380012 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Navigation, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

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

References

Details

Attachments

(4 files)

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/
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
Attached image Screenshot
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)
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)
- 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
I will try to provide you with a Firefox profile which reproduces the bug later today.
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.
Attached file sessionstore.jsonlz
Attached file sessionstore.js
Can you take a look at this?  Maybe some unexpected fallout from the session store compression?
Flags: needinfo?(mdeboer)
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.
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)
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)
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)
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.
c18
Flags: needinfo?(honzab.moz)
(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)
One question: have you reproduced with the same URL or a different one?
Flags: needinfo?(jakub.g.opensource)
It was with different URLs but similar scenario, I opened links from m.facebook.com in a new tab.
Flags: needinfo?(jakub.g.opensource)
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
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
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
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)
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+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/5e89bb12f8ea
Status: ASSIGNED → RESOLVED
Closed: 7 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 → ---
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
Closed: 7 years ago7 years ago
Flags: needinfo?(honzab.moz) → needinfo?(jakub.g.opensource)
Resolution: --- → FIXED
Flags: needinfo?(honzab.moz)
Jakub, any news?
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)
(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.