Closed
Bug 888972
Opened 11 years ago
Closed 11 years ago
Username and password will not always auto-fill despite these having already been remembered by built-in Password manager
Categories
(Firefox :: Tabbed Browser, defect)
Tracking
()
VERIFIED
FIXED
Firefox 26
People
(Reporter: alice0775, Assigned: ttaubert)
References
Details
(Keywords: dev-doc-needed, dogfood, regression)
Attachments
(3 files, 2 obsolete files)
1.94 KB,
patch
|
Gavin
:
review+
bajaj
:
approval-mozilla-aurora+
bajaj
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
4.06 KB,
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
3.16 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
Build Identifier: http://hg.mozilla.org/mozilla-central/rev/d7553251cf43 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130701 Firefox/25.0 ID:20130701031115 Steps To Reproduce: 0. Make sure that you should register a single Google username/password into built-in Password Manager. Open https://accounts.google.com/ and Sign-in Click "Remember Password" button in notification door hanger Sign-ont 1. Restart Browser 2. Open https://accounts.google.com/ServiceLogin?passive=1209600&continue=https%3A%2F%2Faccounts.google.com%2FManageAccount&followup=https%3A%2F%2Faccounts.google.com%2FManageAccount --- observe username/password fields 3. Open a New tab and Close existing tab 4. Repeat from step 2 several times Actual Results: Auto fill fails. No username/password appears. Expected Results: username/password should filled automatically. Regression window(m-c) Good: http://hg.mozilla.org/mozilla-central/rev/def4320f90aa Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130621 Firefox/24.0 ID:20130621193802 Bad: http://hg.mozilla.org/mozilla-central/rev/cea75ce9a559 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130621 Firefox/24.0 ID:20130621211209 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=def4320f90aa&tochange=cea75ce9a559 Regression window(fx-team) Good: http://hg.mozilla.org/integration/fx-team/rev/2b9573964077 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130621 Firefox/24.0 ID:20130621104322 Bad: http://hg.mozilla.org/integration/fx-team/rev/8f5749eb49f6 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130621 Firefox/24.0 ID:20130621112402 Pushlog: http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=2b9573964077&tochange=8f5749eb49f6 Regressed by: 8f5749eb49f6 Tim Taubert β Bug 791670 - part 1 - flip the browser.newtab.preload pref; r=jaws And I confirm that Setting browser.newtab.preload = false fixes the problem
![]() |
Reporter | |
Updated•11 years ago
|
Version: 25 Branch → 24 Branch
![]() |
Reporter | |
Updated•11 years ago
|
Component: Tabbed Browser → Password Manager
Product: Firefox → Toolkit
Comment 2•11 years ago
|
||
Duh, did I/we blame the wrong regressing bug in Bug 886720? Looks like it, hrm...
Comment 3•11 years ago
|
||
Oh, nevermind, I see. There are/were two issues: The bugzilla one mentioned in Bug 886720 (which for me always happened) and the rather random issue with (for example) Google login fields not getting autofilled (probabably this bug here then).
Assignee | ||
Comment 4•11 years ago
|
||
Great find. This is because the first <xul:browser> that is created in the hiddenWindow does not load the content.js frame script and thus we don't fire necessary events that the LoginManagerContent module needs. Should be an easy fix.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86_64 → All
Assignee | ||
Updated•11 years ago
|
Component: Password Manager → Tabbed Browser
Product: Toolkit → Firefox
Comment 6•11 years ago
|
||
Comment on attachment 770197 [details] [diff] [review] load the content.js frame script for preloaded browsers Review of attachment 770197 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/modules/BrowserNewTabPreloader.jsm @@ +297,5 @@ > this._browser.setAttribute("type", "content"); > this._browser.setAttribute("src", NEWTAB_URL); > doc.getElementById("win").appendChild(this._browser); > + > + // Load the content script used by the tabbrowser. Replace this comment with, // The first <xul:browser> that is created in the hiddenWindow // does not load the content.js frame script and thus we don't // fire necessary events that the LoginManagerContent module needs.
Attachment #770197 -
Flags: review?(jaws) → review+
Comment 7•11 years ago
|
||
Hmm, what about content-sessionStore.js? Or any other frame script loaded in the browser.js context? Seems like we need some way to avoid this more generally.
Assignee | ||
Comment 8•11 years ago
|
||
Yeah... I didn't land it yet because the same thoughts occurred to me as well. Custom content scripts like Panorama etc wouldn't be available. OTOH, content scripts active in the hiddenWindow might probably not behave like we expect them to.
Updated•11 years ago
|
Comment 12•11 years ago
|
||
Is there a more-general patch in the works, or can we just go ahead and bandaid this for now?
Flags: needinfo?(ttaubert)
Comment 13•11 years ago
|
||
It seems like this (or a related bug, or the mail.mozilla.com upgrade) is causing mail.mozilla.com to never auto-complete my password. Further, after I do manually log in, I am not offered the opportunity to updated the saved password.
Keywords: dogfood
Comment 14•11 years ago
|
||
(In reply to Brian Smith (:briansmith), was bsmith@mozilla.com (:bsmith) from comment #13) > It seems like this (or a related bug, or the mail.mozilla.com upgrade) is > causing mail.mozilla.com to never auto-complete my password. Further, after > I do manually log in, I am not offered the opportunity to updated the saved > password. As a workaround, you can give focus to the username field and then press tab to the password field. It will then get filled in. (This is actually a nice security feature :P)
Comment 15•11 years ago
|
||
(In reply to Brian Smith (:briansmith), was bsmith@mozilla.com (:bsmith) from comment #13) > It seems like this (or a related bug, or the mail.mozilla.com upgrade) is > causing mail.mozilla.com to never auto-complete my password. That was caused by the zimbra upgrade, unfortunately (it specifies autocomplete="off" on the password field, so we don't auto-fill it, but we will fill it if you enter the user name manually or use Jared's workaround).
Comment 16•11 years ago
|
||
ttaubert: We really need a bandaid patch here, since this regression is now on beta.
Assignee | ||
Comment 17•11 years ago
|
||
Yes, will prepare a patch asap. I looked into more thorough methods of fixing this but to no avail.
Flags: needinfo?(ttaubert)
Assignee | ||
Comment 18•11 years ago
|
||
Ok, this really is a band-aid patch. It loads the two default frame scripts and also the Panorama one. I've long tried to come up with a real fix only to find out that this probably is not too easy. Ideally, we would preload in the hidden window and set some flag on the browser that prevents the docShell from loading frame scripts, should someone ever add some to the hidden window. The second step would then be to kick off loading all pending frame scripts registered on the target window shortly after we swapped the frameLoader into the newly opened tab/browser.
Attachment #770197 -
Attachment is obsolete: true
Attachment #786753 -
Flags: review?(jaws)
Attachment #786753 -
Flags: review?(gavin.sharp)
Comment 19•11 years ago
|
||
Comment on attachment 786753 [details] [diff] [review] (beta, aurora) Load frame scripts for first preloaded browser before swapping Seems like we should put a comment next to the browser.js loadFrameScript callers to indicate that any additions to that list likely need to be duplicated here as well. Can we test this? If we had a way to enumerate the frame scripts loaded for a given messageManager, we could use that to load all the needed scripts here, which might at least be a nicer temporary hack. I suppose we should file a followup to have this work automagically on frameloader swaps. It would be interesting to see where you ran into roadblocks.
Attachment #786753 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Updated•11 years ago
|
Attachment #786753 -
Flags: review?(jaws)
Assignee | ||
Comment 20•11 years ago
|
||
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #19) > Seems like we should put a comment next to the browser.js loadFrameScript > callers to indicate that any additions to that list likely need to be > duplicated here as well. Good idea. > Can we test this? This will not be easy to test as we'd need to open a new tab that receives the first docShell swapped from the hidden window. We'll definitely not be the first test that will open a new tab and receives a preloaded docShell. > If we had a way to enumerate the frame scripts loaded for a given > messageManager, we could use that to load all the needed scripts here, which > might at least be a nicer temporary hack. Yes, I thought about this too. This would maybe even be sufficient without blocking frame scripts from the hidden window, assuming no one puts stuff in there that causes problems. Or any stuff at all. > I suppose we should file a followup to have this work automagically on > frameloader swaps. It would be interesting to see where you ran into > roadblocks. Yes.
Assignee | ||
Comment 21•11 years ago
|
||
Alright, here's another version that achieves the same goal. It does three things: 1) To account for different frame scripts per window (e.g. one with panorama loaded, one without) the hidden browser is discarded after the docShells have been swapped. A new one will be created that will start preloading off a timer as usual. 2) It adds nsIFrameScriptLoader.getDelayedFrameScripts() that returns the list of pending frame scripts attached to the global or window message manager. It doesn't support frame message managers as the list of frame scripts may be incomplete. 3) Just as the other patch it adds the frame scripts retrieved from the target window's message manager to the frame message manager of the newly opened tab. This patch is a lot more solid but also a little more complex and includes an IDL change. I tested it manually and it works as expected and fixes the issue described here. Thoughts? :)
Attachment #786877 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 22•11 years ago
|
||
Comment on attachment 786753 [details] [diff] [review] (beta, aurora) Load frame scripts for first preloaded browser before swapping [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 791670 User impact if declined: First preloaded tab in a session does not support form completion by login manager. Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): Low risk, band-aid patch. String or IDL/UUID changes made by this patch: None. I talked to Gavin and we think that we should land the band-aid patch on Beta and Aurora. The more thorough patch (v2) will be landed on Nightly. The v2 patch is a lot more complex and has IDL changes so it's not the perfect patch to be uplifted, but it is the better long-term solution.
Attachment #786753 -
Flags: approval-mozilla-beta?
Attachment #786753 -
Flags: approval-mozilla-aurora?
Comment 23•11 years ago
|
||
Comment on attachment 786877 [details] [diff] [review] Load frame script for first preloaded browser before swapping, v2 Smaug, can you review the content/ changes?
Attachment #786877 -
Flags: review?(bugs)
Comment 24•11 years ago
|
||
Comment on attachment 786877 [details] [diff] [review] Load frame script for first preloaded browser before swapping, v2 >+nsFrameMessageManager::GetDelayedFrameScripts(nsIDOMDOMStringList** aList) >+{ >+ // Frame message managers may return an incomplete list because scripts >+ // that were loaded after it was connected are not added to the list. >+ if (!IsGlobal() && !IsWindowLevel()) { >+ return NS_ERROR_NOT_IMPLEMENTED; >+ } Could you add some NS_WARNING before returning >+ >+ nsRefPtr<nsDOMStringList> scripts = new nsDOMStringList(); >+ >+ for (uint32_t i = 0; i < mPendingScripts.Length(); ++i) { >+ scripts->Add(mPendingScripts[i]); >+ } >+ >+ *aList = scripts; >+ NS_ADDREF(*aList); scripts.swap(aList); or *aList = script.forget().get();
Attachment #786877 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 25•11 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #24) > >+ *aList = scripts; > >+ NS_ADDREF(*aList); > scripts.swap(aList); > or > *aList = script.forget().get(); scripts.forget(aList) works too, right?
Attachment #786877 -
Attachment is obsolete: true
Attachment #786877 -
Flags: review?(gavin.sharp)
Attachment #787325 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #786753 -
Attachment description: Load frame script for first preloaded browser before swapping → (beta, aurora) Load frame scripts for first preloaded browser before swapping
Updated•11 years ago
|
Attachment #787326 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 27•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/c2b4541bd6d3 https://hg.mozilla.org/integration/fx-team/rev/0b4c32636de7
Assignee | ||
Comment 28•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c2b4541bd6d3 https://hg.mozilla.org/mozilla-central/rev/0b4c32636de7
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Updated•11 years ago
|
Keywords: dev-doc-needed
Updated•11 years ago
|
Attachment #786753 -
Flags: approval-mozilla-beta?
Attachment #786753 -
Flags: approval-mozilla-beta+
Attachment #786753 -
Flags: approval-mozilla-aurora?
Attachment #786753 -
Flags: approval-mozilla-aurora+
Comment 29•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/cbb6544f9769 https://hg.mozilla.org/releases/mozilla-beta/rev/f8089babbb85
Comment 30•11 years ago
|
||
Tested on: https://accounts.google.com/ https://www.facebook.com/ https://twitter.com/ https://wiki.mozilla.org/index.php?title=Special:UserLogin&returnto=Main+Page http://jira.softvision.ro/secure/Dashboard.jspa Verified fixed FF 24b4 Win 7 x64.
Updated•11 years ago
|
QA Contact: paul.silaghi
You need to log in
before you can comment on or make changes to this bug.
Description
•