Closed Bug 1301109 Opened 3 years ago Closed 3 years ago

LoginManagerPrompter initialization fails when called with a null window

Categories

(Toolkit :: Password Manager, defect, P1)

51 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox48 --- unaffected
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: aleth, Assigned: MattN)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

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
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Blocks: 1300059
Version: unspecified → 51 Branch
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.
Attachment #8788951 - Flags: review?(jhofmann)
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?
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).
Flags: needinfo?(amarchesini)
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?
Flags: needinfo?(amarchesini) → needinfo?(MattN+bmo)
I'm going to add more comments and write a test. Thanks for the starting point Aleth.
Assignee: aleth → MattN+bmo
OS: Unspecified → All
Priority: -- → P1
Hardware: Unspecified → All
Attachment #8788951 - Attachment is obsolete: true
Attachment #8788951 - Flags: review?(jhofmann)
Attachment #8788951 - Flags: review?(MattN+bmo)
(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.
Flags: needinfo?(MattN+bmo)
(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 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'
Attachment #8791831 - Flags: review?(jhofmann) → review+
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/651b30ec6c5c
Avoid LoginManagerPrompter initialization failures due to init() with a null window. r=johannh
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
Attachment #8792471 - Flags: review?(MattN+bmo) → review+
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/e0b4bad921cd
Follow-up to fix bustage caused by the string change in bug 1277895. r=bustage
https://hg.mozilla.org/mozilla-central/rev/651b30ec6c5c
https://hg.mozilla.org/mozilla-central/rev/e0b4bad921cd
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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.
Attachment #8791831 - Flags: approval-mozilla-aurora?
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.
Attachment #8792471 - Flags: approval-mozilla-aurora?
(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.
(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 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.
Attachment #8791831 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8792471 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
^ that had the follow-up fix merged with the original patch
You need to log in before you can comment on or make changes to this bug.