66934 - window.print() from javascript as page loads causes crash

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Mack Earnhardt]

Articles at thestreet.com have a 'print this page' link which points to a
printer-friendly version. The printer friendly page contains javascript which
calls windows.print() immediately.  If you click <ok> on the print dialog,
mozilla crashes.  However, if you click <cancle> you can successfully print
using the print button.  At the time the dialog box appears, neither the page
title nor contents have been rendered.  Possibly trying to print before other
steps are complete?  I'll attach a test case as well.

Comment 1

[Mack Earnhardt]

Attachment: Test Case

Comment 2

[Boris Zbarsky [:bzbarsky]]

Crashes on Linux too (CVS build from 2001-01-28 evening).

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Stack trace from crash

Comment 4

[dcone (gone)]

Rod.. this crashes at the ToNewUnicode() call in nsDocumentViewer.cpp... 

Comment 5

[dcone (gone)]

Fixed

Comment 6

[sujay]

Mack, is this working for you now? please mark VERIFIED-FIXED if so..

Comment 7

[Mack Earnhardt]

I still crash on both the original site and the test case using 20010226 nightly.

Comment 8

[Marek Z. Jeziorek]

setting TFM to mozilla0.9.1 as part of the bug triage

Comment 9

[dcone (gone)]

This does not crash in the printing code.. its crashes after the print job is 
completed.  I will send this back to the Javascript people.

Comment 10

[Phil Schwartau]

Stack trace from Mack's testcase, with a Mozilla debug build 2001-03-01.  
Due to insufficient memory on my Linux box, I was only able to expand 
the top 18 frames of the stack (details will be attached below) 


(gdb) bt
#0  0x8181f5c in ?? ()
#1  0x41c26c27 in PresShell::CaptureHistoryState ()
#2  0x41c99479 in nsCSSFrameConstructor::ConstructDocElementFrame ()
#3  0x41ca67e4 in nsCSSFrameConstructor::ContentInserted ()
#4  0x414234eb in StyleSetImpl::ContentInserted ()
#5  0x41c20cf0 in PresShell::InitialReflow ()
#6  0x4128c9bf in HTMLContentSink::StartLayout ()
#7  0x4128ab92 in HTMLContentSink::OpenBody ()
#8  0x416ab1f4 in CNavDTD::OpenBody ()
#9  0x416ab9cb in CNavDTD::OpenContainer ()
#10 0x416a783f in CNavDTD::HandleDefaultStartToken ()
#11 0x416a8786 in CNavDTD::HandleStartToken ()
#12 0x416a6a0a in CNavDTD::HandleToken ()
#13 0x416a5ff4 in CNavDTD::BuildModel ()
#14 0x416bbdcf in nsParser::BuildModel ()
#15 0x416bbb6a in nsParser::ResumeParse ()
#16 0x416bc8be in nsParser::OnDataAvailable ()
#17 0x40f3f41a in nsDocumentOpenInfo::OnDataAvailable ()
#18 0x40d57145 in nsHTTPFinalListener::OnDataAvailable ()



FWIW, here is the entire stack without any expansion: 



