User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0 Build ID: 20160702030219 Steps to reproduce: 0. Have a Firefox Account (FA) ready. 1. Sign into a FA in about:preferences#sync, or from hamburger menu, in a new profile. 2. Open Browser Console. Turn on only JS log to reduce noises. 3. Visit any website, e.g. http://example.com Actual results: A log "Invalid chrome URI: /" appears in Browser Console after the page is loaded. Reloading the page will generate a new but same log. If I sign out the FA and reload webpage again, no new logs are created. (In my main profile, there are indeed 2 instances of the log created from every page load. Signing in FA only accounts for 1 instance of the log. I can reduce 1 instance by logging off my FA. I have yet to find out the cause of the other instance.) I do suspect that the culprit may not be FA/Sync function, but a defected preference being sync across my testings. I, however, have not planned to set up another clean FA if this issue is reproducible by other FAs.

From Mozregression: Last good Nightly: 2016-06-25 First bad Nightly: 2016-06-26 Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0e073f5ca38a002d43e92016ee40d686da4a0534&tochange=c2da34d96746288b5fee27bf6542a12c9f410988 Inbound: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=15f7b6d288b65d2f1c4abe7c9da7869427cb3270&tochange=0009a43ad49a316c86cdb52bcb44006b32011170 Suspect: Bug 1281529 followup - Don't call mallocSizeOf on a base class pointer. I did mozregression twice but it provides the same range, even though the bug sounds unrelated. I tested on a inbound build[1] which contain bug 1281529 part 9 (it should be on 07-03 Nightly) but my bug still persists. [1]: https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-win32/1467459982/

(In reply to Fanolian from comment #0) > I do suspect that the culprit may not be FA/Sync function, but a defected > preference being sync across my testings. I, however, have not planned to > set up another clean FA if this issue is reproducible by other FAs. Please disregard this. If a defected preference is already synced to a new profile, the bug should still exist after logging out of FA since the defected preference stays.

I also see the "Invalid chrome URI: /" message, without any file location. will look into this.

I have a different regression window: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=bb34ab2ea11bd3b292ef5122e0212ab3abf19c74&tochange=1d1b4e27b684390c385937c4dbade2c9db04aab9 Regressed by: Bug 1127579

Here's JS backtrace for the error, on fdcee57b4e4f revision #0 resource://gre/modules/LoginHelper.jsm:192 #1 NightlyDebug.app/Contents/Resources/components/storage-json.js:307 #2 NightlyDebug.app/Contents/Resources/components/storage-json.js:342 #3 NightlyDebug.app/Contents/Resources/components/storage-json.js:265 #4 NightlyDebug.app/Contents/Resources/components/nsLoginManager.js:391 #5 resource://gre/modules/LoginHelper.jsm:165 #6 resource://gre/modules/LoginManagerParent.jsm:521 #7 self-hosted:1086 #8 resource://gre/modules/Task.jsm:319 #9 self-hosted:931 #10 resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937 #11 resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816 #12 self-hosted:889 #13 self-hosted:889 #14 resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:750

https://dxr.mozilla.org/mozilla-central/rev/fdcee57b4e4f66a82831ab01e61500da98a858e8/toolkit/components/passwordmgr/LoginHelper.jsm#199 > let loginURI = Services.io.newURI(aLoginOrigin, null, null); > let searchURI = Services.io.newURI(aSearchOrigin, null, null); > if (loginURI.scheme == "http" && searchURI.scheme == "https" && > loginURI.hostPort == searchURI.hostPort) { > return true; > } > } catch (ex) { > // newURI will throw for some values e.g. chrome://FirefoxAccounts > return false; aLoginOrigin is "chrome://FirefoxAccounts" there, and the comment says it will throw, and it does. it would be better filtering such case.

I found the same results with mozregression as in comment 1. I dont use sync.

There are 4 places that is known to throw. We should handle them before calling newURI instead of adding comment, to suppress negligible error message. https://dxr.mozilla.org/mozilla-central/search?q=chrome%3A%2F%2FFirefoxAccounts&case=true&=mozilla-central

Here's draft patch to filter out "chrome://FirefoxAccounts" before calling newURI. can I have some feedback on it? also, what kind of test could be done for the interaction between "Browser Console", "Password Manager", and "Firefox Account" ? is there any example available in-tree?

This is just paving over the root problem which is that an invalid URI was chosen to store FxA credentials. This keeps getting in my way as I'm increasingly needing an nsIURI from the login hostname. Mark, how hard would it be to change to a valid chrome URI? A quick GitHub search shows that iOS doesn't use it and because supposedly this credential isn't synced by the password engine I think we could do the following which is slightly higher risk that paving over it but prevents the problem from recurring in new code: 1) Migrate any "chrome://FirefoxAccounts" login hostname to "chrome://FirefoxAccounts/content/" or "chrome://FirefoxAccounts/content/keys", for example. 2) Add "chrome://FirefoxAccounts" to getSyncCredentialsHostsLegacy. 3) Change FXA_PWDMGR_HOST to the new hostname. https://dxr.mozilla.org/mozilla-central/search?q=FXA_PWDMGR_HOST&redirect=false

Yeah, I agree that if we can change the credentials, we should change it :)

