265790 - "Force links ... to open in a new tab" can let a tab close the whole browser window - FF10PR1 [@ nsFrameManager::GetPropertyListFor]

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Michael Newton]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.3) Gecko/20041022 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.3) Gecko/20041022 Firefox/1.0

Sorry for the awful summary!  The link above has images which, when clicked,
open a new window with a full-size image.  If you are opening new windows in
tabs instead, closing the resulting tab will close the whole browser window
(even if there are other tabs open.)
This appears to be because of a self.close() statement that executes onblur.  A
testcase is attached to the bug.
I marked this as major, because it could cause data loss.

Reproducible: Always
Steps to Reproduce:
1.Set new windows to open in a new tab instead
2.Go to the above URL, click on one of the pictures
3.Close the tab that is created.

Actual Results:  
Firefox closes

Expected Results:  
Just the one tab closes

Comment 1

[Michael Newton]

Attachment: testcase part 2 (popup)

Comment 2

[Michael Newton]

Attachment: testcase part 1 (parent)

Comment 3

[Phil Ringnalda (:philor)]

Certainly seems to be closing quite a bit more than it should be able to. ->NEW,
cc:danm

Comment 4

[Jesse Ruderman]

I get a crash after both tabs close.

Comment 5

[chris hofmann]

Incident ID: 1505180
Stack Signature	nsFrameManager::GetPropertyListFor e28ab00d
Email Address	[e-mail]
Product ID	Firefox10
Build ID	2004102006
Trigger Time	2004-10-24 21:58:19.0
Platform	Win32
Operating System	Windows NT 5.1 build 2600
Module	FIREFOX.EXE + (00233ae1)
URL visited	
User Comments	testing blocker nomination bug
Since Last Crash	346340 sec
Total Uptime	356402 sec
Trigger Reason	Access violation
Source File, Line No.
d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsFrameManager.cpp,
line 1875
Stack Trace 	
nsFrameManager::GetPropertyListFor 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsFrameManager.cpp,
line 1875]
nsFrame::Destroy 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsFrame.cpp,
line 622]
nsSubDocumentFrame::Destroy 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/layout/html/document/src/nsFrameFrame.cpp,
line 567]
nsFrameList::DestroyFrames 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 129]
nsBoxFrame::Destroy 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1065]
nsFrameList::DestroyFrame 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 214]
nsFrameManager::RemoveFrame 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsFrameManager.cpp,
line 757]
nsCSSFrameConstructor::ContentRemoved 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 9528]
PresShell::ContentRemoved 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 5278]
nsGenericElement::doRemoveChild 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/content/base/src/nsGenericElement.cpp,
line 3122]
nsXULElement::RemoveChild 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/content/xul/content/src/nsXULElement.cpp,
line 853]
XPCWrappedNative::CallMethod 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp,
line 2028]
XPC_WN_CallMethod 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,
line 1287]
js_Invoke 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c,
line 941]
js_Interpret 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c,
line 2972]
js_Invoke 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c,
line 958]
js_InternalInvoke 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c,
line 1035]
JS_CallFunctionValue 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/js/src/jsapi.c, line
3698]
nsJSContext::CallEventHandler 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/dom/src/base/nsJSEnvironment.cpp,
line 1297]
nsJSEventListener::HandleEvent 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/dom/src/events/nsJSEventListener.cpp,
line 184]
nsEventListenerManager::HandleEventSubType 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventListenerManager.cpp,
line 1436]
nsEventListenerManager::HandleEvent 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventListenerManager.cpp,
line 1516]
nsXULElement::HandleDOMEvent 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/content/xul/content/src/nsXULElement.cpp,
line 2841]
PresShell::HandleDOMEventWithTarget 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 6139]
nsButtonBoxFrame::MouseClicked 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsButtonBoxFrame.cpp,
line 178]
nsButtonBoxFrame::HandleEvent 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsButtonBoxFrame.cpp,
line 147]
PresShell::HandleEventInternal 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 6103]
PresShell::HandleEventWithTarget 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 5984]
nsEventStateManager::CheckForAndDispatchClick 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventStateManager.cpp,
line 2965]
nsEventStateManager::PostHandleEvent 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventStateManager.cpp,
line 1965]
PresShell::HandleEventInternal 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 6111]
PresShell::HandleEvent 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 5921]
nsViewManager::HandleEvent 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp,
line 2326]
nsViewManager::DispatchEvent 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp,
line 2066]
HandleEvent 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/view/src/nsView.cpp,
line 77]
nsWindow::DispatchEvent 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1067]
nsWindow::DispatchMouseEvent 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 5261]
ChildWindow::DispatchMouseEvent 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 5511]
nsWindow::WindowProc 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1349]
USER32.dll + 0x8709 (0x77d48709)
USER32.dll + 0x87eb (0x77d487eb)
USER32.dll + 0x89a5 (0x77d489a5)
USER32.dll + 0x89e8 (0x77d489e8)
nsAppShell::Run 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsAppShell.cpp,
line 159]
nsAppShellService::Run 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/xpfe/appshell/src/nsAppShellService.cpp,
line 495]
main 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/browser/app/nsBrowserApp.cpp,
line 58]
kernel32.dll + 0x16d4f (0x7c816d4f)

