Closed Bug 949333 Opened 6 years ago Closed 6 years ago
Defect - Metro Firefox fails to submit login forms if profile has a Master Password
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.  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.
This is happening because PK11PasswordPrompt is calling into toolkit's nsPrompter.js instead of browser/metro/components/PromptService.js.
(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.
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.
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.
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.
I realized there's a much cleaner way to fix this, without adding hacky prefs to nsPrompter. Also added an automated test.
I'd like to take this for it21 with points p=3. Thanks!
Accidentally removed one line too many.
Added to IT#21.
Priority: -- → P2
QA Contact: jbecerra
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.
Attachment #8347697 - Flags: review?(tabraldes) → review+
> 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"
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [beta28] p=3 [fixed-in-fx-team] → [beta28] p=3
Target Milestone: --- → Firefox 29
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.
Attachment #8347697 - Flags: approval-mozilla-aurora?
Attachment #8347697 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.