I can't see any problem with that approach. Somewhat related is bug 1213510, where we'd like to hide this login from the UI - depending on how that ended up being implemented, we could possibly use a real (eg, https) URL - the only real reason it's chrome:// is to try and make it obvious it's not a regular password. Another option might be to use "resource://FirefoxAccounts" - that seems to be a valid URL and it's arguable that the average user wouldn't understand what "chrome://" means in this context so *any* prefix other than http[s] etc would serve the same purpose.

(In reply to Mark Hammond [:markh] from comment #12) > Another option might be to use > "resource://FirefoxAccounts" - that seems to be a valid URL and it's > arguable that the average user wouldn't understand what "chrome://" means in > this context so *any* prefix other than http[s] etc would serve the same > purpose. Oops - ignore that, I misread Matt's comment - chrome://FirefoxAccount/credentials" or similar would be fine.

Comment on attachment 8768591 [details] [diff] [review] Filter out known problematic URIs in Password Manager. okay, then I withdraw this patch.

Tooru, since I outlined the steps to fix this, it's fairly straightforward other than testing, and I don't have time to fix this shortly, would you be interested in submitting a patch? (In reply to Mark Hammond [:markh] from comment #13) > (In reply to Mark Hammond [:markh] from comment #12) > > Another option might be to use > > "resource://FirefoxAccounts" - that seems to be a valid URL and it's > > arguable that the average user wouldn't understand what "chrome://" means in > > this context so *any* prefix other than http[s] etc would serve the same > > purpose. > > Oops - ignore that, I misread Matt's comment - > chrome://FirefoxAccount/credentials" or similar would be fine. chrome://FirefoxAccount/credentials isn't valid since the first part of the path needs to be content/skin/etc. I'd be fine with a different resource URI or an https one, as long as its a valid nsIURI. Then we can enforce valid nsIURI in addLogin to prevent this from happening again.

I'm afraid I'm not capable of properly implementing migration works in short term. so I'd like to leave this bug to others.

I can't see a simple way to migrate this that will work if the user happens to "downgrade" their profile - we will have written the credentials with the new URL and the older channel will see no credentials and re-prompt for a login. An approach we could consider would be to write a patch that looks for *both* new and old URL, but always writes to the "old" name and uplift that as far as we dare. Another patch that rides the trains then writes to the new name and deletes the old (ie, migrates). This still leaves an edge-case if the user downgrades after changing their password, but that might be OK to live with. However, I don't have a good understanding of the problems the status-quo has, and whether the timing of such a strategy will actually help in practice.

As long as we're trying to use old credentials, newURI will report the error message to browser console. so, with the approach, we still need attachment 8768591 [details] [diff] [review] or something like that to suppress the error message, until we remove the code related to old credential completely.

While we could fix Sync, I'm pretty sure a number of addons also use chrome://<foo>/, so it seems like that would remain an issue. Is there a reason this isn't a valid nsIURI? Could we just fix the chrome URI handler to allow this?

Hi, this is tagged as a regression and 50 is now Aurora. What can we do to get it fixed so we can keep up our "don't ship regressions" policy?

(In reply to Justin Dolske [:Dolske] from comment #19) > While we could fix Sync, I'm pretty sure a number of addons also use > chrome://<foo>/, so it seems like that would remain an issue. You mean manually through the API? It's not possible to have a page at chrome://<foo>/ since the chrome scheme requires another path component after the package name. > Is there a reason this isn't a valid nsIURI? Could we just fix the chrome > URI handler to allow this? Because it's meaningless for the chrome URI scheme? I don't see why we would want to allow this when the URI wouldn't be resolvable to anything or useful for anything. See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/The_Chrome_URL I'd rather not have to special case certain URIs and would rather break compatibility so we can use nsIURI everywhere.

I indirectly fixed the spew on every page load by fixing bug 1291346 and that already got uplifted to all affected versions. We can deal with the issue again when it comes up.