Comment 6

[Dan M]

I see a whole bunch of unexpected window.close()s followed by a crash on WinXP.

By the way this problem can be mostly concealed by backing out the patch for bug
263844. But even then it can be exposed again by setting
dom.allow_scripts_to_close_windows true.

Comment 7

[timeless]

*** This bug has been marked as a duplicate of 232356 ***

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I can't reproduce this crash on Linux/GTK2.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This seems to be Windows-only.  I'm somewhat suspicious of the duplicate
marking, but if someone looking at it in the debugger could be a bit more confident.

Comment 10

[Asa Dotzler [:asa]]

I can't reproduce with the two testcases on windows XP but I can reproduce with
the original crash testing URL. 

Comment 11

[Dan M]

Reopened: see bug 232356 comment 14.

Comment 12

[chris hofmann]

on the radar until we know more

Comment 13

[sairuh (rarely reading bugmail)]

marcia and I cannot crash using the URL to niho.com on mac or linux. (nor am I
able to crash using the reduced test attachments on linux.)

Comment 14

[Brian Ryner (not reading)]

This crash tends to be Windows-specific due to the synchronous nature of event
handling on Windows.  In particular, here's what happens:

- The tab close box is clicked, which causes us to remove the associated xul
content from the browser document.
- We go to destroy the frames for the removed content.
- Destroying the nsSubDocumentFrame calls nsDocumentViewer::Hide() which calls
nsWindow::Show(PR_FALSE).
- This synchronously posts a blur event to the Windows message queue.
- We handle the blur event and run all of the blur event handlers.  As a result,
we call self.close(), which closes the window (as stated in the bug here, we try
to close the toplevel window).  The presshell is destroyed, everything is
deleted, etc.
- We unwind back out of the blur event handling and attempt to finish the call
to nsSubDocumentFrame::Destroy from clicking the tab close box.  We're pretty
much hosed here, since the PresShell and all of the RuleNodes etc appear to be
deleted.

It could be that the deferred window.close() behavior isn't working as designed
here, I'll need to investigate a little more.

Comment 15

[Brian Ryner (not reading)]

The reason we don't hit the deferred window close code is because we hit this 
code instead:

  if (cx) {
    nsIScriptContext *currentCX = nsJSUtils::GetDynamicScriptContext(cx);

    if (currentCX && currentCX == mContext) {
      currentCX->SetTerminationFunction(CloseWindow,
                                        NS_STATIC_CAST(nsIDOMWindow *,
                                                       this));
      return NS_OK;
    }
  }

then on unwinding from the event handler dispatch, we come back to 
nsJSContext::ScriptEvaluated, see that the termination function has been set, 
and call it right away.

I don't really understand this early-exit codepath from globalwindow close... 
hoping jst or brendan do.

In the meantime, I'm backing out bug 263844 for RC1.

Comment 16

[mmortal03]

I get the crash as well.  This is really not cool.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041025
Firefox/1.0 (sweetlou)

Comment 17

[Ben Wilson-Hill]

Also having the same problem. Not using tabs it works perfectly - images pop no
problem. Using tabs - goodbye browser. Actually having the problem with a site I
throw together - www.bechill.net. Used a pre-made script I found some time ago.
Can provide the code if needed. Oh yes XP version and using lastnight's build
(25/10).

Comment 18

[Tracy Walker [:tracy]]

I don't get a crash with any of the test case scenarios given here. As seen on
windows firefox branch build 2004102607

Comment 19

[Josh Powell]

Woah, this needs to be fixed! Thats one nasty bug :P

Comment 20

[Asa Dotzler [:asa]]

Josh Powell, please refrain from commenting in bugs unless your comment provides
information useful for fixing the problem. Thanks.

Comment 21

[Tim Meader]

