scenario where clicking cancel in the "closing window with <n> tabs open" prompt closes the window, instead of keeing it open this may be related to bug #331457 or bug #327139 steps to reproduce aren't perfect, but I am able to reproduce this. 1) quickly open many tabs at once, using ctrl t 2) hit the close [x] on the window 3) get the confirm to close window with open tabs prompt 4) hit cancel expected results: window does not close actual results: window closes in my console I see: JavaScript error: , line 0: uncaught exception: [Exception... "Component returne d failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowInternal.focus]" nsr esult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://global/c ontent/bindings/tabbrowser.xml :: setFocus :: line 773" data: no] it is coming from the elemet.focus() call in setFocus() in browser.xml function setFocus(element) { document.commandDispatcher.suppressFocusScroll = true; if (element instanceof Window || element instanceof NSHTMLElement || element instanceof XULElement) { element.focus(); } document.commandDispatcher.suppressFocusScroll = false; } gavin, perhaps we should wrap that call to focus with a try / catch in addition to figuring out why it is failing?

> it is coming from the elemet.focus() call in setFocus() in browser.xml I meant tabbrowser.xml, not browser.xml

WARNING: Should not try to set the focus on a disabled window, file c:/builds/bo necho/mozilla/dom/src/base/nsGlobalWindow.cpp, line 3539 from http://lxr.mozilla.org/mozilla1.8/source/dom/src/base/nsGlobalWindow.cpp#3539 3534 nsCOMPtr<nsIBaseWindow> treeOwnerAsWin; 3535 GetTreeOwner(getter_AddRefs(treeOwnerAsWin)); 3536 if (treeOwnerAsWin && (canFocus || isActive)) { 3537 PRBool isEnabled = PR_TRUE; 3538 if (NS_SUCCEEDED(treeOwnerAsWin->GetEnabled(&isEnabled)) && !isEnabled) { 3539 NS_WARNING( "Should not try to set the focus on a disabled window" ); 3540 return NS_ERROR_FAILURE; 3541 } I think that error is bubbling up, throwing an exception, and is causing this (and possible other problems). note, this code comes from jfd's fix for bug #109081 (back in 2002). I'm not sure if this really is a regression, but I'm going to call it one. I think that for starters, we should wrap the call to .focus with a try / catch, and then work backwards to figure out why the element disabled (and if that is OK or not.)

What Seth mentioned in comment 2 is indeed the cause of the bug. For some reason, I can't reproduce this on trunk. It seems to me that could should just return NS_OK to silently fail, not? That's also what is happening somewhere up in the code. http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#3495

> For some reason, I can't reproduce this on trunk. I am on "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060809 BonEcho/2.0b1", my own debug build. > It seems to me that could should just return NS_OK to silently fail, not? that seems reasonable, but it makes me a little nervous until we check what all the callers (firefox, tbird and seamonkey) are expecting. I think we should also figure out which disabled element(s) we are trying to focus. I think we should do something for ff2 here, so asking for blocking.

Blocking due to the dataloss issue.

Comment on attachment 233066 [details] [diff] [review] patch? Moving request over to someone who's not on vacation :)

Comment on attachment 233097 [details] [diff] [review] alternative, low-risk patch for the branch until everything is sorted out I'm still nervous about changing nsGlobalWindow.cpp until we audit the callers. mconnor / bryner, what do you think of my alernative patch?

(In reply to comment #8) > (From update of attachment 233097 [details] [diff] [review] [edit]) > I'm still nervous about changing nsGlobalWindow.cpp until we audit the callers. > > mconnor / bryner, what do you think of my alernative patch? > I agree, especially at this stage of FF2 development. It sounds like what's happening here is that the window is disabled, which happens when a modal dialog is shown, when the setFocus timeout runs. Catching the exception seems reasonable, though it's unfortunate that focus won't actually end up in the right place (well, you could poke at the commandDispatcher, maybe...)

> For some reason, I can't reproduce this on trunk. me neither, but I'd still like to land my patch on trunk and branch, until we decide to make focus() not return failure. > I agree, especially at this stage of FF2 development. thanks brian (and thanks for the review.) I'll land on trunk (and then seek approval for the branch.) I'll log a new bug about removing my try / catch, and/or fixing the nsGlobalWindow.cpp code, per martijn's patch.

Comment on attachment 233066 [details] [diff] [review] patch? obsoleting. I'll move this patch from martijn to the spin off bug.

fixed on trunk. will seek a= for the branch.

> I'll log a new bug about removing my try / catch, and/or fixing the > nsGlobalWindow.cpp code, per martijn's patch. see bug #348357 for martijn's patch and the rest of the issues

Comment on attachment 233097 [details] [diff] [review] alternative, low-risk patch for the branch until everything is sorted out a=drivers, please land on MOZILLA_1_8_BRANCH

fix landed on the branch.

Comment on attachment 233097 [details] [diff] [review] alternative, low-risk patch for the branch until everything is sorted out I think this fix is worth taking for 1.8.0.7. see bug #331457 for another issue that it fixes.

Comment on attachment 233097 [details] [diff] [review] alternative, low-risk patch for the branch until everything is sorted out approved for 1.8.0 branch, a=dveditz for drivers

landed on the 1.8.0 branch