Closed Bug 1350152 Opened 7 years ago Closed 7 years ago

Bug 1266836 introduced toolkit/browser dependency (breaks http auth on XUL apps)

Categories

(Toolkit :: Password Manager, defect, P1)

Product:
Toolkit
Component:
Password Manager
Version:
51 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
thunderbird_esr52 --- fixed
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: mkaply, Assigned: johannh)

References

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1350152 - Don't rely on gBrowser in nsLoginManagerPrompter.js.
59 bytes, text/x-review-board-request
mkaply
: review+
Details 
Bug 1266836 added this code to the login manager:

      let browser = win.gBrowser.getBrowserForContentWindow(aWindow);

https://hg.mozilla.org/mozilla-central/annotate/e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/toolkit/components/passwordmgr/nsLoginManagerPrompter.js#l1423

This depends on a specific behavior of Firefox (window.gBrowser). SeaMonkey worked around it, but other XUL apps have this problem as well. It broke http auth on all my XUL apps when I went to move to Firefox 52.

There should be another way to do this.
I can confirm this bug completely horks http auth in BlueGriffon. There should not be such dependencies from toolkit on browser.
Argh. That bug was pretty complex. I agree that there shouldn't be such a dependency, but I don't think there's a way to do this without access to the tabbrowser right now. I'm happy to take suggestions.

It seems to me like we could add a workaround by re-instating some of the code that was removed here: https://hg.mozilla.org/mozilla-central/diff/6a563348b8be/toolkit/components/passwordmgr/nsLoginManagerPrompter.js#l1.63, e.g. under the condition that there's no gBrowser on those windows.

Matt, do you think that's a good idea?
Flags: needinfo?(MattN+bmo)
Considering toolkit apps probably shouldn't be e10s, I think that should work.

