Closed Bug 1274042 Opened 4 years ago Closed 3 years ago

[e10s] "Use Password Manager to remember this password" checkbox shouldn't appear on HTTP auth dialogs

Categories

(Toolkit :: Password Manager, defect, P2)

48 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
e10s + ---
firefox47 --- unaffected
firefox48 --- wontfix
firefox49 --- fixed
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: MattN, Assigned: saadq)

References

(Blocks 1 open bug, )

Details

(Keywords: regression, Whiteboard: [passwords:http-auth])

Attachments

(3 files)

The "Use Password Manager to remember this password" checkbox is not supposed to show for HTTP auth dialogs in Firefox as we instead only ask to remember via a doorhanger notification if the login was actually successful (technically it appears then quickly gets removed). The doorhanger doesn't appear after a successful login either.

The e10s behaviour should work like non-e10s. Marking as an e10s regression.

STR
1) Load http://httpbin.org/basic-auth/user/passwd (correct username is user, password is passwd)

Expected result:
HTTP auth dialog appears without a checkbox (see "Expected result screenshot"). Upon successful login (user:passwd), the remember login doorhanger should appear asking to save the password.
Upon entering the incorrect password, no doorhanger icon should remain (it may appear then disappear right away)

Actual result:
HTTP auth dialog appears with a checkbox "Use Password Manager to remember this password". I don't know if it works but it shouldn't be there in the first place as it means we will save logins even if they are wrong.

