949333 - Defect - Metro Firefox fails to submit login forms if profile has a Master Password

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Components, defect, P2)

Description

[Matt Brubeck (:mbrubeck)]

Steps to reproduce:
1) In desktop Firefox, open the Options window.
2) Under Security, activate the Master Password feature.
3) Relaunch in Metro mode.
4) In Metro Firefox, go to a page with a login form.
5) Try to log in.

Expected results: You can submit the form and try to log in to the site.

Actual results: When you try to submit the form, nothing happens.

It looks like this is similar to bug 948478 and bug 945829. In a debug build, the following messages are logged:

MetroWidget::Create
eWindowType_dialog window requested, returning failure.
[18784] WARNING: Invalid window type requested.: file c:/Users/mbrub_000/src/elm/widget/windows/winrt/MetroWidget.cpp, line 239

Note: We used to have a Master Password UI in Metro; it was removed by bug 854881.  Some of that code might be useful for this bug.

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: stack trace

This is happening because PK11PasswordPrompt is calling into toolkit's nsPrompter.js instead of browser/metro/components/PromptService.js.

Comment 2

[Matt Brubeck (:mbrubeck)]

(In reply to Matt Brubeck (:mbrubeck) from comment #1)
> This is happening because PK11PasswordPrompt is calling into toolkit's
> nsPrompter.js instead of browser/metro/components/PromptService.js.

The security code calls nsWindowWatcher::GetNewPrompter which calls Metro's PromptService.getPrompt which then delegates to nsPrompter.js (as of bug 866304).

It then calls nsPrompter's promptPassword implementation.  Because it doesn't pass a document associated with a tab, it won't create a tab modal prompt.  Instead it falls back to openModalWindow, which doesn't work on Metro because WinRT widget code can't create new windows.

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: WIP: force master password prompt to be tab-modal on Metro

This patch (untested) takes prompts that do not pass any domWindow, and turns them into tab-modal prompts for the foreground tab, in Metro Firefox only.

This is an extension of what Metro is already doing for prompts that pass a domWindow not associated with any tab; we retarget those to the selected tab.  We previously discussed issues related to that in bug 866304 comment 16 and 24.

Desktop Firefox already does something similar to this patch:  When a prompt is created without passing a DOM window, desktop Firefox associates it with the active window.  Based on that, I don't think the code creating these prompts is relying on non-reentrance, since it can already be called again from a different window while the prompt is showing.

Longer-term we may need to support window-modal dialogs in Metro Firefox.  (Either that, or banish window-modal dialogs completely!)  But until then, is this a viable way to prevent us silently failing when prompting for a Master Password?

If not, an alternative might be to disable the password manager (and Firefox Sync, by extension) in Metro Firefox when a Master Password is set.

Comment 4

[Matt Brubeck (:mbrubeck)]

I think I'll also try to whip up a patch for window-modal prompts in Metro, to see if that's a workable alternative.

Comment 5

[Matt Brubeck (:mbrubeck)]

Attachment: WIP v2

Fixed a bug in the previous WIP patch; this one is tested and works.

(In reply to Matt Brubeck (:mbrubeck) from comment #4)
> I think I'll also try to whip up a patch for window-modal prompts in Metro,
> to see if that's a workable alternative.

Tim Abraldes convinced me we want to avoid this if possible, and instead continue converting window-modal prompts to tab-modal prompts and fixing any problems this causes with consumers.  This will provide a better UX for Metro, and hopefully help with eventually eliminating window-modal dialogs on desktop.

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: patch

I realized there's a much cleaner way to fix this, without adding hacky prefs to nsPrompter.  Also added an automated test.

Comment 7

[Matt Brubeck (:mbrubeck)]

I'd like to take this for it21 with points p=3.  Thanks!

Comment 8

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Accidentally removed one line too many.

Comment 9

[Marco Mucci [:MarcoM]]

Added to IT#21.

Comment 10

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 8347697 [details] [diff] [review]
patch

Review of attachment 8347697 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! Tested on http://people.mozilla.org/~mbrubeck/test/password.html

There is an issue we should fix as a follow-up bug.

In DesktopFx:
  1. Open a page that causes the master password prompt to appear
  2. Dismiss the master password prompt
  3. Open another tab
  4. Switch to metroFx
The master password prompt will now appear over the wrong page.

Comment 11

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

> There is an issue we should fix as a follow-up bug.

To be clear, I meant "as a follow-up bug, but not necessarily for v1"

Comment 12

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/fx-team/rev/b0a0fca79056

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b0a0fca79056

Comment 14

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8347697 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 924860

User impact if declined: Password Manager fails silently in Metro Firefox if user has a Master Password, leaving some web forms broken.

Testing completed (on m-c, etc.): Landed on m-c on 12/16; includes an automated test.

Risk to taking this patch (and alternatives if risky): Low-risk Metro-only patch.

String or IDL/UUID changes made by this patch: None.

Comment 15

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5be7ada24379