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

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Alice0775 White]

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

Comment 1

[Justin Dolske [:Dolske]]

Hmm, this sounds just like bug 886720.

Comment 2

[Frank Wein [:mcsmurf]]

Duh, did I/we blame the wrong regressing bug in Bug 886720? Looks like it, hrm...

Comment 3

[Frank Wein [:mcsmurf]]

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).

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: load the content.js frame script for preloaded browsers

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 12

[Justin Dolske [:Dolske]]

Is there a more-general patch in the works, or can we just go ahead and bandaid this for now?

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Brian Smith (:briansmith), was [e-mail] (: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

[:Gavin Sharp [email: [e-mail]]]

(In reply to Brian Smith (:briansmith), was [e-mail] (: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

[Justin Dolske [:Dolske]]

ttaubert: We really need a bandaid patch here, since this regression is now on beta.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Yes, will prepare a patch asap. I looked into more thorough methods of fixing this but to no avail.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Attachment: (beta, aurora) Load frame scripts for first preloaded browser before swapping

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.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gavin Sharp (use [e-mail] 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.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Load frame script for first preloaded browser before swapping, v2

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? :)

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 786877 [details] [diff] [review]
Load frame script for first preloaded browser before swapping, v2

Smaug, can you review the content/ changes?

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

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();

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 1 - add nsIFrameScriptLoader.getDelayedFrameScripts()

(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?

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 2 - Load pending frame scripts and discard browser after swapping

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/c2b4541bd6d3
https://hg.mozilla.org/integration/fx-team/rev/0b4c32636de7

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/c2b4541bd6d3
https://hg.mozilla.org/mozilla-central/rev/0b4c32636de7

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cbb6544f9769
https://hg.mozilla.org/releases/mozilla-beta/rev/f8089babbb85

Comment 30

[Paul Silaghi, QA [:pauly]]

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.

Comment 31

[Paul Silaghi, QA [:pauly]]

Reproduced on nightly 2013-07-01.
Verified fixed FF 25b2 Win 7.

Comment 32

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 26.0a2 (2013-10-03) Win 7