35896 - close browser with print dialog up --> crash

Status: VERIFIED WORKSFORME

(Core :: DOM: Navigation, defect, P1)

Description

[Jesse Ruderman]

Go to file, print, and while the print dialog is still up, close mozilla.  
Mozilla crashes.  Build 2000 041409.

MOZILLA caused an invalid page fault in
module JSDOM.DLL at 015f:60b974ec.
Registers:
EAX=00000000 CS=015f EIP=60b974ec EFLGS=00010246
EBX=0068eca0 SS=0167 ESP=0068ec30 EBP=0068ec70
ECX=0068ec68 DS=0167 ESI=00000000 FS=4bdf
EDX=0068ec68 ES=0167 EDI=01646124 GS=0000
Bytes at CS:EIP:
8b 08 ff 51 1c 56 8d 4d f0 89 75 f0 e8 ab cb ff 
Stack dump:
00000000 0068ec68 00000000 00000000 016d1c58 60bd7378 00000000 00000000 
60bd7378 00000000 00000000 00000000 60cc5549 602dce68 00000000 00000000

Comment 1

[Sean Richardson]

Reproduced with 2000-04-14-09-M16; generated a Talkback blackbox, but
"the Agent is unable to connect to the server" -- so it's still in my queue.
Will provide Incident ID when I have one; mentioned bug 35896 (this one).  

Comment 2

[Sean Richardson]

Talkback Incident ID: TB8689535W

Comment 3

[dcone (gone)]

Print dialog is modal (or should be).. and access to the menu to quit should not 
be allowed.  Handing this To Don Melton.. so he can assign to the correct party.

Comment 4

[Jesse Ruderman]

Can javascript call the print dialog?

Comment 5

[verah (gone)]

Reassigning for Don

Comment 6

[don]

Move to M21 target milestone.

Comment 7

[Blake Ross]

still crashing as of win ME 60208, this wouldn't happen if print dialog was 
modal as it should be

Comment 8

[Blake Ross]

This visible, easily reproducable, reliable crash should really be fixed for 
beta2.

Comment 9

[leger]

Putting on [nsbeta2-] radar.  We can relnote for beta2.

Is this a top Talkback crasher?

Comment 10

[Shiva Thirumazhusai]

Stack Trace Sean reported using Talkback System. 
(Talkback Incident ID: TB8689535W)

