Ciprian Georgiu, Desktop QA Reporter
	Description • 6 years ago

[Affected versions]:
- 63.0-build1 (20181015152800)
- 60.3.0esr-build1 (20181017185317)

[Affected platforms]:
- Windows 10 x64
- macOS 10.13
- Ubuntu 16.04 x64

[Steps to reproduce]:
1. Download Firefox 55.0.3 `en-US`, or any other locale: https://archive.mozilla.org/pub/firefox/candidates/55.0.3-candidates/build2/win64/es-ES/
2. Launch the browser on a clean profile.
3. Sign in to Firefox Accounts via “Welcome to Firefox” page.
- confirm the account if necessary and wait until sync is completed  
4. Update Firefox to 63.0 from “About Firefox” option.

[Expected result]:
- The first run/new tab page is properly displayed in Firefox.

[Actual result]:
- The first run/new tab page (for the en-US/es-ES locale) is displayed as a blank page and also there’s no link populated in the URL bar.

[Regression range]:
- I don't think this is a regression at least not a recent one, I managed to reproduce this issue while updating to FF 62.0.3 as well. I will investigate further, asap; in case of an old regression.

[Additional notes]:
- Please note, that this issue is reproducible on Firefox Release builds only if the user is logged into Sync before updating the browser to its latest version, and it happens with other builds too; e.g. 56.0-build6, `de` locale.
- On Esr builds, I’ve managed to reproduce it, without login into FxA first, e.g. when updating from 52.1.1esr `ko` and 52.3.0esr `fr` locales to 60.3.0-build1. It appears that the first run page is displayed blank in Firefox if the user is coming from an ESR version <52.x.x.
- If this helps please see logs from browser console: https://pastebin.com/bir3GcUT

	Robert Strong (they/them - no direct email)
	Comment 1 • 6 years ago

