#1709 HTTP basic auth can block page loads

Open
opened 3 months ago by Moonchild · 18 comments

This is apparently fallout from making the prompt async way-back-when out of the concern that multiple basic auth prompts would be “confusing” to the user.
While we don’t have a major issue here unlike Mozilla (since we use modal prompts that block window interaction, and not tab-modal prompts that can be easily ignored) it is still an issue with window-centric browsing: If an http basic auth prompt is opened and ignored in one browser window, any other browser window will be unable to load other sites with basic auth until the first prompt is dealt with (either cancelled or completed).

Kind of a corner case but should still be fixed. Probably easiest to just revert this prompt to being synchronous so there’s no “queue” of prompts.

This is apparently fallout from making the prompt async way-back-when out of the concern that multiple basic auth prompts would be "confusing" to the user. While we don't have a major issue here unlike Mozilla (since we use modal prompts that block window interaction, and not tab-modal prompts that can be easily ignored) it is still an issue with window-centric browsing: If an http basic auth prompt is opened and ignored in one browser window, any other browser window will be unable to load other sites with basic auth until the first prompt is dealt with (either cancelled or completed). Kind of a corner case but should still be fixed. Probably easiest to just revert this prompt to being synchronous so there's no "queue" of prompts.
Moonchild added the
Bug
label 3 months ago
Moonchild added the
C: Networking
label 3 months ago
Moonchild added the
C: UI
label 3 months ago
Moonchild added the
Good first issue
label 3 months ago
Moonchild added the
Low Priority
label 3 months ago
Moonchild added the
Low Risk
label 3 months ago
mattatobin commented 3 months ago
Collaborator

I am good with this being done. Async modal promps/dialogs sounds like a disaster planned for the future in to justify change to something that didn’t need changed. Typical Mozilla.

I am good with this being done. Async modal promps/dialogs sounds like a disaster planned for the future in to justify change to something that didn't need changed. Typical Mozilla.
athenian200 self-assigned this 3 months ago
athenian200 commented 3 months ago
Collaborator

I figured out when and where this was done, and can kinda follow the history of the issue on Bugzilla. I feel like I have a reasonable chance of figuring this out because I’m getting used to looking at the older parts of the code and seeing how they evolved into their current forms.

I can’t say with confidence that it will be ready by the next release, but if it goes the way I expect it and nothing else comes up in real life, it shouldn’t take more than two days.

I figured out when and where this was done, and can kinda follow the history of the issue on Bugzilla. I feel like I have a reasonable chance of figuring this out because I'm getting used to looking at the older parts of the code and seeing how they evolved into their current forms. I can't say with confidence that it will be ready by the next release, but if it goes the way I expect it and nothing else comes up in real life, it shouldn't take more than two days.
Moonchild commented 3 months ago
Owner

It shouldn’t have to take much at all. The synchronous call is still there as a fallback, and we can just use that instead, I think.

https://repo.palemoon.org/MoonchildProductions/UXP/src/branch/master/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp#L1208-L1228

It shouldn't have to take much at all. The synchronous call is still there as a fallback, and we can just use that instead, I think. https://repo.palemoon.org/MoonchildProductions/UXP/src/branch/master/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp#L1208-L1228
athenian200 commented 3 months ago
Collaborator

Well, the C++ part seems to be fairly straightforward so far.

https://bugzilla.mozilla.org/show_bug.cgi?id=475053

The async-related parts of the final patch on here are still basically possible to undo in the C++ section of the code, after accounting for the fact that nsHttpChannel was moved to a different directory and most of the logic was split out into nsHttpChannelAuthProvider.

The thing that’s giving me trouble is the nsLoginManagerPrompter part of this. There’s a lot of logic in that part that I don’t quite know what to do with, because there’s a password manager/master key functionality that was built on top of the async code. The code they added for that feature seems kind of bloated and difficult to follow, and the original function was so simple that I’m not sure it can do everything required here. It’s possible I’m just overthinking all this, but I don’t want to accidentally remove or break a feature that’s tangled up with the async code.