Call Stack:    (Signature = GlobalWindowImpl::GetPrivateRoot 3703584a) 
GlobalWindowImpl::GetPrivateRoot[d:\builds\seamonkey\mozilla\dom\src\base\nsGlob
alWindow.cpp, line 2134]
CheckForFocus[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, 
line 1360]
PresShell::InitialReflow[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPres
Shell.cpp, line 1460]
DocumentViewerImpl::Print[d:\builds\seamonkey\mozilla\layout\base\src\nsDocument
Viewer.cpp, line 1319]
nsBrowserInstance::Print[d:\builds\seamonkey\mozilla\xpfe\browser\src\nsBrowserI
nstance.cpp, line 1928]
XPTC_InvokeByIndex[d:\builds\seamonkey\mozilla\xpcom\reflect\xptcall\src\md\win3
2\xptcinvoke.cpp, line 139]
nsXPCWrappedNativeClass::CallWrappedMethod[d:\builds\seamonkey\mozilla\js\src\xp
connect\src\xpcwrappednativeclass.cpp, line 899]
WrappedNative_CallMethod[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwra
ppednativejsops.cpp, line 195]
js_Invoke[d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 687]
js_Interpret[d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 2465]
js_Invoke[d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 703]
js_InternalInvoke[d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 776]
JS_CallFunctionValue[d:\builds\seamonkey\mozilla\js\src\jsapi.c, line 2796]
nsJSContext::CallEventHandler[d:\builds\seamonkey\mozilla\dom\src\base\nsJSEnvir
onment.cpp, line 732]
nsJSEventListener::HandleEvent[d:\builds\seamonkey\mozilla\dom\src\events\nsJSEv
entListener.cpp, line 141]
nsEventListenerManager::HandleEventSubType[d:\builds\seamonkey\mozilla\layout\ev
ents\src\nsEventListenerManager.cpp, line 706]
nsEventListenerManager::HandleEvent[d:\builds\seamonkey\mozilla\layout\events\sr
c\nsEventListenerManager.cpp, line 1481]
nsXULElement::HandleDOMEvent[d:\builds\seamonkey\mozilla\rdf\content\src\nsXULEl
ement.cpp, line 3296]
nsMenuFrame::Execute[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsMenuFrame
.cpp, line 1284]
nsMenuFrame::HandleEvent[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsMenuF
rame.cpp, line 295]
PresShell::HandleEvent[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresSh
ell.cpp, line 3462]
nsView::HandleEvent[d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 811]
nsView::HandleEvent[d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 784]
nsViewManager2::DispatchEvent[d:\builds\seamonkey\mozilla\view\src\nsViewManager
2.cpp, line 1355]
HandleEvent[d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 69]
nsWindow::DispatchEvent[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.
cpp, line 515]
nsWindow::DispatchWindowEvent[d:\builds\seamonkey\mozilla\widget\src\windows\nsW
indow.cpp, line 532]
nsWindow::DispatchMouseEvent[d:\builds\seamonkey\mozilla\widget\src\windows\nsWi
ndow.cpp, line 3238]
ChildWindow::DispatchMouseEvent[d:\builds\seamonkey\mozilla\widget\src\windows\n
sWindow.cpp, line 3443]
nsWindow::ProcessMessage[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow
.cpp, line 2396]
nsWindow::WindowProc[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp
, line 741]
USER32.dll + 0x1250 (0x77e71250) 

Comment 11

[leger]

Though namachi found the Talkback stack trace, this bug was not found to be a 
top Talkback report.

Comment 12

[Sean Richardson]

Bug 41670, "Crash if cookie confirm dialog answered after File>Quit", may or 
may not be related... see its talkback blackbox data.

Comment 13

[verah (gone)]

Nav triage team: [nsbeta3+]

Comment 14

[Daniel Veditz [:dveditz]]

Adding nsbeta3 keyword to bugs which already have nsbeta3 status markings so 
the queries don't get all screwed up.

Comment 15

[Bill Law]

OK, the fix is to make the print dialog modal; there seems to be agreement on 
that.

Making the print dialog is kind of a PITA, though.  We need the parent window at 
the point where the call stack looks like this:

nsDeviceContextSpecFactoryWin::CreateDeviceContextSpec
DocumentViewerImpl::Print
nsBrowserInstance::Print

nsBrowserInstance knows the parent window, so it could pass it to 
DocumentViewerImpl.  But nsIContentViewerFile::Print (the method 
nsBrowserInstance is calling) doesn't currently provide a means of passing it.  
Likewise nsDeviceContextSpecFactoryWin::CreateDeviceContextSpec.

I am not confident changing either of these two interfaces.  One is in the 
general "webshell" category, the other deeper in layout.  So I'm not sure who to  
hand this off to.  I'm going to hand it off to the "webshell" component (once I 
find it); we can deal with the other component when we cross it, so to speak.

Once the interface(s) support passing a parent window, I'll be more than happy 
to tweak nsBrowserInstance to do so.

One possibility is to exploit the nsIProgressListener argument (currently unused 
by nsBrowserInstance) to get the parent window to the next component down the 
line.  That still requires some additional enhancement in order to get it passed 
to nsDeviceContextSpecFactoryWin.

Or, CreateDeviceContextSpec can simply do QueryInterfaces and random method 
calls to get to the parent window.  See, for example, 
nsUrlWidget::SetURLToHiddenControl (in 
http://lxr.mozilla.org/seamonkey/source/xpfe/components/urlwidget/nsUrlWidget.cp
p).

