82601 - mozilla will continue trying to create http auth dialogs until the cows come home

Status: RESOLVED FIXED

Description

[Christopher Blizzard (:blizzard)]

If you don't support the creation of new windows at all, like nautilus, mozilla
will continue to try and create them for http auth.  This is because return
values from creating the new windows aren't checked and aren't passed to callers.

Comment 1

[Christopher Blizzard (:blizzard)]

Created attachment 36007 [details] [diff] [review]
patch

Comment 2

[Christopher Blizzard (:blizzard)]

Taking this bug since I need to track it and get it in.  Also marking critical
since I want nautilus users to be able to use this release.

This is a basic patch that checks all of the places where DoDialog() is called
and makes sure that return values are passed back up to the callers.  It also
make sure that DoDialog() will return the call to create the new window.

However, before I check this in I need to make sure that we make a decision
about whether or not to check the param blocks for values or not even if there
is a failure.  Right now some of them do and some of them don't.  Can the owner
of this code make a statement about whether or not they want an early return?  I
have to change the patch in either case for consistency.

Comment 3

[Conrad Carlen (not reading bugmail)]

Unless Dan thinks otherwise, I'd say your patch is good. I don't understand why
DoDialog just swallows the error. Also, the early return seems fine because, on
error, none of the out params from these methods should be touched.

Comment 4

[Christopher Blizzard (:blizzard)]

If they shouldn't be touched then I need to do another patch.  There are some
calls to DoDialog() that still return the return value but read parts from the
param block first.  Patch to follow.

Comment 5

[Christopher Blizzard (:blizzard)]

Created attachment 36020 [details] [diff] [review]
early returns for everyone!

Comment 6

[Christopher Blizzard (:blizzard)]

OK, looking for review.

Comment 7

[Conrad Carlen (not reading bugmail)]

Oh no, I think I gave bad advice about not touching the output values on
failure. Normally, that's what I'd expect: If a method fails, what comes back
from it shouldn't be looked at. On a quick lxr for AlertCheck, I found callers
ignoring the result code. I'd say they were wrong but, on the other hand, we
don't want to break everybody. Let me take a closer look at what to initialize
and what not to. 

Comment 8

[Dan M]

Ugh. We weren't counting on a concerted attempt to disallow UI. I'd say the patch 
is good, but incomplete. (Assuming that WindowWatcher returns a failure code

+  rv = mWatcher->OpenWindow(aParent, aChromeURL, "_blank",
+                            "centerscreen,chrome,modal,titlebar", arguments,
+                            getter_AddRefs(dialog));

and not just a null dialog!) I don't think we can assume that failure to open the 
dialog should be treated the same as hitting "cancel."

"Warning: you're about to do something dangerous. Show this alert again?" I don't 
think we should assume that a single failure to open the alert implies no 
support. The Right Thing is probably to treat failure as if the checkbox were 
checked.

"Save data before closing window?" Again, if you have no UI at all (!?) maybe the 
answer isn't important, but failure to open the alert for any reason should 
probably treat this one as a "yes."

I'm saying that I don't think we can assume the correct answer at the toolkit 
level. Individual users all need to decide their own default values in case of 
error. The toolkit itself should probably simply exit early, leaving the 
parameter block alone.

So like I said before, I think the patch is good, as far as it goes. But it does 
seem like a huge, many-file patch is also called for.

Comment 9

[Conrad Carlen (not reading bugmail)]

In doing an lxr search of callers, turns out that the case I came across first,
in which the result code was ignored but the check value was used, is unusual in
its disregard of result codes. While I haven't checked out every call site
(there's tons), it seems that other callers are doing the right thing.

I second the opinion that at this level we can't assume what values should be
considered default if the dialog fails and we can't get real values. So, the
early returns are right.

r=ccarlen

Comment 10

[tor]

File a bug for checking the callers.  sr=tor

Comment 11

[Christopher Blizzard (:blizzard)]

Bug 82719 is open on the callers issue.

Comment 12

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to 0.9.1

Comment 13

[Christopher Blizzard (:blizzard)]

Checked in.  Thanks, guys.