The original bug added an observer to the LoginManagerPromptFactory function, which originally had no observers. There have been two additional observers added on top of that since, both related to passwordmgr-crypto-login.

http://xref.palemoon.org/moonchild-central/source/platform/toolkit/components/passwordmgr/nsLoginManagerPrompter.js#35

I’m half-wondering if this function needs observers at all without the async stuff, and whether the vast majority of the additional spaghetti code can just be scrapped. Like mostly this stuff with asyncPrompts and hashkeys:

http://xref.palemoon.org/moonchild-central/source/platform/toolkit/components/passwordmgr/nsLoginManagerPrompter.js#84

This code messes with the exact way asyncPrompts are implemented, and I can’t see an equivalent to that function for the simpler original function. It all seems to be built on top of the async junk. The original function doesn’t seem to have anything that deals with the hashkey like the newer one does, which is the main thing giving me pause. It’s also possible this code just manages queues and pumps and observers that aren’t needed at all for synchronous operation.

Well, the C++ part seems to be fairly straightforward so far. https://bugzilla.mozilla.org/show_bug.cgi?id=475053 The async-related parts of the final patch on here are still basically possible to undo in the C++ section of the code, after accounting for the fact that nsHttpChannel was moved to a different directory and most of the logic was split out into nsHttpChannelAuthProvider. The thing that's giving me trouble is the nsLoginManagerPrompter part of this. There's a lot of logic in that part that I don't quite know what to do with, because there's a password manager/master key functionality that was built on top of the async code. The code they added for that feature seems kind of bloated and difficult to follow, and the original function was so simple that I'm not sure it can do everything required here. It's possible I'm just overthinking all this, but I don't want to accidentally remove or break a feature that's tangled up with the async code. The original bug added an observer to the LoginManagerPromptFactory function, which originally had no observers. There have been two additional observers added on top of that since, both related to passwordmgr-crypto-login. http://xref.palemoon.org/moonchild-central/source/platform/toolkit/components/passwordmgr/nsLoginManagerPrompter.js#35 I'm half-wondering if this function needs observers at all without the async stuff, and whether the vast majority of the additional spaghetti code can just be scrapped. Like mostly this stuff with asyncPrompts and hashkeys: http://xref.palemoon.org/moonchild-central/source/platform/toolkit/components/passwordmgr/nsLoginManagerPrompter.js#84 This code messes with the exact way asyncPrompts are implemented, and I can't see an equivalent to that function for the simpler original function. It all seems to be built on top of the async junk. The original function doesn't seem to have anything that deals with the hashkey like the newer one does, which is the main thing giving me pause. It's also possible this code just manages queues and pumps and observers that aren't needed at all for synchronous operation.
Moonchild commented 3 months ago
Owner

I think you’re overthinking this and not keeping to the scope of this particular prompt.
nsLoginManagerPrompter is used by wbesite logins too and that functionality works as it should (and needs to be async for the surrounding plumbing), but the http basic auth prompt isn’t in the same kind of environment.
Also, reaching all the way back to that old bug and trying to work around the many code changes in the meantime is most likely going to open up a can of worms you shouldn’t, and with extremely high risk of introducing more bugs.

As for the observers; many sync functions don’t need any of that because nothing gets deferred that needs to be observed for “when it’s done”. if you have a straight-forward sequential piece of code, then all you need is to fire the prompt to get the data, and then use that data. the sync prompter I’m sure has all the hooks necessary to pull details from the password manager as well (since it worked that way before this async stuff was introduced)...

How about you don’t try to trace history here and simply take the code I indicated and use the fallback sync call as the first chance call? using the authPrompt->PromptAuth() call instead of authPrompt->AsyncPromptAuth and setting the values with holder->SetToHttpAuthIdentity() as done in the fallback code block?