Comment 16

[Adam Lock]

The content viewer already knows it's parent widget from the call to Init(), so 
it's just a matter of getting it to the context. Perhaps there should be a new 
parameter to CreateDeviceContextSpec() that let's the device context spec 
factory call back for additional information?

Comment 17

[verah (gone)]

*** Bug 31439 has been marked as a duplicate of this bug. ***

Comment 18

[Phil Peterson]

PDT thinks this is a P2

Comment 19

[Bill Law]

*** Bug 39053 has been marked as a duplicate of this bug. ***

Comment 20

[Phil Peterson]

Steps to reproduce seem uncommon enough that most users won't encounter this. 
Marking nsbeta3- and adding rtm nomination.

Comment 21

[Frank Tang]

Patch to fix this. Do you want to ask for rtm++ ?
===================================================================
RCS file: /m/pub/mozilla/layout/html/base/src/nsPresShell.cpp,v
retrieving revision 3.341.2.5
diff -u -r3.341.2.5 nsPresShell.cpp
--- nsPresShell.cpp     2000/10/12 14:52:13     3.341.2.5
+++ nsPresShell.cpp     2000/10/17 22:13:36
@@ -1706,6 +1706,9 @@
   nsCOMPtr<nsIDOMWindowInternal> rootWindow;
   nsCOMPtr<nsPIDOMWindow> ourWindow = do_QueryInterface(globalObject);
   ourWindow->GetPrivateRoot(getter_AddRefs(rootWindow));
+  NS_ASSERTION(rootWindow , "cannot get rootWindow");
+  if(nsnull == rootWindow)
+         return;
   nsCOMPtr<nsIDOMDocument> rootDocument;
   rootWindow->GetDocument(getter_AddRefs(rootDocument));

cvs server: Diffing dom/src/base
Index: dom/src/base/nsGlobalWindow.cpp
===================================================================
RCS file: /m/pub/mozilla/dom/src/base/nsGlobalWindow.cpp,v
retrieving revision 1.351.2.4
diff -u -r1.351.2.4 nsGlobalWindow.cpp
--- nsGlobalWindow.cpp  2000/10/15 23:33:19     1.351.2.4
+++ nsGlobalWindow.cpp  2000/10/17 22:13:36
@@ -2561,6 +2561,9 @@

   nsCOMPtr<nsIScriptGlobalObject> parentTop = do_QueryInterface(parent);
   nsCOMPtr<nsIDocShell> docShell;
+  NS_ASSERTION(parentTop, "cannot get parentTop");
+  if(parentTop == nsnull)
+       return NS_ERROR_FAILURE;
   parentTop->GetDocShell(getter_AddRefs(docShell));
   nsCOMPtr<nsIChromeEventHandler> chromeEventHandler;
   docShell->GetChromeEventHandler(getter_AddRefs(chromeEventHandler));

Comment 22

[Blake Ross]

this patch needs to be reviewed (Adam?) and super reviewed ASAP before this can 
be plussed.

Comment 23

[Adam Lock]

Can I have a description of how the patch stops the problem? 

Comment 24

[Frank Tang]

if you follow the reproduce procedure, then you will get parentTop equal to 0x00
after
nsCOMPtr<nsIScriptGlobalObject> parentTop = do_QueryInterface(parent);
If we continue, parentTop->GetDocShell(getter_AddRefs(docShell));

will cause the crash. so... the only thing we can do is to return with error code.

If we return, the caller will call
rootWindow->GetDocument(getter_AddRefs(rootDocument));

so we need to add additional check before it and return.

This is not the RIGHT fix. But it is a fix which will stop the crash. I left the
asserion there so the module owner can still hit the problem code and work out a
REALLY GOOD fix later.
ourWindow->GetPrivateRoot(getter_AddRefs(rootWindow));

Comment 25

[Adam Lock]

r=adamlock.

