Closed
Bug 82601
Opened 24 years ago
Closed 24 years ago
mozilla will continue trying to create http auth dialogs until the cows come home
Categories
(Core :: DOM: Navigation, defect)
Tracking
()
RESOLVED
FIXED
mozilla0.9.1
People
(Reporter: blizzard, Assigned: blizzard)
Details
(Whiteboard: critical for mozilla 0.9.1)
Attachments
(2 files)
1.55 KB,
patch
|
Details | Diff | Splinter Review | |
2.41 KB,
patch
|
Details | Diff | Splinter Review |
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 1•24 years ago
|
||
Assignee | ||
Comment 2•24 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•24 years ago
|
Status: NEW → ASSIGNED
Comment 3•24 years ago
|
||
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•24 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•24 years ago
|
||
Assignee | ||
Comment 6•24 years ago
|
||
OK, looking for review.
Comment 7•24 years ago
|
||
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.
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•24 years ago
|
||
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•24 years ago
|
||
File a bug for checking the callers. sr=tor
Assignee | ||
Comment 11•24 years ago
|
||
Bug 82719 is open on the callers issue.
Comment 12•24 years ago
|
||
a= asa@mozilla.org for checkin to 0.9.1
Assignee | ||
Comment 13•24 years ago
|
||
Checked in. Thanks, guys.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•