I believe that checkbox may be needed for certain cases like HTTP auth from Thunderbird so it's not that the checkbox should be removed but it shouldn't be used in this case. The non-e10s behaviour is what we should match.
Summary: [e10s] "Use Password Manager to remember this password" checkbox shouldn't apepar on HTTP auth dialogs → [e10s] "Use Password Manager to remember this password" checkbox shouldn't appear on HTTP auth dialogs
(In reply to Matthew N. [:MattN] (behind on reviews) from comment #0)
> The doorhanger doesn't appear after a successful login either.

Clarification: The doorhanger doesn't appear after a successful login if the checkbox isn't checked. It does appear if it is checked so this is not the end of the world, users who want to be able to save will need to check the box in e10s and also confirm in the doorhanger.
Priority: -- → P2
Not block since this we still confirm before saving the information. We'd like to get this working like non-e10s so tracking + P2.
Calling this a regression in 48 since that's the first release going to GA.
Version: unspecified → 48 Branch
Flags: needinfo?(MattN+bmo)
Saad is going to investigate what's going on.
Assignee: nobody → saad
Status: NEW → ASSIGNED
Flags: needinfo?(MattN+bmo)
Whiteboard: [passwords:http-auth]
https://reviewboard.mozilla.org/r/69316/#review66476

::: toolkit/components/passwordmgr/test/browser/browser.ini:59
(Diff revision 1)
>  [browser_passwordmgr_switchtab.js]
>  [browser_passwordmgrdlg.js]
> +[browser_http_auth_prompt.js]

Nit: keep alphabetical ordering please

::: toolkit/components/passwordmgr/test/browser/browser_http_auth_prompt.js:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Nit: since you only changed a few lines it may have been useful to have this file be an `hg copy` with modifications.
It looks like there are a few unit tests having issues for the prompt here with my changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e8d8d3f9afe&selectedJob=25134189

I'll look into that.
It looks like the reason those tests were failing was because of a couple places where this code was used:

    if (isE10S) {
      state.checkHidden = true;
      state.checkMsg = "Use Password Manager to remember this password."
    }

I'm not sure if this is actually the expected result or if it was just a temporary thing to handle the checkbox existing in e10s.
(In reply to Saad Quadri [:saadq] from comment #9)
> I'm not sure if this is actually the expected result or if it was just a
> temporary thing to handle the checkbox existing in e10s.

I would recommend looking at the history of those lines to find out.
^ Woops, the above code should be:

    if (isE10S) {
      state.checkHidden = false;
      state.checkMsg = "Use Password Manager to remember this password."
    }

In other words, the test was assuming that the checkbox SHOULD be displayed for e10s. After reading some comments from the link below, I believe this was just there to make the test work for e10s.

https://bugzilla.mozilla.org/show_bug.cgi?id=1230462
Comment on attachment 8777906 [details]
Bug 1274042 - Fix checkbox for HTTP auth dialogs in e10s.

https://reviewboard.mozilla.org/r/69314/#review68498

Note to self: Saad says _getNotifyBox() is failing because the last line in that function [notifyBox = chromeWin.getNotificationBox(notifyWin);] is failing in E10S, I suppose in theory we should figure out how to make that code work in E10S, but it's kind of moot for Firefox since we always prefer to use popup notifications (doorhangers). Also kind of amusing that this bug wouldn't have ever happened if we had made this fix back when we first added _getPopupNote() -- we used it everywhere else except here, apparently.
Attachment #8777906 - Flags: review?(dolske) → review+
Comment on attachment 8777907 [details]
Bug 1274042 - Remove unnecessary edge case handling for E10S mode in prompt tests.

https://reviewboard.mozilla.org/r/69316/#review68504

I think we can skip adding the new "browser_http_auth_prompt.js" test -- the existing test_subresources_prompts.html ends up actually covering (as you found on try :), and eventually the normal prompt tests will fully cover this when we get them running in E10S. So no real need to add this extra test for this specific case.
Attachment #8777907 - Flags: review?(dolske) → review+
(In reply to Justin Dolske [:Dolske] from comment #15)

> I suppose in theory we should figure out how to make that
> code work in E10S, but it's kind of moot for Firefox since we always prefer
> to use popup notifications (doorhangers).

Ah, so, getNotificationBox() fails because it's looking for which tab has the specified (content) window, but we're working with ChromeWindows in E10S mode, and so that doesn't work.

We should probably rip out the the notification bar stuff from password manager, and require other apps (SM/TB) to implement the popup notifications API (if not the actual UI, mapping those calls to show a notification bar). But that's fodder for a different bug.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8ebdc1289aa1
Fix checkbox for HTTP auth dialogs in e10s. r=Dolske
https://hg.mozilla.org/integration/autoland/rev/e4ed73577c76
Remove unnecessary edge case handling for E10S mode in prompt tests. r=Dolske
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8ebdc1289aa1
https://hg.mozilla.org/mozilla-central/rev/e4ed73577c76
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Hi :Saadq, :MattN,
Since this bug is a regression and also affects 49/50, are you also considering to uplift this patch to 49/50?
Flags: needinfo?(saad)
Flags: needinfo?(MattN+bmo)
Dolske, how long do you think this should bake before uplift?
Flags: needinfo?(MattN+bmo) → needinfo?(dolske)
I don't think this needs any bake time. Well-understood bug/patch, there's decent test coverage, and since this prompt gets a lot of usage at Mozilla we'll quickly notice if something somehow breaks. :)
Flags: needinfo?(dolske)
I was also thinking about the notification bar case but I'm not sure if non-Firefox apps test with m-c code.
Comment on attachment 8777906 [details]
Bug 1274042 - Fix checkbox for HTTP auth dialogs in e10s.

Approval Request Comment
[Feature/regressing bug #]: E10S
[User impact if declined]: The user sees a checkbox on HTTP auth dialogs that they shouldn't see. If they don't check it, we won't ask to save the password.
[Describe test coverage new/current, TreeHerder]: Adjusted existing test to catch this
[Risks and why]: "Well-understood bug/patch, there's decent test coverage, and since this prompt gets a lot of usage at Mozilla we'll quickly notice if something somehow breaks."
[String/UUID change made/needed]: None
Flags: needinfo?(saad)
Attachment #8777906 - Flags: approval-mozilla-beta?
Attachment #8777906 - Flags: approval-mozilla-aurora?
Comment on attachment 8777907 [details]
Bug 1274042 - Remove unnecessary edge case handling for E10S mode in prompt tests.

Approval Request Comment: See comment 26
Attachment #8777907 - Flags: approval-mozilla-beta?
Attachment #8777907 - Flags: approval-mozilla-aurora?
Comment on attachment 8777906 [details]
Bug 1274042 - Fix checkbox for HTTP auth dialogs in e10s.

Fix for e10s related regression from 48. Let's uplift this for beta 6.
Attachment #8777906 - Flags: approval-mozilla-beta?
Attachment #8777906 - Flags: approval-mozilla-beta+
Attachment #8777906 - Flags: approval-mozilla-aurora?
Attachment #8777906 - Flags: approval-mozilla-aurora+
The second patch doesn't apply cleanly to beta, did some file get renamed or newly created in 50+?
Flags: needinfo?(saad)
Attachment #8777907 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Wes Kocher (:KWierso) from comment #29)
> The second patch doesn't apply cleanly to beta, did some file get renamed or
> newly created in 50+?

The test file was renamed in bug 1230462 and that's what added the lines that are being deleted so it's fine to land without that file being touched for Beta (even though it means we don't get the same test coverage on Beta).
Flags: needinfo?(saad)
Attachment #8777907 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.