I think the print mechanism needs a rethink to bring in the concept of modality 
but something to stop the crash is good enough for the moment.

Comment 26

[Frank Tang]

could we take this for rtm ? This is a simple fix.

Comment 27

[selmer (gone)]

please get an sr= (soon!) and set it back to [rtm+] so we can then move it to 
approved.

Comment 28

[vidur (gone)]

Ugh...what Adam said - there's too much code out there that does the wrong thing
with respect to modality because of the inavailability of a suitable parent
window. Frank's change is a reasonable stop-gap for rtm, so sr=vidur. A small
nit - Frank, please make sure that you conform to the 2-space tabbing that's
used in the rest of the file.

Comment 29

[Gervase Markham [:gerv]]

Marking relnote-user in case this doesn't make it in.

Gerv

Comment 30

[selmer (gone)]

rtm++

Comment 31

[Andreas Franke (gone)]

If this hasn't already been checked in, is someone planning to do so? 
Please excuse my spam.

Comment 32

[Phil Peterson]

This fix has missed the first N6 candidate build, so we can not take it unless
we respin. This bug is in candidate limbo.  We will reconsider this fix once we
have a candidate in hand, but we can't take this fix before then. PDT marking [rtm+]

Comment 33

[Phil Peterson]

PDT marking [rtm++]. This bug is now out of limbo and approved for checkin to
the branch. Please check in ASAP.

Comment 34

[Adam Lock]

Fix is now checked into trunk and branch. Marking FIXED

Comment 35

[harishd]

Does it happen only on win98? 

Comment 36

[harishd]

On winnt 2000-10-31-14-MN6 build this still happens. However, in the debug build 
I see a bunch of assertions but no crash... I'm reopening the bug.

Comment 37

[Phil Peterson]

Changing [rtm++] to [rtm need info] due to reopen.

Comment 38

[Peter Trudelle]

I don't see this crash using the 10-31-14 MN6 candidate build on Win98.

Comment 39

[Adam Lock]

I don't see crash either. I'm using the latest branch pull with NT. 

It does print the page however.

Comment 40

[Jesse Ruderman]

Reopening 31439 for the "page prints when you close Mozilla with a print dialog 
up" issue.

Comment 41

[Jim Roskind]

Given this is not being reproduced at this point, I'm pushing it to RTM minus

The RTM train is leaving....

Comment 42

[Adam Lock]

Assigning bug back to Don Cone

Comment 43

[dcone (gone)]

This is an issue for application..the close should not be an option if this 
dialog is up..
Assigning to Don Melton for Triage.

Comment 44

[Dawn Endico]

don is no longer @netscape.com 
reassigning to vishy

Comment 45

[Judson Valeski]

please re-open if this is still reproducible.

Comment 46

[Jesse Ruderman]

Still crashes for me.

Comment 47

[Viswanath Ramachandran]

nominating for nsbeta1. 

Comment 48

[Sean Richardson]

Quoting from above:
> ------- Additional Comments From [e-mail] 2000-08-09 17:30 -------
> OK, the fix is to make the print dialog modal; there seems to be agreement on 
> that.

This bug was not fixed in that fashion; the temporary fix no longer works, 
so this is a regression; adding keyword.

New talkback data: TB29334548M

Comment 49

[Viswanath Ramachandran]

nsbeta1+, P1, => mcafee. 

Comment 50

[Viswanath Ramachandran]

missed actually reassigning to chris last time around ;-)

Comment 51

[Viswanath Ramachandran]

nav triage: not a beta stopper as this is not a common user behaviour. moving to 
mozilla0.9.2. 

Comment 52

[Viswanath Ramachandran]

actually changing the milestone this time :-). 

Comment 53

[Chris McAfee]

this didn't crash for me, w2k, current bits, marking WFM again.
please reopen if this doesn't workforyou.

Comment 54

[Chad Austin]

wfm win2k 2001061905
    win98 2001061905

verified