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

Categories

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

28 Branch
All
Windows 8.1
defect

Tracking

(firefox27 unaffected, firefox28 fixed, firefox29 fixed)

RESOLVED FIXED
Firefox 28
Tracking Status
firefox27 --- unaffected
firefox28 --- fixed
firefox29 --- fixed

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

()

Details

(Whiteboard: [beta28] p=3 [qa-])

Attachments

(2 files, 3 obsolete files)

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.
Attached file stack trace
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.
Blocks: 866304
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.
Attachment #8346725 - Flags: feedback?(dolske)
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.
Attached patch WIP v2 (obsolete) — Splinter Review
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.
Attachment #8346725 - Attachment is obsolete: true
Attachment #8346725 - Flags: feedback?(dolske)
Attachment #8346760 - Flags: feedback?(dolske)
Attached patch patch (obsolete) — Splinter Review
I realized there's a much cleaner way to fix this, without adding hacky prefs to nsPrompter.  Also added an automated test.
Assignee: nobody → mbrubeck
Attachment #8346760 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8346760 - Flags: feedback?(dolske)
Attachment #8347695 - Flags: review?(tabraldes)
I'd like to take this for it21 with points p=3.  Thanks!
Blocks: metrov1it21
No longer blocks: metrov1backlog
Flags: needinfo?(mmucci)
Whiteboard: [triage] → [beta28] p=3
Attached patch patchSplinter Review
Accidentally removed one line too many.
Attachment #8347695 - Attachment is obsolete: true
Attachment #8347695 - Flags: review?(tabraldes)
Attachment #8347697 - Flags: review?(tabraldes)
Added to IT#21.
Flags: needinfo?(mmucci)
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"
https://hg.mozilla.org/integration/fx-team/rev/b0a0fca79056
Flags: in-testsuite+
Whiteboard: [beta28] p=3 → [beta28] p=3 [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/b0a0fca79056
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+
Target Milestone: Firefox 29 → Firefox 28
Whiteboard: [beta28] p=3 → [beta28] p=3 [qa-]
You need to log in before you can comment on or make changes to this bug.