(gdb) bt
#0  0x8181f5c in ?? ()
#1  0x41c26c27 in ?? () from
/data/phil/mozilla/dist/bin/components/libgklayout.so
#2  0x41c99479 in ?? () from
/data/phil/mozilla/dist/bin/components/libgklayout.so
#3  0x41ca67e4 in ?? () from
/data/phil/mozilla/dist/bin/components/libgklayout.so
#4  0x414234eb in ?? () from
/data/phil/mozilla/dist/bin/components/libgkcontent.so
#5  0x41c20cf0 in ?? () from
/data/phil/mozilla/dist/bin/components/libgklayout.so
#6  0x4128c9bf in ?? () from
/data/phil/mozilla/dist/bin/components/libgkcontent.so
#7  0x4128ab92 in ?? () from
/data/phil/mozilla/dist/bin/components/libgkcontent.so
#8  0x416ab1f4 in ?? () from
/data/phil/mozilla/dist/bin/components/libhtmlpars.so
#9  0x416ab9cb in ?? () from
/data/phil/mozilla/dist/bin/components/libhtmlpars.so
#10 0x416a783f in ?? () from
/data/phil/mozilla/dist/bin/components/libhtmlpars.so
#11 0x416a8786 in ?? () from
/data/phil/mozilla/dist/bin/components/libhtmlpars.so
#12 0x416a6a0a in ?? () from
/data/phil/mozilla/dist/bin/components/libhtmlpars.so
#13 0x416a5ff4 in ?? () from
/data/phil/mozilla/dist/bin/components/libhtmlpars.so
#14 0x416bbdcf in ?? () from
/data/phil/mozilla/dist/bin/components/libhtmlpars.so
#15 0x416bbb6a in ?? () from
/data/phil/mozilla/dist/bin/components/libhtmlpars.so
#16 0x416bc8be in ?? () from
/data/phil/mozilla/dist/bin/components/libhtmlpars.so
#17 0x40f3f41a in ?? () from
/data/phil/mozilla/dist/bin/components/liburiloader.so
#18 0x40d57145 in ?? () from /data/phil/mozilla/dist/bin/components/libnecko.so
#19 0x40d2236d in ?? () from /data/phil/mozilla/dist/bin/components/libnecko.so
#20 0x40d18ee4 in ?? () from /data/phil/mozilla/dist/bin/components/libnecko.so
#21 0x40d554c1 in ?? () from /data/phil/mozilla/dist/bin/components/libnecko.so
#22 0x40cf1284 in ?? () from /data/phil/mozilla/dist/bin/components/libnecko.so
#23 0x40cf00c0 in ?? () from /data/phil/mozilla/dist/bin/components/libnecko.so
#24 0x40128a2e in ?? () from /data/phil/mozilla/dist/bin/./libxpcom.so
#25 0x4012884c in ?? () from /data/phil/mozilla/dist/bin/./libxpcom.so
#26 0x4012a779 in ?? () from /data/phil/mozilla/dist/bin/./libxpcom.so
#27 0x407e2d64 in ?? () from
/data/phil/mozilla/dist/bin/components/libwidget_gtk.so
#28 0x407e299f in ?? () from
/data/phil/mozilla/dist/bin/components/libwidget_gtk.so
#29 0x409aa52a in ?? () from /usr/lib/libglib-1.2.so.0
#30 0x409abbe6 in ?? () from /usr/lib/libglib-1.2.so.0
#31 0x409ac1a1 in ?? () from /usr/lib/libglib-1.2.so.0
#32 0x409ac341 in ?? () from /usr/lib/libglib-1.2.so.0
#33 0x408d6209 in ?? () from /usr/lib/libgtk-1.2.so.0
#34 0x407e345a in ?? () from
/data/phil/mozilla/dist/bin/components/libwidget_gtk.so
#35 0x405c51f4 in ?? () from
/data/phil/mozilla/dist/bin/components/libnsappshell.so
#36 0x8056fbb in main1 (argc=1, argv=0xbffffb54, nativeApp=0x0) at
nsAppRunner.cpp:1004
#37 0x8057caa in main (argc=1, argv=0xbffffb54) at nsAppRunner.cpp:1295

Comment 11

[Phil Schwartau]

Attachment: Top 18 frames of Linux stack trace

Comment 12

[Phil Schwartau]

There are no JS Engine issues apparent in the stack trace. 
Reassigning this to Embedding:Docshell for further triage - 

Comment 13

[Adam Lock]

Perhaps the printing code is not waiting for loading to complete before 
proceeding? I imagine all sorts of nasty race conditions could happen if that is 
the case.

Reassigning to printing for further comment.

Comment 14

[dcone (gone)]

Initial relow is called.. and printing will not start until the URL's have been 
loaded, we have addressed those issue for thing with large images, or sites that 
have not been loaded.  This actually prints.. and crashes after the print code 
has executed.

Comment 15

[Adam Lock]

CC Don and Radha because it's definitely a printing/history issue.

This code is a bit confusing, but I will summarize what I think is happening.

Firstly the URL in question contains Javascript that invokes window.print() 
before the page has finished loading, so as soon as content arrives and before 
the initial reflow nsGlobalWindowImpl::Print is being invoked.

The caush of the crash occurs later when DocumentViewerImpl::ReflowPrintObject 
calls nsPresShell::CaptureHistoryState on a preshell (presumably to feed the 
layout history into the print preshell). When ReflowPrintObject() goes out of 
scope, the smart pointer holding the layout state releases its reference and the 
object is destroyed. This is because preshell holds a weak reference to the 
object and does not AddRef its own internal copy (in mHistoryState).

Later during reflow, CaptureHistoryState is called again and the presshell 
thinks mHistoryState is valid. When it attempts to use it the crash occurs.

I imagine that the solution to this bug would be to make mHistoryState into a 
smart pointer. I don't understand from the code why nsPresShell only holds a 
weak pointer when it causes all this refcounting confusion. What would be the 
effect on session history if a presshell were to manage the lifetime of the 
layout history for itself?