We also need to back port to the esr. People are building stable toolkit apps based on the ESR (because it maintains plugins).
status-firefox-esr52: --- → affected
We'd take a patch for this in 52 ESR, but not in 52.  Are we still supporting XUL apps?
status-firefox52: affected → wontfix
status-firefox53: affected → fix-optional
status-firefox54: affected → fix-optional
(In reply to Mike Kaply [:mkaply] from comment #0)
> SeaMonkey worked around it, but other XUL apps have this problem as well.

Why would SeaMonkey work around a regression instead of having it fixed properly for everyone :(

(In reply to Johann Hofmann [:johannh] from comment #2)
> Matt, do you think that's a good idea?

Yes, bringing back code doing `….chromeEventHandler.ownerDocument.defaultView` as a fallback makes sense.
Flags: needinfo?(MattN+bmo)
Mike, would you be willing to fix this yourself, considering the fix should be pretty straightforward? I don't really have the time to run my own XUL app to test this on right now. Matt and I would be happy to help you out if you have any questions.
Flags: needinfo?(mozilla)
Yep. I'll get patch together.
Flags: needinfo?(mozilla)
FYI, I started looking at this and it got complicated fast. So far all consumers have been able to work around.

It's still on my list, but I'm not sure when it will be done.
Blocks: 1367191
SeaMonkey has a Browser so there wasn't a workaround needed but bug 1347857 which came up later is still open and seems to also caused by this one here.
Blocks: 1347857
Depends on: 1358313
Blocks: 1358313
No longer depends on: 1358313
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Priority: -- → P1
Comment on attachment 8873768 [details]
Bug 1350152 - Don't rely on gBrowser in nsLoginManagerPrompter.js.

Mike, Jorg, Frank-Rainer, can you please give me feedback on whether this patch fixes the problem in your apps? I only tested in Firefox.

Also my solution got a little verbose because I had to assume that not everyone implements chromeWindow.getBrowser() (which frg used in the proposed solution in bug 1367191 to fix SeaMonkey/Thunderbird). Do you think that assumption is correct?
Attachment #8873768 - Flags: feedback?(mozilla)
Attachment #8873768 - Flags: feedback?(jorgk)
Attachment #8873768 - Flags: feedback?(frgrahl)
Comment on attachment 8873768 [details]
Bug 1350152 - Don't rely on gBrowser in nsLoginManagerPrompter.js.

Thanks for the effort, but sadly it doesn't work in TB. No prompt displayed, instead:

JavaScript error: file:///C:/mozilla-source/comm-central/obj-i686-pc-mingw32/dist/bin/components/nsLoginManagerPrompter.js, line 1440: TypeError: tabbrowser.getBrowserForContentWindow is not a function
JavaScript error: , line 0: uncaught exception: 2147500033
JavaScript error: file:///C:/mozilla-source/comm-central/obj-i686-pc-mingw32/dist/bin/components/nsPrompter.js, line 740: TypeError: checkValue is undefined
JavaScript error: file:///C:/mozilla-source/comm-central/obj-i686-pc-mingw32/dist/bin/components/nsLoginManagerPrompter.js, line 1440: TypeError: tabbrowser.getBrowserForContentWindow is not a function
JavaScript error: , line 0: uncaught exception: 2147500033
JavaScript error: file:///C:/mozilla-source/comm-central/obj-i686-pc-mingw32/dist/bin/components/nsPrompter.js, line 740: TypeError: checkValue is undefined

:-(
Attachment #8873768 - Flags: feedback?(jorgk) → feedback-
>> tabbrowser.getBrowserForContentWindow is not a function

That is the problem. Its also the same for SeaMonkey. While SeaMonkeys tabbrowser does have it neither TBs nor SeaMonkeys tabmail.xul defines it. So in Seamonkey it fails only if the Mail window was/is up first. See Magnus post in Bug 1358313 Comment 8
(In reply to Frank-Rainer Grahl (:frg) from comment #14)
> >> tabbrowser.getBrowserForContentWindow is not a function
> 
> That is the problem. Its also the same for SeaMonkey. While SeaMonkeys
> tabbrowser does have it neither TBs nor SeaMonkeys tabmail.xul defines it.
> So in Seamonkey it fails only if the Mail window was/is up first. See Magnus
> post in Bug 1358313 Comment 8

I see, thanks. Does that window actually want to show the doorhanger that prompts the user to save or change their password? If not, I don't think we need to actually set the browser and can skip the call to getBrowserForContentWindow. I'll make a patch.
Attachment #8873768 - Flags: feedback?(jorgk)
Comment on attachment 8873768 [details]
Bug 1350152 - Don't rely on gBrowser in nsLoginManagerPrompter.js.

This works nicely: I added my standard test case picture http://www.jorgk.com/auth/ausflag.png to an e-mail and got the prompt, also with a check box to add to the password manager. I filled in the details, the image got loaded and the credentials were stored in the password manager. Can't ask for more ;-)

Thanks again!
Attachment #8873768 - Flags: feedback?(jorgk) → feedback+
Comment on attachment 8873768 [details]
Bug 1350152 - Don't rely on gBrowser in nsLoginManagerPrompter.js.

So far the latest version also seems to work fine with SeaMonkey. We got another duplicate report in today against this so if it could be reviewed soon I would be glad.
Attachment #8873768 - Flags: feedback+
Attachment #8873768 - Flags: review?(mozilla)
Attachment #8873768 - Flags: review?(dolske)
Attachment #8873768 - Flags: review?(MattN+bmo)
Comment on attachment 8873768 [details]
Bug 1350152 - Don't rely on gBrowser in nsLoginManagerPrompter.js.

https://reviewboard.mozilla.org/r/145182/#review162324

I'm behind on reviews so redirecting this to dolske or mkaply who are also peers.
Justin and Mike, this has been broken by bug 1266836 for about a year now. The review has been sitting here for two months now.

So some action would be much appreciated.
Flags: needinfo?(mozilla)
Flags: needinfo?(dolske)
Comment on attachment 8873768 [details]
Bug 1350152 - Don't rely on gBrowser in nsLoginManagerPrompter.js.

https://reviewboard.mozilla.org/r/145182/#review162596
Attachment #8873768 - Flags: review?(mozilla) → review+
Thanks!
Flags: needinfo?(mozilla)
Flags: needinfo?(dolske)
Johann, thanks for solving this problem, can you please get this landed.
Flags: needinfo?(jhofmann)
Comment on attachment 8873768 [details]
Bug 1350152 - Don't rely on gBrowser in nsLoginManagerPrompter.js.

Oh, wow, progress. Ok. I'm interpreting the OR in

> I'm behind on reviews so redirecting this to dolske or mkaply who are also peers.

as a sign that I can cancel the review request for dolske.

I'll rebase and give this a try run, then we can land.
Flags: needinfo?(jhofmann)
Attachment #8873768 - Flags: review?(dolske)
Great. That's how I understand the two review requests, too.
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/260ffddfa3b8
Don't rely on gBrowser in nsLoginManagerPrompter.js. r=mkaply
https://hg.mozilla.org/mozilla-central/rev/260ffddfa3b8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Do we need to nominate this for Beta/ESR52 approval or can it ride the 57 train?
status-firefox53: fix-optional → wontfix
status-firefox54: fix-optional → wontfix
status-firefox55: affected → wontfix
status-firefox56: --- → affected
Flags: needinfo?(jhofmann)
Version: Trunk → 51 Branch
I don't know, it looks like Thunderbird already resolved it on their end. Mike, do you think this needs to go to Beta/ESR52?
Flags: needinfo?(jhofmann) → needinfo?(mozilla)
(To be clear, this isn't a user-facing Firefox issue).
Blocks: 1388166
(In reply to Johann Hofmann [:johannh] from comment #34)
> I don't know, it looks like Thunderbird already resolved it on their end.
> Mike, do you think this needs to go to Beta/ESR52?

But: SeaMonkey still needs this here to go to Beta/ESR52 - see Bug 1347857.
Unfortunately as indicated, I think seamonkey needs it on ESR.
Flags: needinfo?(mozilla)
Is there something stopping them from creating a relbranch like TB already does?
We're negotiating sharing that branch with SM. BTW, we'd need bug 1388166 as well.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: