Username and password will not always auto-fill despite these having already been remembered by built-in Password manager

VERIFIED FIXED in Firefox 24

Status

()

defect
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: alice0775, Assigned: ttaubert)

Tracking

({dev-doc-needed, dogfood, regression})

24 Branch
Firefox 26
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox23 unaffected, firefox24+ verified, firefox25+ verified, firefox26 verified)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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

6 years ago
Version: 25 Branch → 24 Branch
(Reporter)

Updated

6 years ago
Component: Tabbed Browser → Password Manager
Product: Firefox → Toolkit
Hmm, this sounds just like bug 886720.
Duh, did I/we blame the wrong regressing bug in Bug 886720? Looks like it, hrm...
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).
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

6 years ago
Component: Password Manager → Tabbed Browser
Product: Toolkit → Firefox
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+
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.
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.
Duplicate of this bug: 889618
Duplicate of this bug: 886720
Duplicate of this bug: 893039
Is there a more-general patch in the works, or can we just go ahead and bandaid this for now?
Flags: needinfo?(ttaubert)
Blocks: 839961
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
(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)
(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).
ttaubert: We really need a bandaid patch here, since this regression is now on beta.
Yes, will prepare a patch asap. I looked into more thorough methods of fixing this but to no avail.
Flags: needinfo?(ttaubert)
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 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

6 years ago
Attachment #786753 - Flags: review?(jaws)
(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.
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)
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 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 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+
(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

6 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
Attachment #787326 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/mozilla-central/rev/c2b4541bd6d3
https://hg.mozilla.org/mozilla-central/rev/0b4c32636de7
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Attachment #786753 - Flags: approval-mozilla-beta?
Attachment #786753 - Flags: approval-mozilla-beta+
Attachment #786753 - Flags: approval-mozilla-aurora?
Attachment #786753 - Flags: approval-mozilla-aurora+
Keywords: verifyme
QA Contact: paul.silaghi
Reproduced on nightly 2013-07-01.
Verified fixed FF 25b2 Win 7.
Verified fixed FF 26.0a2 (2013-10-03) Win 7
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.