(In reply to Ciprian Georgiu [:ciprian_georgiu], Release Desktop QA from comment #0)
<snip>
> [Actual result]:
> - The first run/new tab page (for the en-US/es-ES locale) is displayed as a
> blank page and also there’s no link populated in the URL bar.
Hi Ciprian, the screenshot has the url in the url bar which is contrary to this statement. Can you confirm? Thanks!

	Robert Strong (they/them - no direct email)
	Comment 2 • 6 years ago

For the first run page all that app update does is save the xml data provided by balrog which includes the url as well as set a pref and there is Firefox specific code that opens the url. The screenshot shows that the url is opened so it appears that balrog is correctly providing the data and that the pref is set.

Given the above and with this only happening "on Firefox Release builds only if the user is logged into Sync before updating the browser to its latest version" I highly suspect this is caused by a recent change to Sync that is interacting badly with opening a new page on startup from the following Firefox code that provides the url.
https://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js#118

needinfoing dolske to give him a heads up.

	Ciprian Georgiu, Desktop QA Reporter
	Comment 3 • 6 years ago

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #1)
> (In reply to Ciprian Georgiu [:ciprian_georgiu], Release Desktop QA from
> comment #0)
> <snip>
> > [Actual result]:
> > - The first run/new tab page (for the en-US/es-ES locale) is displayed as a
> > blank page and also there’s no link populated in the URL bar.
> Hi Ciprian, the screenshot has the url in the url bar which is contrary to
> this statement. Can you confirm? Thanks!

Hey Robert, I think I had in mind this screenshot https://drive.google.com/open?id=11fwVKztlY3lJnYbRV8Unc9gdt53bhmKM when I wrote the last part of that AR; taken from an `es-ES` locale build, where the about:newtab page was displayed blank, after updating to the latest version. An issue that I have also encountered during this testing.

But yes, looking again at all the screenshots I’ve made for the first run page, the URL was present in the URL bar.

Let me know if I can help with more info. Thanks!

	Robert Strong (they/them - no direct email)
	Comment 4 • 6 years ago

Thanks Ciprian, this doesn't appear to be due to app update per comment #2 so changing component

	Ryan VanderMeulen [:RyanVM]
	Comment 5 • 6 years ago

Julie, is there someone on the Sync team who can take a look at this?

	Lina Butler [:lina]
	Comment 6 • 6 years ago

Hi Ciprian, would you mind testing with add-ons and preferences unchecked in about:preferences#sync, please? Desktop Sync is in maintenance mode; I can't think of any recent changes we've made here, but it might be that we're syncing a bogus pref or extension around. Thanks!

	Lina Butler [:lina]
	Comment 7 • 6 years ago

(In reply to Lina Cambridge (she/her) [:lina] from comment #6)
> Desktop Sync is in maintenance mode; I can't think of any recent changes we've made here...

Hmm, I was wrong. :-) This might be related to bug 1499766. Ciprian, instead of add-ons and preferences, could you please try disabling addresses and credit cards?

	Ciprian Georgiu, Desktop QA Reporter
	Comment 8 • 6 years ago

(In reply to Lina Cambridge (she/her) [:lina] from comment #7)
> (In reply to Lina Cambridge (she/her) [:lina] from comment #6)
> > Desktop Sync is in maintenance mode; I can't think of any recent changes we've made here...
> 
> Hmm, I was wrong. :-) This might be related to bug 1499766. Ciprian, instead
> of add-ons and preferences, could you please try disabling addresses and
> credit cards?

It seems that the issue is still reproducible even with the following prefs set on false, on FF 56.0: "extensions.formautofill.addresses.enabled" and "extensions.formautofill.creditCards.enabled" before updating to FF 63.0.

Also, it appears the issue is not repro if the user is signed into FxA via about:preferences, only from the first run page. 

Starting with FF 57.0 the user is redirected to about:newtab, after syncing with FxA via first run page, and the issue is no longer encountered, since upon restart about:newtab is opened instead of the first run page.

	:Gijs (he/him)
	Comment 9 • 6 years ago

The log has:

SessionHistory: Couldn't deserialize the triggeringPrincipal, falling back to NullPrincipal
Security Error: Content at moz-nullprincipal:{cd018427-ac2f-40c9-a11e-7014a45cdad4} may not load or link to about:home.

I think this needs a fix in session restore to use a different fallback value, or to realize that the session file comes from an old version that doesn't store triggering principals (or doesn't store them in a compatible format).

Passing this to Mike. This isn't a great experience for users, so if we could get a fix into 64 I think that'd be a Good Thing.

	:Gijs (he/him)
	Comment 10 • 6 years ago

I kind of expect this is fallout from bug 1461407, but then it doesn't really make sense that 60esr is affected.

Can you get a browser console log from the 60esr case and see if the same error crops up there?

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 11 • 6 years ago

> I think this needs a fix in session restore to use a different fallback value

Ah does ESR not have the triggeringPrincipal?

	:Gijs (he/him)
	Comment 12 • 6 years ago

(In reply to Jonathan Kingston [:jkt] from comment #11)
> > I think this needs a fix in session restore to use a different fallback value
> 
> Ah does ESR not have the triggeringPrincipal?

60esr would do, but 52 wouldn't, I guess. It's possible that the builds from which this suggests upgrading (52, 55, 56) didn't, I guess. I'm not sure off-hand exactly when we started storing these in session restore.

Thinking about it more, comment #10 is probably a red herring in that the *target* page is about:home/about:newtab, and the triggering principal might have been the first run page or something? I dunno.

A dump of the sessionstore file from the profile right before clicking the 'restart' button for the upgrade (waiting a minute or two before clicking it to ensure it's been written out) may be helpful in figuring out exactly what's missing. I don't have a setup where I can easily test this myself right now.

	Ciprian Georgiu, Desktop QA Reporter
	Comment 13 • 6 years ago

(In reply to :Gijs (he/him) from comment #10)
> I kind of expect this is fallout from bug 1461407, but then it doesn't
> really make sense that 60esr is affected.
> 
> Can you get a browser console log from the 60esr case and see if the same
> error crops up there?

I can see some errors related to session restore on esr build as well, e.g.: SessionHistory.jsm:456
TypeError: this._historyListener is null  ContentRestore.jsm:286:5. I have pasted here the entire browser console output from 60.3.0esr: https://pastebin.com/TuVUTH05. Hope this helps!

	:Gijs (he/him)
	Comment 14 • 6 years ago

(In reply to Ciprian Georgiu [:ciprian_georgiu], Release Desktop QA from comment #13)
> (In reply to :Gijs (he/him) from comment #10)
> > I kind of expect this is fallout from bug 1461407, but then it doesn't
> > really make sense that 60esr is affected.
> > 
> > Can you get a browser console log from the 60esr case and see if the same
> > error crops up there?
> 
> I can see some errors related to session restore on esr build as well, e.g.:
> SessionHistory.jsm:456
> TypeError: this._historyListener is null  ContentRestore.jsm:286:5. I have
> pasted here the entire browser console output from 60.3.0esr:
> https://pastebin.com/TuVUTH05. Hope this helps!

Thanks, this log also has:

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsISerializationHelper.deserializeObject]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/sessionstore/Utils.jsm :: deserializePrincipal :: line 143"  data: no]
NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsISHEntry.triggeringPrincipal]  SessionHistory.jsm:456

So yeah, somehow our principal serialization code changed and that's breaking this stuff.

	Mike de Boer [:mikedeboer]
	Comment 15 • 6 years ago

(In reply to :Gijs (he/him) from comment #9)
> The log has:
> 
> SessionHistory: Couldn't deserialize the triggeringPrincipal, falling back
> to NullPrincipal
> Security Error: Content at
> moz-nullprincipal:{cd018427-ac2f-40c9-a11e-7014a45cdad4} may not load or
> link to about:home.
> 
> I think this needs a fix in session restore to use a different fallback
> value, or to realize that the session file comes from an old version that
> doesn't store triggering principals (or doesn't store them in a compatible
> format).
> 
> Passing this to Mike. This isn't a great experience for users, so if we
> could get a fix into 64 I think that'd be a Good Thing.

I agree, but I have no clue about how this _should_ work and/ or what a proper fallback value would be here. I'm looping in the experts here, if I'm right.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 16 • 6 years ago

My view is that using system explicitly for the builds that we know are broken is completely OK. Is there any way we can target the change just to ESR though? I would us rather use the new improved security for all other releases where we can.
Do we have a pref for the session storage changes or a way to detect that?

	Christoph Kerschbaumer [:ckerschb]
	Comment 17 • 6 years ago

(In reply to Mike de Boer [:mikedeboer] from comment #15)
> (In reply to :Gijs (he/him) from comment #9)
> > The log has:
> > 
> > SessionHistory: Couldn't deserialize the triggeringPrincipal, falling back
> > to NullPrincipal
> > Security Error: Content at
> > moz-nullprincipal:{cd018427-ac2f-40c9-a11e-7014a45cdad4} may not load or
> > link to about:home.

Can we somehow limit the attack surface and just generate a different principal (other than a nullprincipal) as the fallback principal in case we are loading whatever about: URL? A general fallback to using the SystemPrincipal sounds possibly dangerous to me and I would like to just do 'enough' to make that corner case work.

Or are we going to face more problems similar to this one for other URLs? I would imagine not, because a NullPrincipal should be able to load whatever top-level web URL.