I think you're overthinking this and not keeping to the scope of this particular prompt. `nsLoginManagerPrompter` is used by wbesite logins too and that functionality works as it should (and needs to be async for the surrounding plumbing), but the http basic auth prompt isn't in the same kind of environment. Also, reaching all the way back to that old bug and trying to work around the many code changes in the meantime is most likely going to open up a can of worms you shouldn't, and with extremely high risk of introducing more bugs. As for the observers; many sync functions don't need any of that because nothing gets deferred that needs to be observed for "when it's done". if you have a straight-forward sequential piece of code, then all you need is to fire the prompt to get the data, and then use that data. the sync prompter I'm sure has all the hooks necessary to pull details from the password manager as well (since it worked that way before this async stuff was introduced)... How about you don't try to trace history here and simply take the code I indicated and use the fallback sync call as the first chance call? using the `authPrompt->PromptAuth()` call instead of `authPrompt->AsyncPromptAuth` and setting the values with `holder->SetToHttpAuthIdentity()` as done in the fallback code block?
athenian200 commented 3 months ago
Collaborator

Ah, that was actually the first thing I did. I basically just removed the async call and made the fallback call the default one, which was both obvious from the code and backed up by looking at the bug. That fallback code you’re talking about is the original code, they just shoved it into an else block when they added the async call.

It seems like I misunderstood goal here and thought I needed to remove all of that stuff to get the desired result for some reason, when removing the easiest part to remove was sufficient.

Ah, that was actually the first thing I did. I basically just removed the async call and made the fallback call the default one, which was both obvious from the code and backed up by looking at the bug. That fallback code you're talking about is the original code, they just shoved it into an else block when they added the async call. It seems like I misunderstood goal here and thought I needed to remove all of that stuff to get the desired result for some reason, when removing the easiest part to remove was sufficient.
Moonchild removed the
Good first issue
label 3 months ago
Moonchild commented 3 months ago
Owner

I think what we should do is keep an eye on this bug and see what Mozilla comes up with for their own usage. It’s more exposed to them because they moved to tab-modal prompts for http basic auth which makes occurrence of multiple auth prompts at the same time much more likely of a scenario.

I think what we should do is keep an eye on [this bug](https://bugzilla.mozilla.org/show_bug.cgi?id=1684469) and see what Mozilla comes up with for their own usage. It's more exposed to them because they moved to tab-modal prompts for http basic auth which makes occurrence of multiple auth prompts at the same time much more likely of a scenario.
athenian200 commented 3 months ago
Collaborator

Yeah, I agree. I’ve been looking into the issue more, and it turns out the async isn’t the problem at all. At the same time async was implemented, they added a queue that deliberately presents only one prompt at a time. So the async behavior itself isn’t what is causing this, they added an entire queuing architecture while they were in there that just happens to rely on the async functionality.

Earlier versions of the original patch were actually window modal, and there was a huge debate about whether the prompt should be window-modal or app-modal (the app side won at that time), and then much, much later someone decided it should be tab-modal on the Mozilla side. In other words, there’s no technical reason having async prompts should prevent us from having more than one functional dialog box on screen. It’s more that they decided that a window-modal version of the patch would make the code too complicated to review.

I think removing the async prompting code just happens to half-break the queuing mechanism that was built on top of it at the same time, which is why this worked at all. So I’m guessing that what Mozilla will do is remove that mess of a queuing architecture somehow, but still ultimately have async prompts.

I mistakenly assumed when I started on this bug that the async was only added to support that queuing architecture, and intended to remove the whole mess... the async, the queuing, everything. Turns out that what is really needed is probably to leave the async alone and remove that queue they implemented somehow.

Yeah, I agree. I've been looking into the issue more, and it turns out the async isn't the problem at all. At the same time async was implemented, they added a queue that *deliberately* presents only one prompt at a time. So the async behavior itself isn't what is causing this, they added an entire queuing architecture while they were in there that just happens to rely on the async functionality. Earlier versions of the original patch were actually window modal, and there was a huge debate about whether the prompt should be window-modal or app-modal (the app side won at that time), and then much, much later someone decided it should be tab-modal on the Mozilla side. In other words, there's no technical reason having async prompts should prevent us from having more than one functional dialog box on screen. It's more that they decided that a window-modal version of the patch would make the code too complicated to review. I think removing the async prompting code just happens to half-break the queuing mechanism that was built on top of it at the same time, which is why this worked at all. So I'm guessing that what Mozilla will do is remove that mess of a queuing architecture somehow, but still ultimately have async prompts. I mistakenly assumed when I started on this bug that the async was only added to support that queuing architecture, and intended to remove the whole mess... the async, the queuing, everything. Turns out that what is really needed is probably to leave the async alone and remove that queue they implemented somehow.
Moonchild commented 3 months ago
Owner

what is really needed is probably to leave the async alone and remove that queue they implemented somehow.

Agreed with this.

> what is really needed is probably to leave the async alone and remove that queue they implemented somehow. Agreed with this.
Moonchild commented 3 months ago
Owner

The referenced BZ bug now has preliminary patches attached that remove the queuing code which can be used to resolve this. @athenian200 you should be able to move forward with this adopting that code to our use. be careful since it seems other changes they are making regress window-modal prompting which they are setting up to remove on their end so won’t be fixing (probably the whole “make fully async” part). Pick those cherries carefully and skip the rotten ones ;)

