348183 - scenario where clicking cancel in the "closing window with <n> tabs open" prompt closes the window, instead of keeping it open

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[(not reading, please use [e-mail] instead)]

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?

Comment 1

[(not reading, please use [e-mail] instead)]

> it is coming from the elemet.focus() call in setFocus() in browser.xml

I meant tabbrowser.xml, not browser.xml

Comment 2

[(not reading, please use [e-mail] instead)]

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.)

Comment 3

[Martijn Wargers (dead)]

Attachment: patch?

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

Comment 4

[(not reading, please use [e-mail] instead)]

> 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.

Comment 5

[(not reading, please use [e-mail] instead)]

Attachment: alternative, low-risk patch for the branch until everything is sorted out

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking due to the dataloss issue.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 233066 [details] [diff] [review]
patch?

Moving request over to someone who's not on vacation :)

Comment 8

[(not reading, please use [e-mail] instead)]

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?

Comment 9

[Brian Ryner (not reading)]

(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...)

Comment 10

[(not reading, please use [e-mail] instead)]

> 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 11

[(not reading, please use [e-mail] instead)]

Attachment: trunk version

Comment 12

[(not reading, please use [e-mail] instead)]

Comment on attachment 233066 [details] [diff] [review]
patch?

obsoleting.  I'll move this patch from martijn to the spin off bug.

Comment 13

[(not reading, please use [e-mail] instead)]

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

Comment 14

[(not reading, please use [e-mail] instead)]

> 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 15

[Mike Beltzner [:beltzner, not reading bugmail]]

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

Comment 16

[(not reading, please use [e-mail] instead)]

fix landed on the branch.

Comment 17

[(not reading, please use [e-mail] instead)]

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 18

[Daniel Veditz [:dveditz]]

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

Comment 19

[(not reading, please use [e-mail] instead)]

landed on the 1.8.0 branch