1685337 - UI freeze and crash when Basic Auth prompter opens after "are you sure you want to quit" dialog is open

Status: RESOLVED FIXED

(Firefox :: Security, defect)

Description

[Ori Avtalion (salty-horse)]

Attachment: Python server to simulate this crash

Using latest Nightly.

Steps to reproduce:

1. Visit a website that requires Basic Authentication.
2. Before that website loads, try closing the browser.
3. Attempting to close Firefox opens the "Are you sure you want to close all tabs" dialog. Wait for the website to open in the background and the Basic Authentication dialog to appear.
4. Close the "Are you sure you want to quit" dialog. The Firefox UI will now be frozen.
5. Close Firefox again and confirm the "are you sure you want to close all tabs" dialog.
6. The window will close, but the process will keep running until it will crash.

This can happen naturally when you open Firefox with many tabs, when one of them requires basic authentication, and immediately try to close it, before the tab with Basic Authentication loads.

I'm attaching a simple python web server that delays its Basic Authentication by 5 seconds. It serves a website at localhost:8000

Submitted crash: https://crash-stats.mozilla.org/report/index/940026ed-7517-49a2-9d98-100c70210106

Comment 1

[Ori Avtalion (salty-horse)]

Is this related to the new tab-modal prompts?

Comment 2

[Emma Zühlcke [:emz]]

Thanks for filing!

This might be an issue with how we're spinning nested event loops. Nested event loops are used to wait for prompt results so that callers can prompt synchronously: https://searchfox.org/mozilla-central/rev/c59d9181cbcd8356ce9271723e31be11641e7010/toolkit/components/prompts/src/Prompter.jsm#1077
They are also used in the code of the window prompts to wait for user input.
When experimenting with different prompt types in the past, I've noticed that a new nested event loop of a tab prompt that opens while a window prompt is open can stop the event loop of the window prompt. This leads to the window prompt not responding to user input anymore. In the past I was never able to reproduce this in a real world scenario though. This looks like one.

There are different approaches to this:

1. Figure out how we can spin nested event loops without them locking up each other. For example we could suppress tab prompts while window prompts are open.
2. Switch over all (affected?) prompt callers to use the async prompt methods, which don't need to spin a nested event loop. This is easy for JS callers. We have some C++ core consumers though that might not support this.
3. Switch away from window prompts, there is an idea for this in Bug 1685313.

Bug 1679116 seems to be a related issue.

Gijs, do you think this could be fixed via Bug 1685313? Would this cover all cases?

Comment 3

[:Gijs (he/him)]

(In reply to Paul Zühlcke [:pbz] from comment #2)

Thanks for filing!

This might be an issue with how we're spinning nested event loops. Nested event loops are used to wait for prompt results so that callers can prompt synchronously: https://searchfox.org/mozilla-central/rev/c59d9181cbcd8356ce9271723e31be11641e7010/toolkit/components/prompts/src/Prompter.jsm#1077
They are also used in the code of the window prompts to wait for user input.
When experimenting with different prompt types in the past, I've noticed that a new nested event loop of a tab prompt that opens while a window prompt is open can stop the event loop of the window prompt. This leads to the window prompt not responding to user input anymore. In the past I was never able to reproduce this in a real world scenario though. This looks like one.

There are different approaches to this:

1. Figure out how we can spin nested event loops without them locking up each other. For example we could suppress tab prompts while window prompts are open.

And do what instead? Break the http auth in the meantime, as we can't prompt for credentials? Waiting would involve spinning the event loop, which we don't want to do... :-)

ISTR having a similar discussion with Olli somewhere but I can't find the bug. Something to do with multiple alert() calls (which also (used to?) spin event loops). It isn't a straightforward problem to solve.

2. Switch over all (affected?) prompt callers to use the async prompt methods, which don't need to spin a nested event loop. This is easy for JS callers. We have some C++ core consumers though that might not support this.

Indeed, the http auth prompt is a C++ consumer. Something is spinning the event loop there, I'm fairly sure. I don't know how hard it would be to make the processing there truly async, so that we could drop the event loop in that case. I'm guessing Valentin has a better grasp of that problem. Needinfo for this. :-)

3. Switch away from window prompts, there is an idea for this in Bug 1685313.

This would still likely involve a spun event loop, because the quit code is implemented as a series of sync callbacks that can object to quitting.

We could try fixing that, of course, and support async callbacks, but that's a non-trivial amount of work and as you point out may not solve the general problem. It'd probably fix this bug, though?

Bug 1679116 seems to be a related issue.

Gijs, do you think this could be fixed via Bug 1685313? Would this cover all cases?

I don't think so, see above. There are 2 nested event loops. Removing either would fix the bug. I don't know which of those projects would be easier, but removing the one for the quit dialog is an orthogonal issue to where the prompt displays, because of what the prompt is designed to do: provide a synchronous response as to whether quitting is allowed or not.

Comment 4

[Emma Zühlcke [:emz]]

This is the event loop that gets interrupted: https://searchfox.org/mozilla-central/rev/31ddf859c57e812878a5f817e4097efb06de4d97/xpfe/appshell/AppWindow.cpp#510
Once the auth prompt opens it stops spinning. That leaves the parent window in a disabled state. EnableParent is never called.

I've also tested the bug with window auth prompts (prompts.modalType.httpAuth set to 3). The behavior there is also a bit unexpected. If I confirm the tab warning prompt the browser doesn't exit, but stays open until the auth prompt is closed. Normally the auth prompt takes exclusive focus, but in this case I can still focus the main window. However, it's disabled via the mechanism mentioned above.

Comment 5

[Valentin Gosu [:valentin]]

(In reply to :Gijs (he/him) from comment #3)

2. Switch over all (affected?) prompt callers to use the async prompt methods, which don't need to spin a nested event loop. This is easy for JS callers. We have some C++ core consumers though that might not support this.

Indeed, the http auth prompt is a C++ consumer. Something is spinning the event loop there, I'm fairly sure. I don't know how hard it would be to make the processing there truly async, so that we could drop the event loop in that case. I'm guessing Valentin has a better grasp of that problem. Needinfo for this. :-)

The HTTP auth should already be async - the sync fallback is from back when we still had to deal with XPCOM addons.
The only other instances are in the FTP code, but that should be going away soon.

Comment 6

[Emma Zühlcke [:emz]]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #5)

The HTTP auth should already be async - the sync fallback is from back when we still had to deal with XPCOM addons.

The auth prompts are async from nsHttpChannelAuthProvider to the nsIAuthPrompt implementation (LoginManagerAuthPrompter). However, after that we still spin a nested event loop, because we use the sync prompt method of nsIPromptService. My patch for Bug 1684469 should fix this soon.

Comment 7

[Emma Zühlcke [:emz]]

With the changes we made to auth prompting in Bug 1684469 I can no longer reproduce this issue.
Reporter, could you test if you can still reproduce with the latest Nightly? I've tested with 87.0a1 (2021-01-27)

Comment 8

[Ori Avtalion (salty-horse)]

I can confirm the bug no longer reproduces. Firefox stays responsive after canceling out of the quit dialog, and if I choose to quit, the process terminates correctly.

Thanks! I'll leave you to set the correct resolution status.

Comment 9

[Emma Zühlcke [:emz]]

https://bugzilla.mozilla.org/show_bug.cgi?id=1684469#c24