The referenced BZ bug now has preliminary patches attached that remove the queuing code which can be used to resolve this. @athenian200 you should be able to move forward with this adopting that code to our use. be careful since it seems other changes they are making regress window-modal prompting which they are setting up to remove on their end so won't be fixing (probably the whole "make fully async" part). Pick those cherries carefully and skip the rotten ones ;)
Moonchild added this to the (deleted) milestone 3 months ago
athenian200 commented 3 months ago
Collaborator

I can’t seem to see those patches. I saw this comment: https://bugzilla.mozilla.org/show_bug.cgi?id=1684469#c12

He says he has a draft patch, but there doesn’t seem to be one attached anywhere that I noticed. There seem to be no files listed under “Attachments.” It looks like they might have removed the patches before I had a chance to snag them.

I can't seem to see those patches. I saw this comment: https://bugzilla.mozilla.org/show_bug.cgi?id=1684469#c12 He says he has a draft patch, but there doesn't seem to be one attached anywhere that I noticed. There seem to be no files listed under "Attachments." It looks like they might have removed the patches before I had a chance to snag them.
Moonchild commented 3 months ago
Owner

Oh they didn’t actually attach the patch draft yet? I could have sworn... but eh never matter I’m sure it’ll be posted one of these days™

Oh they didn't actually attach the patch draft yet? I could have sworn... but eh never matter I'm sure it'll be posted one of these days™
Moonchild modified the milestone from 29.0.0 to 29.0.0 3 months ago
Moonchild modified the milestone from 29.0.0 to 29.1.0 2 months ago
Moonchild commented 2 months ago
Owner

It does look like Mozilla landed this now, so maybe you could have another look?

It does look like Mozilla landed this now, so maybe you could have another look?
athenian200 commented 2 months ago
Collaborator

Looking at it now. Hopefully the relevant parts are easy to backport. It’s looking good so far, but I’ll try to report on where I am with this by the end of the day.

Looking at it now. Hopefully the relevant parts are easy to backport. It's looking good so far, but I'll try to report on where I am with this by the end of the day.
Moonchild commented 1 month ago
Owner

Since I heard nothing further i’ll move the target version to 29.2 for this.

Since I heard nothing further i'll move the target version to 29.2 for this.
Moonchild modified the milestone from 29.1.0 to 29.2.0 1 month ago
athenian200 commented 1 month ago
Collaborator

