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

RESOLVED FIXED in mozilla0.9.1

Status

()

Core
Document Navigation
--
major
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: blizzard, Assigned: blizzard)

Tracking

Trunk
mozilla0.9.1
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: critical for mozilla 0.9.1)

Attachments

(2 attachments)

(Assignee)

Description

17 years ago
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.
(Assignee)

Comment 2

17 years ago
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.
Assignee: adamlock → blizzard
Severity: normal → major
Whiteboard: critical for mozilla 0.9.1
Target Milestone: --- → mozilla0.9.1
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
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.
(Assignee)

Comment 4

17 years ago
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.
(Assignee)

Comment 5

17 years ago
Created attachment 36020 [details] [diff] [review]
early returns for everyone!
(Assignee)

Comment 6

17 years ago
OK, looking for review.
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

17 years ago
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.
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

17 years ago
File a bug for checking the callers.  sr=tor
(Assignee)

Comment 11

17 years ago
Bug 82719 is open on the callers issue.

Comment 12

17 years ago
a= asa@mozilla.org for checkin to 0.9.1
(Assignee)

Comment 13

17 years ago
Checked in.  Thanks, guys.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.