With the link given, I cannot reproduce this with the Mozilla/5.0 (Windows; U;
Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041026 Firefox/1.0 build.

Anyone else?

I could reproduce it with yesterday's.

Comment 22

[sairuh (rarely reading bugmail)]

this sounds like it's fixed for aviary1.0 --but if there are more fixes pending
for this issue, pls do remove the keyword.

Comment 23

[Dan M]

I'd like to extend the invitation not to dilute the useful posts with "me, too"s
to mmortal03 and Ben Wilson-Hill.

comment 21:
>With the link given, I cannot reproduce this with the Mozilla/5.0 (Windows; U;
>Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041026 Firefox/1.0 build.

For petesake yes the crash was concealed this morning by backing out bug 263844.
Observe the checkin log. This bug is not fixed. But it's not a crash problem for
the upcoming RC1 release because it's been hidden.

comment 15:
>I don't really understand this early-exit codepath from globalwindow close... 
>hoping jst or brendan do.

Brian I don't think the early-exit codepath is the problem. The point of that
code is to delay the window closing until the script has finished executing, if
the script belongs to the window. That's old code that fixed a similar problem a
long time ago, and it seems like a good idea.

The problem seems to me to be that the window-closing callback doesn't know
about tabs. It reaches up and strangles the entire containing XUL window. It
seems to me that the problem lies there. In fact I can make this crash go away if I

* (reinstate bug 263844 to unconceal it)
* make Close not re-enter itself in the middle of executing
* teach ReallyCloseWindow not to kill the entire window if it's
  just a tab we're talking about
* --ahem-- remove the "if (!event.isTrusted)" clause in
  tabbrowser.xml's DOMWindowClose event handler
  (see bug 265921 comment 11)
* unexpectedly, I also need to put a kungFuDeathGrip in
  nsXULTooltipListener::DestroyTooltip

That list item worries me because it implies that something's still not right.
And can I just say that nsXULTooltipListeners seem to be leaking right and left.

Comment 24

[Brian Ryner (not reading)]

(In reply to comment #23)
> The problem seems to me to be that the window-closing callback doesn't know
> about tabs. It reaches up and strangles the entire containing XUL window. It
> seems to me that the problem lies there. In fact I can make this crash go away
if I

There are definitely multiple levels of bugs here.  But it seems like if you had
a script which did legitimately go to close the toplevel window on blur, we
would hit the crash all the same.

Comment 25

[Johnny Stenback (:jst)]

Attachment: Make the DOMWindowClose event (and a few others) fired from native code be trusted.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 163479 [details] [diff] [review]
Make the DOMWindowClose event (and a few others) fired from native code be trusted.

r+sr=bzbarsky

Comment 27

[chris hofmann]

Comment on attachment 163479 [details] [diff] [review]
Make the DOMWindowClose event (and a few others) fired from native code be trusted.

a=chofmann for aviary1.0

Comment 28

[Johnny Stenback (:jst)]

Comment on attachment 163479 [details] [diff] [review]
Make the DOMWindowClose event (and a few others) fired from native code be trusted.

This patch is now checked in on the aviary and 1.7 branches.

Comment 29

[Johnny Stenback (:jst)]

Marking this bug fixed, any remaining issues are being tracked in bug 263844.

Comment 30

[Tracy Walker [:tracy]]

verified with Windows RC1

Comment 31

[Jay Patel [:jay]]

I can also verify that the crash is gone with Mozilla/5.0 (Windows; U; Windows
NT 5.1; en-US; rv:1.7.3) Gecko/20041026 Firefox/1.0RC1.  Also adding topcrash
info for future reference.

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This bug isn't fixed! I can reproduce the crash with a current cvs build:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.5) Gecko/20041112 Firefox/1.0

Incident ID with the same stack trace: TB1911241W

Since this bug was marked as fixed around the 2004-10-26, we received more than
 800 other talkback reports. 

You have to do following steps to reproduce the crash:

1. Open following link into a new window:
http://de.shuttle.com/de/desktopdefault.aspx/tabid-72/170_read-4934/

2. Open manually a new tab which is located on the right side of the first one.

3. Go back to tab one and click on one image which will open a new tab with the
image.

4. Trying to close the new tab crashes Firefox.

Firefox doesn't crash if you don't execute step 2. It seems that this crash only
happens when more then one tab is located between the source and destination tab.

=> Reopening

Comment 33

[Dan M]

Neither the original testcase or website mentioned in this bug report crashes
any longer, so strictly speaking this bug is fixed. A crash bug definitely
remains here, we knew that, and it's being addressed in bug 265962. But this
bug's summary, though very confusing, is a more general match for the symptoms.
So I s'pose I'll leave this one open, too.

I can reproduce Henrik's crash, but only after backing out a private patch I've
been evaluating for bug 265962. (Not the patch currently posted to that bug,
which is no help on my system.) Neither patch made it into 1.0 because of time
constraints. But it seems I already have a fix for both bugs. May as well
reassign...

Patch soon. Still evaluating. Got time, after all: 1.0 has already shipped.

Funny about the 800 talkback reports. This crash can't happen unless the user
has tweaked the advanced pref introduced in bug 266759, can it? But word spread
quickly. The mozillaZine forums are full of recommendations to tweak that pref
without regard to consequences. I guess we should have disabled single window
mode a little more thoroughly in 1.0.

Comment 34

[Dan M]

Re-closing, and moving to bug 265962.