90060 - crash when hitting back button twice after printing

Status: VERIFIED FIXED

(Core :: Printing: Output, defect)

Description

[Tom Everingham]

Overview Description:  If you hit the back button quickly after printing the 
browser crashes.

Steps to Reproduce:
1.)  Go to the URL. 
2.)  Select a link to display a particular song.  The 'PL' links usually work.  
3.)  select file | print | ok to send it to printer
4.)  quickly press the back button 

Actual Results:  crash

Build Date & Platform Bug Found:  branch 2001-07-09-05-0.9.2 WinNT 4 

Additional Builds and Platforms Tested On:  works on linux and mac 

Additional Information:  I think this is happening at any site if you press back 
quickly after printing but its easier to reproduce with the short text files in 
the supplied link.

talkback report 32705608:

0x0455bd70 
nsAppShellService::Run            
[d:\builds\seamonkey\mozilla\xpfe\appshell\src\nsAppShellService.cpp, line 419]
netscp6.exe + 0x16f0 (0x004016f0)
netscp6.exe + 0x11b8 (0x004011b8) 
netscp6.exe + 0x3243 (0x00403243) 
KERNEL32.dll + 0x1ba3c (0x77f1ba3c)

Comment 1

[sujay]

I cannot reproduce this. Tom, can you show me in my cube?

Comment 2

[Amarendra Hanumanula]

   Sunjay, I can reproduce it on win2000 Build # 2001070904
steps to reproduce: 
1)if you print and hit back button couple of times it crashes..
 

Comment 3

[sujay]

okay I can reproduce this now....

1) launch netscape
2) go to any web page, surf a couple pages
3) hit print
4) immediately hit Back button twice

crash.

output makes it out fine though..

also you can get this to happen for any URL

Comment 4

[sujay]

updating summary...adding "twice". that does the trick.

Comment 5

[Tom Everingham]

this happens pretty consistantly for me but you have to hit the back button
quickly, cc'ing radha for help determining where this belongs

Comment 6

[Radha on family leave (not reading bugmail)]

This is the stack trace I have in today's debug build, of the branch. 

nsTimer::~nsTimer() line 122 + 24 bytes
nsTimer::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsTimer::Release(nsTimer * const 0x058bd4b0) line 94 + 151 bytes
nsTimerManager::FireNextReadyTimer(nsTimerManager * const 0x0343ac90, unsigned
int 0) line 117 + 12 bytes
nsAppShell::Run(nsAppShell * const 0x0389b280) line 118
nsAppShellService::Run(nsAppShellService * const 0x0389daa0) line 425
main1(int 1, char * * 0x00485d70, nsISupports * 0x00000000) line 1227 + 32 bytes
main(int 1, char * * 0x00485d70) line 1531 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e97d08()

Basically, mCallback is a illegal pointer in nsTimer::~nsTimer(, when the crash
happens.

Comment 7

[Tom Everingham]

Hey Rick, does any of this mean anything to you?

Initially, when I was hitting back while printing my printouts were being 
interupted and I wouldn't get all the pages.  Then it started crashing.  

Comment 8

[rpotts (gone)]

Oh God...  the horrors have begun :-)

I bet that all hell beaks loose when the nsDocumentViewer associated with the 
page being printed is destroyed while the page is printing...

the printing code shares the same document but creates a new PresShell...  So, i 
wouldn't be surprise if something is getting deleted (or prematurely removed) 
while the printing is being done...

at one point i suggested that we probably don't want to delete DocumentViewers 
while their document is being printed...

I would give this one to done cone... cause i think that he's still doing 
printing :-)

-- rick

Comment 9

[dcone (gone)]

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

Comment 10

[dcone (gone)]

Attachment: Patch to refcount the docviewer

Comment 11

[Marc Attinasi]

I think you might need to check for null on the addref like you do on the
release - or just assert if aDocViewerImpl cannot be null. sr=attinasi

Comment 12

[Peter Lubczynski]

r=peterl

Comment 13

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

With the recent changes I made to the document viewer, this approach probably
isn't going to work anymore without additional changes.  As I suggested in
email, you can, with a tri-state member variable on the PrintData struct (?),
defer the destruction until after the timer fires.  The states would be (0) no
timer exists (1) timer exists, but should not destroy doc viewer (2) timer
exists, and should destroy doc viewer.

Comment 14

[dcone (gone)]

I feel very uncomfortable keeping track of the documentViewer this way.. it 
seems to circumvent the reason we refcount.. and I would be very paranoid that I 
would cause a leak.. or destroy the documentViewer before its time.  Besides the 
fact that I really dont understand what your suggesting all the way.  Dont you 
think we should just use the refCounting.. it could be made to work with your 
changes also.

Comment 15

[dcone (gone)]

Attachment: patch to fix

Comment 16

[dcone (gone)]

Attachment: patch with the -u

Comment 17

[dcone (gone)]

This is a patch that refcounts and does not let the "destroy()" kill the 
printing code.  When the refcount goes to 0 after the print is finished.. then 
the the printing objects should be cleaned up.

Comment 18

[sujay]

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

Comment 19

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

But that will leak the timer and the mPrt (and thus everything the mPrt has
strong references to), won't it?

Comment 20

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

Attachment: another possible solution (not tested)

Comment 21

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

Actually, that's wrong, I can't addref in Init and release in the dtor (of the
timer) since those need not balance.

Comment 22

