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.
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
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.
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.
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.
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
File a bug for checking the callers. sr=tor
Bug 82719 is open on the callers issue.
a= email@example.com for checkin to 0.9.1
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.