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

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P2)

Description

[Matthew N. [:MattN]]

Attachment: Expected result screenshot

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.

Comment 1

[Matthew N. [:MattN]]

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

Comment 2

[Jim Mathies [:jimm]]

Not block since this we still confirm before saving the information. We'd like to get this working like non-e10s so tracking + P2.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Calling this a regression in 48 since that's the first release going to GA.

Comment 4

[Matthew N. [:MattN]]

Saad is going to investigate what's going on.

Comment 5

[Saad Quadri [:saadq]]

Attachment: Bug 1274042 - Fix checkbox for HTTP auth dialogs in e10s.

Review commit: https://reviewboard.mozilla.org/r/69314/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69314/

Comment 6

[Saad Quadri [:saadq]]

Attachment: Bug 1274042 - Remove unnecessary edge case handling for E10S mode in prompt tests.

Review commit: https://reviewboard.mozilla.org/r/69316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69316/

Comment 7

[Matthew N. [:MattN]]

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.

Comment 8

[Saad Quadri [:saadq]]

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.

Comment 9

[Saad Quadri [:saadq]]

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.

Comment 10

[Matthew N. [:MattN]]

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

Comment 11

[Saad Quadri [:saadq]]

^ 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 12

[Matthew N. [:MattN]]

Yep, bug 1230462 comment 61 and later discuss this.

Comment 13

[Saad Quadri [:saadq]]

Comment on attachment 8777906 [details]
Bug 1274042 - Fix checkbox for HTTP auth dialogs in e10s.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69314/diff/1-2/

Comment 14

[Saad Quadri [:saadq]]

Comment on attachment 8777907 [details]
Bug 1274042 - Remove unnecessary edge case handling for E10S mode in prompt tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69316/diff/1-2/

Comment 15

[Justin Dolske [:Dolske]]

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.

Comment 16

[Justin Dolske [:Dolske]]

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.

Comment 17

[Justin Dolske [:Dolske]]

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

Comment 18

[Saad Quadri [:saadq]]

Comment on attachment 8777906 [details]
Bug 1274042 - Fix checkbox for HTTP auth dialogs in e10s.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69314/diff/2-3/

Comment 19

[Saad Quadri [:saadq]]

Comment on attachment 8777907 [details]
Bug 1274042 - Remove unnecessary edge case handling for E10S mode in prompt tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69316/diff/2-3/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8ebdc1289aa1
https://hg.mozilla.org/mozilla-central/rev/e4ed73577c76

Comment 22

[Gerry Chang [:gchang]]

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?

Comment 23

[Matthew N. [:MattN]]

Dolske, how long do you think this should bake before uplift?

Comment 24

[Justin Dolske [: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. :)

Comment 25

[Matthew N. [:MattN]]

I was also thinking about the notification bar case but I'm not sure if non-Firefox apps test with m-c code.

Comment 26

[Matthew N. [:MattN]]

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

Comment 27

[Matthew N. [:MattN]]

Comment on attachment 8777907 [details]
Bug 1274042 - Remove unnecessary edge case handling for E10S mode in prompt tests.

Approval Request Comment: See comment 26

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

The second patch doesn't apply cleanly to beta, did some file get renamed or newly created in 50+?

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6d735a27df8f
https://hg.mozilla.org/releases/mozilla-aurora/rev/41fb33de0efc

Comment 31

[Matthew N. [:MattN]]

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

Comment 32

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fe76857ae374
https://hg.mozilla.org/releases/mozilla-beta/rev/0f76572d37ef