Don, Radha can you shed some light?

Comment 16

[Adam Lock]

Attachment: mHistoryState changed to use strong references (more notes to follow)

Comment 17

[Adam Lock]

The previous patch prevents the patch but might have object ownership issues 
with the session history.

Also note that although it fixed the crash, the page didn't print properly 
afterwards, just printing a blank single with the correct URL in the header and 
a footer. This crash is hiding another bug to do with printing a page which is 
still downloading.

Comment 18

[dcone (gone)]

The page should not print until it is fully loaded, there are listeners that 
should prevent such things from happening.  The first reflow is just to notify 
printing when the document has been loaded.  Then once the document is loaded 
printing should complete.  

As far as the history.. is seems the a stong reference is the solution, I am not 
sure why the page is blank though.  There was a bug on that issue about a week 
or two ago, but that was fixed.  

Comment 19

[Adam Lock]

Could it be that the print code is holding onto the wrong presshell in this 
instance, such as the outgoing one? I noticed one time when it printed a blank 
page that it title in the header was from the previous URL, even though the 
header got the right URL.

Comment 20

[dcone (gone)]

It could be.. that the document is being switched out from under the 
printing.  It thinks its printing the old document.. which is loaded.. but then 
that document is somehow changing.. 
Kind of the same situation as printing a bunch of urls one after the other.. 
some document stuff gets messed up.. and can cause a crash.

Comment 21

[Radha on family leave (not reading bugmail)]

The weak ptr for Historystate in PresShell has caused problems in the past. So, 
the  strong reference is a good thing. History will be happy as long as it has a 
valid reference to the state. cc'ing pollmann who owns the 
nsILayoutHistoryState.

Comment 22

[Adam Lock]

Does somebody want to take this then or r/a= my diff?

Note that this bug just fixes the crash and doesn't attempt to make window.print 
during loading work instead of printing blank content.

Comment 23

[Radha on family leave (not reading bugmail)]

I will try your patch and see if it affects any of the form value restoration 
for SH. I think pollmann should review it.

Comment 24

[Radha on family leave (not reading bugmail)]

SH seems to be OK with the attached patch

Comment 25

[Eric Pollmann]

The change seems reasonable r=[e-mail]

I talked with Nisheeth about this (he originally wrote the code) and he
suggested use of an nsWeakPtr instead of an nsCOMPtr.  This should also fix the
crash and would be less likely to cause leaks or unintended state preservation.
 Neither of us could think of anything immediately wrong with using an nsCOMPtr
though, so I'm okay giving an r= for either change with slight preference for
the nsWeakPtr.

Comment 26

[Adam Lock]

Attachment: Patch uses weak references instead of strong

Comment 27

[Adam Lock]

Eric, could you review this latest patch which uses weak references?

Thanks

Comment 28

[Eric Pollmann]

This looks great, r=pollmann.  You did a good thing making the following a bit
more straightforward.  Now you can move the (now repeated) *aState =
historyState, NS_IF_ADDREF(*aState) outside the if / else blocks, and axe the
else block entirely!  :)

...
Index: mozilla/layout/html/base/src/nsPresShell.cpp
...
@@ -4150,20 +4151,23 @@
 
   NS_PRECONDITION(nsnull != aState, "null state pointer");
 
-  if (!mHistoryState) {
+  nsCOMPtr<nsILayoutHistoryState> historyState = do_QueryReferent(mHistoryState);
+  if (!historyState) {
     // Create the document state object
-    rv = NS_NewLayoutHistoryState(aState); // This addrefs
+    rv = NS_NewLayoutHistoryState(getter_AddRefs(historyState)); // This addrefs
   
     if (NS_FAILED(rv)) { 
       *aState = nsnull;
       return rv;
     }    
 
-    mHistoryState = *aState;
+    *aState = historyState;
+    NS_IF_ADDREF(*aState);
+    mHistoryState = getter_AddRefs(NS_GetWeakReference(historyState));
   }
   else {
-    *aState = mHistoryState;
-    NS_IF_ADDREF(mHistoryState);
+    *aState = historyState;
+    NS_IF_ADDREF(*aState);
   }
   
   // Capture frame state for the entire frame hierarchy

Comment 29

[Adam Lock]

Attachment: Thanks Eric for the suggestion. This patch does away with the else statement. I'm submitting this version for sr

Comment 30

[Johnny Stenback (:jst)]

sr=jst

Comment 31

[Adam Lock]

Thanks everyone, fix is checked in.

Don, I've opened bug 80572 to cover the blank page printing issues.