1301109 - LoginManagerPrompter initialization fails when called with a null window

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[aleth [:aleth]]

Since Bug 1266836, the loginManagerPrompter init() (and therefore also Prompter.getPrompt()) unintentionally throws when called with a null window. 
Allowing initialization with a null window remains safe as if anything actually tries to use a null window, it will still fail.

This has caused some xpcshell failures on c-c in tests completely unrelated to logins where the loginManagerPrompter init() ends up getting called during the event loop (Bug 1300059). I've tried investigating what's actually causing the getPrompt() call in these tests, but it's been surprisingly hard to find out.

There are also explicit calls to getPrompt() in the codebase with a null window argument, e.g. http://searchfox.org/mozilla-central/source/netwerk/base/nsPACMan.cpp#717

Comment 1

[aleth [:aleth]]

Attachment: Avoid LoginManagerPrompter initialization failures due to init() with a null window

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 8788951 [details] [diff] [review]
Avoid LoginManagerPrompter initialization failures due to init() with a null window

Can someone please review this half-line change.

You've changed the behaviour of the system here:
https://hg.mozilla.org/mozilla-central/rev/6a563348b8be#l5.13
which is not nice if you want to guarantee stable interfaces.

Since Aleth presented attachment 8788009 [details] [diff] [review] for review on 2016-09-05 (bug 1266836 comment #61) we're suffering from multiple Xpcshell test failures in our test suite. Bug 1217876 is also waiting for this fix and there appear to be other calls to GetPrompt() with a null argument
https://dxr.mozilla.org/comm-central/search?q=getprompt(null&redirect=false
(although I haven't checked the details).

Johann said that this was acceptable (bug 1266836 comment #62). The next branch date is coming in less than a week, so it would be good to settle the issue before then.

Comment 3

[Matthew N. [:MattN]]

The problem is that I think this change is going to hide real errors and it's not trivial to figure out a solution without a big context switch.

In order to review this I would have to audit that the prompt and login saving prompt still work with the change. e.g. does it fix bug 1217876 and get the doorhanger after? Can someone verify that for me?

Comment 4

[Jorg K (CEST = GMT+2)]

Andrea, you're interested in bug 1217876, can you provide that information? I'm just the poor guy trying to keep the Thunderbird Treeherder green (and this issue breaks some of our tests).

Comment 5

[Andrea Marchesini [:baku]]

The issue is this: XHR can be used by addons and they *could* not have a window object (they run in sandboxes). If the authentication prompt is needed, we must support the case where XHR doesn't have a proper window.
I see in the code a ModalPromper (is this what I suspect it is? - awesome), but I guess we must support non-modal-old-style prompts for addons/sandboxes. For more security, can we do some checks on the principal?

Comment 6

[Matthew N. [:MattN]]

I'm going to add more comments and write a test. Thanks for the starting point Aleth.

Comment 7

[Matthew N. [:MattN]]

Attachment: Bug 1301109 - Avoid LoginManagerPrompter initialization failures due to init() with a null window.

Also document the fact that there may be no ChromeWindow so this case is kept in mind.

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

Comment 8

[Matthew N. [:MattN]]

(In reply to Andrea Marchesini [:baku] from comment #5)
> The issue is this: XHR can be used by addons and they *could* not have a
> window object (they run in sandboxes). If the authentication prompt is
> needed, we must support the case where XHR doesn't have a proper window.
> I see in the code a ModalPromper (is this what I suspect it is? - awesome),
> but I guess we must support non-modal-old-style prompts for
> addons/sandboxes. 

I don't really know what you're asking but maybe I answered it already with my patch

> For more security, can we do some checks on the principal?

In the case where we have a null window we currently have nothing in pwmgr to check the principal of. Maybe you mean at a lower level but I don't know what you're trying to solve.

Comment 9

[Jorg K (CEST = GMT+2)]

(In reply to Matthew N. [:MattN] from comment #6)
> I'm going to add more comments and write a test. Thanks for the starting
> point Aleth.
Thanks Matt for getting traction on this and sorry for the impatience. Now all we need is have this land on mozilla51 before the branch date ;-)

Comment 10

[Johann Hofmann [:johannh]]

Comment on attachment 8791831 [details]
Bug 1301109 - Avoid LoginManagerPrompter initialization failures due to init() with a null window.

https://reviewboard.mozilla.org/r/79098/#review77794

r=me, although I'm not sure I've got enough knowledge of the pwdmgr tests to judge your test code

::: toolkit/components/passwordmgr/test/mochitest/test_prompt_noWindow.html:69
(Diff revision 1)
> +    req.open("GET", url, true);
> +    req.send(null);
> +  }
> +
> +  let loginModifiedPromise = promiseStorageChanged(["modifyLogin"]);
> +  sandbox.sandboxedRequest = new sandboxedRequest(url);

Nit: I don't think you need the 'new'

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8791831 [details]
Bug 1301109 - Avoid LoginManagerPrompter initialization failures due to init() with a null window.

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/651b30ec6c5c
Avoid LoginManagerPrompter initialization failures due to init() with a null window. r=johannh

Comment 13

[Matthew N. [:MattN]]

Attachment: Bug 1301109 - Follow-up to fix bustage caused by the string change in bug 1277895.

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

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8791831 [details]
Bug 1301109 - Avoid LoginManagerPrompter initialization failures due to init() with a null window.

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

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8792471 [details]
Bug 1301109 - Follow-up to fix bustage caused by the string change in bug 1277895.

https://reviewboard.mozilla.org/r/79514/#review78106

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0b4bad921cd
Follow-up to fix bustage caused by the string change in bug 1277895. r=bustage

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/651b30ec6c5c
https://hg.mozilla.org/mozilla-central/rev/e0b4bad921cd

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8791831 [details]
Bug 1301109 - Avoid LoginManagerPrompter initialization failures due to init() with a null window.

Approval Request Comment
[Feature/regressing bug #]: Bug 1266836
[User impact if declined]: No user impact, but a few tests failing in Thunderbird testing.
[Describe test coverage new/current, TreeHerder]: Yes, patch includes test.
[Risks and why]: Low. This just missed the branch date by a few hours.
[String/UUID change made/needed]: None.

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 8792471 [details]
Bug 1301109 - Follow-up to fix bustage caused by the string change in bug 1277895.

Approval Request Comment
Second part of patch. Need to go with other changeset.

Comment 20

[Matthew N. [:MattN]]

(In reply to Jorg K (GMT+2, never had any PTO during summer) from comment #18)
> [User impact if declined]: No user impact, but a few tests failing in
> Thunderbird testing.

This is incorrect. I believe there is user impact from the two consumers at https://dxr.mozilla.org/comm-central/search?q=getprompt(null+-path%3Atests&redirect=false which includes PAC and some other HTTP request. I think the user impact is that no auth prompt will be shown in those cases.

Comment 21

[Jorg K (CEST = GMT+2)]

(In reply to Matthew N. [:MattN] (In Taipei until Sep. 23) from comment #20)
> This is incorrect. I believe there is user impact ...
Sorry for the incorrect statement. So there's even more reason for the uplift ;-)

Comment 22

[Gerry Chang [:gchang]]

Comment on attachment 8791831 [details]
Bug 1301109 - Avoid LoginManagerPrompter initialization failures due to init() with a null window.

This patch fixes a regression. Take it in 51 aurora.

Comment 23

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c299490db559

Comment 24

[Matthew N. [:MattN]]

^ that had the follow-up fix merged with the original patch