[Marc Attinasi]

I think the timer and the mPrt will have to be cleaned up in the destructor if
they are not cleaned up in the Destroy method...

Comment 23

[dcone (gone)]

The destroy would try to get rid of what it can, if its printing it will not get 
rid of the timer and the mPtr.  When the timer is finished.. and release is 
called, the nsDocumentViewer destructor will get called and clean up the timer 
and the mPtr.  Is that assumption wrong?

Comment 24

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

Attachment: working version of previous patch

Comment 25

[dcone (gone)]

You have this snippet of code there.. where you release.. then addref..

does this do something not obvious (in that case.. a comment would be nice to 
answer questions like this).. or is this just an oversite?

+    NS_IF_RELEASE(mDocViewer);
     mDocViewer       = aDocViewerImpl;
+    NS_ADDREF(mDocViewer);

Comment 26

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

That's a pretty common pattern -- release the old, assign, and addref the new. 
At least, it was common before we started using nsCOMPtrs uniformly.  (I
couldn't use an nsCOMPtr since it's an implementation pointer pointing to a
class that ambigously inherits from nsISupports.  Or at least I was too lazy to
figure out whether all the methods that code needed were on one interface.)

Comment 27

[dcone (gone)]

ok.. let me ask another question.. I dont see the initial addref of the 
mDocViewer by the nsPagePrintTimer.. I understand that everytime we init, 
because it can be inited many times we release..then addref, but shouldnt there 
be an initial addref by the timer that is not balanced this way?

Comment 28

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

The first time Init is called, mDocViewer will be null, so the NS_IF_RELEASE
won't do anything.  (Is Init ever called mupltiple times?  The code makes it
look like it's allowed, but I'm not sure whether it will ever happen.)

Comment 29

[dcone (gone)]

ok.. thanks.. good idea!

Comment 30

[dcone (gone)]

I checked all my tests.. I will check this in.. as soon as I get and sr, I will
r this..
r=dcone

Comment 31

[dcone (gone)]

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

Comment 32

[sujay]

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

Comment 33

[sujay]

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

Comment 34

[David Hyatt]

sr=hyatt

Comment 35

[dcone (gone)]

Added patch, tests seem to work.  Fixed.

Comment 36

[sujay]

looks good on my end...gonna have Tom Everingham try it out on his end.

will mark this VERIFIED-FIXED after that.

Comment 37

[Tom Everingham]

I am still crashing using the exact same steps but now I'm getting a different
talkback trace.  Reproduced 3 times.

I'm copying this trace to this report in case it's related.  If you want a
separate report filed please tell me.   Reopening in the meantime.

XPTC_InvokeByIndex 
[d:\builds\seamonkey\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp,
line 139]
XPCWrappedNative::CallMethod 
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednative.cpp, line 1884]
XPC_WN_CallMethod 
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednativejsops.cpp,
line 1253]
js_Invoke 
[d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 809]
js_Interpret 
[d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 2700]
js_Invoke 
[d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 825]
js_InternalInvoke 
[d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 897]
JS_CallFunctionValue 
[d:\builds\seamonkey\mozilla\js\src\jsapi.c, line 3351]
nsJSContext::CallEventHandler 
[d:\builds\seamonkey\mozilla\dom\src\base\nsJSEnvironment.cpp, line 949]
nsJSEventListener::HandleEvent 
[d:\builds\seamonkey\mozilla\dom\src\events\nsJSEventListener.cpp, line 140]
nsEventListenerManager::HandleEventSubType
[d:\builds\seamonkey\mozilla\content\events\src\nsEventListenerManager.cpp, line
1197]
nsEventListenerManager::HandleEvent 
[d:\builds\seamonkey\mozilla\content\events\src\nsEventListenerManager.cpp, line
2189]
nsXULElement::HandleDOMEvent 
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 3698]
PresShell::HandleDOMEventWithTarget 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5696]
nsMenuFrame::Execute 
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsMenuFrame.cpp, line 1538]
nsMenuFrame::HandleEvent 
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsMenuFrame.cpp, line 439]
PresShell::HandleEventInternal 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5665]
PresShell::HandleEvent 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5575]
nsView::HandleEvent 
[d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 377]
nsView::HandleEvent 
[d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 350]
nsView::HandleEvent 
[d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 350]
nsView::HandleEvent 
[d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 350]
nsViewManager::DispatchEvent 
[d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp, line 2058]
HandleEvent 
[d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 68]
nsWindow::DispatchEvent 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 725]
nsWindow::DispatchWindowEvent 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 742]
nsWindow::DispatchMouseEvent 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 4248]
ChildWindow::DispatchMouseEvent 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 4495]
nsWindow::ProcessMessage 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 3240]
nsWindow::WindowProc 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 990]
USER32.dll + 0x1268 (0x77e71268) 

And once the above trace was preceeded with:

nsCOMPtr_base::assign_assuming_AddRef 
[..\..\..\dist\include\nsCOMPtr.h, line 421]
GlobalWindowImpl::Print 
[d:\builds\seamonkey\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp,
line 139]

Comment 38

[dcone (gone)]

This is not the same bug.. the stack trace is totally different.  Also.. this 
works for me, I did see the problem, after the fix it is not there.  I dont see 
anything in the stack related to printing either.

Comment 39

[Tom Everingham]

ok, I will open a new bug.  Sujay, this is working for me.

Comment 40

[sujay]

verified.