Yeah, I mistakenly thought I wrote a follow-up already and forgot about this. I probably typed it up but never submitted it.

My impression is that the part of the Mozilla bug that removes the queue can be adapted to our codebase, but what Mozilla has is different enough from what we have that it seems like it would be very involved and require a serious understanding of JavaScript.

They remove huge blocks of code and replace them with new blocks in ways that I can’t relate back to what we have easily because there were a couple refactors in the Mozilla code, plus it also looks like a few things were moved around or added to our version in this specific area since the fork point.

If this were C++ or even Python I could probably follow it a lot better. Since the relevant changes are all in fairly involved JavaScript, I’m way out of my depth here and probably can’t even adapt these changes to what we have, nor actually be sure how or whether they’d have to be tweaked to deal our notifications not being doorhangers, etc. GUI interfaces in general are also another area that’s outside my experience.

I’m disappointed that I don’t have anything more to offer after a week, but I probably wouldn’t have considered taking this on if I’d known what it was going to involve in the end.

Yeah, I mistakenly thought I wrote a follow-up already and forgot about this. I probably typed it up but never submitted it. My impression is that the part of the Mozilla bug that removes the queue can be adapted to our codebase, but what Mozilla has is different enough from what we have that it seems like it would be very involved and require a serious understanding of JavaScript. They remove huge blocks of code and replace them with new blocks in ways that I can't relate back to what we have easily because there were a couple refactors in the Mozilla code, plus it also looks like a few things were moved around or added to our version in this specific area since the fork point. If this were C++ or even Python I could probably follow it a lot better. Since the relevant changes are all in fairly involved JavaScript, I'm way out of my depth here and probably can't even adapt these changes to what we have, nor actually be sure how or whether they'd have to be tweaked to deal our notifications not being doorhangers, etc. GUI interfaces in general are also another area that's outside my experience. I'm disappointed that I don't have anything more to offer after a week, but I probably wouldn't have considered taking this on if I'd known what it was going to involve in the end.
Moonchild commented 1 month ago
Owner

My impression is that the part of the Mozilla bug that removes the queue can be adapted to our codebase, but what Mozilla has is different enough from what we have that it seems like it would be very involved and require a serious understanding of JavaScript.

Yeah I didn’t have time to look at it in depth myself yet. Been working through other stuff.

I’m disappointed that I don’t have anything more to offer after a week, but I probably wouldn’t have considered taking this on if I’d known what it was going to involve in the end.

Not a problem! Sometimes you don’t really know what you’re looking at until you’re actually trying to work with it. I’m misjudged the complexity of Moz bugs more than once myself as well so don’t feel bad.

I’ll just put it on the backlog for someone else to look at.

> My impression is that the part of the Mozilla bug that removes the queue can be adapted to our codebase, but what Mozilla has is different enough from what we have that it seems like it would be very involved and require a serious understanding of JavaScript. Yeah I didn't have time to look at it in depth myself yet. Been working through other stuff. > I’m disappointed that I don’t have anything more to offer after a week, but I probably wouldn’t have considered taking this on if I’d known what it was going to involve in the end. Not a problem! Sometimes you don't really know what you're looking at until you're actually trying to work with it. I'm misjudged the complexity of Moz bugs more than once myself as well so don't feel bad. I'll just put it on the backlog for someone else to look at.
athenian200 was unassigned by Moonchild 1 month ago
Moonchild removed this from the 29.2.0 milestone 1 month ago
Moonchild commented 1 month ago
Owner

That is, unless you think you can remove the queueing as-is because i think tat would likely solve it for us (since we can only have one modal prompt per window anyway). And we won’t need to deal with tab-modal auth.

That is, unless you think you can remove the queueing as-is because i think tat would likely solve it for us (since we can only have one modal prompt per window anyway). And we won't need to deal with tab-modal auth.
Sign in to join this conversation.
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.