248258 - Mozilla 1.7 and Firefox 0.8.0 Crash when I try to open this XSLT stylesheet [@ txMozillaXSLTProcessor::DoTransform ]

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[utcursch]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; X11; Linux i686) Opera 7.21  [en]
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7b) Gecko/20040316

I came across this "XSLT stylesheet to perform a knight's tour of the 
chessboard". It uses MSXML3. I know the page cannot (and should not) be seen 
very nicely in non-IE browsers, but hey! Mozilla **crashes** when I try to 
render this page. The page is rendered nicely in Microsoft Internet Explorer 6.
0. Opera 7.51 doesn't display the page properly, but doesn't crash.

Reproducible: Always
Steps to Reproduce:
1. Open the file mstour.xsl in Mozilla 1.7 or Firefox 0.8.0
Actual Results:  
I tried this with Mozilla 1.7 and Firefox 0.8.0 on Windows xp. I get this error 
(very common on Windows xp): "mozilla.exe has encountered a problem and needs to 
close.  We are sorry for the inconvenience."

I also tried it on Linux and I get the Netscape Quality Feedback Agent prompt.

Expected Results:  
Give some message like "could not parse xsl file" or something similar.

Comment 1

[utcursch]

Attachment: The problem file (XSLT stylesheet)

When I try to open this file in Mozilla 1.7 or Firefox 0.8.0, the browsers
crash.

Comment 2

[Adam Hauner]

WFM Ff0.9/W2K

Reporter, could you provide TalkBack incident ID of your crash?

Comment 3

[Frank Wein [:mcsmurf]]

Comment on attachment 151507 [details]
The problem file (XSLT stylesheet)

sorry for the bugspam, i thought loading a stylesheet from web would work (it
didn't, got "Error loading stylesheet: A network error occured loading an XSLT
stylesheet:http://bugzilla.mozilla.org/ms-tour.xsl" error)...

Comment 4

[Ian Graham]

I did the following:

1) saved the attached XSLT to desktop.
2) loaded the XSLT into Mozilla 1.7 (File .. open) 

Result:  no crash: just a error message (Error loading stylesheet: (null)
error.) in the browser window.

Comment 5

[Jay Patel [:jay]]

I can't seem to reproduce this either...but it would still be nice to have a
Talkback ID to see what this crash looks like.

Comment 6

[Kevin Brosnan]

It is sample code from the XSLT Programmer's Reference, 2nd Edition the file is
avalible from, it is the last item on the list.
http://www.wrox.com/dynamic/books/download.aspx

I was able to crash Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7)
Gecko/20040614 Firefox/0.9

TB id TB165642E and TB165641H

Comment 7

[Adam Hauner]

TB165642E:
0x01cec738
txMozillaXSLTProcessor::DoTransform 
[c:/builds/tinderbox/firefox_branch/WINNT_5.0_Clobber/mozilla/extensions/transformiix/source/xslt/txMozillaXSLTProcessor.cpp,
line 382]
txMozillaXSLTProcessor::setStylesheet 
[c:/builds/tinderbox/firefox_branch/WINNT_5.0_Clobber/mozilla/extensions/transformiix/source/xslt/txMozillaXSLTProcessor.cpp,
line 667]

TB165641H stack looks same.

Comment 8

[Axel Hecht]

This one crashes while deleting an output handler from the execution state 
destructor, no idea what's going on.

#8  0x42aff50c in ~txExecutionState (this=0xbfffbec0)
    at
/u/hecht/Source/moz/aviary/mozilla/extensions/transformiix/source/xslt/txExecutionState.cpp:135
#9  0x42b3ba6d in txMozillaXSLTProcessor::DoTransform() (this=0x87f5df0)
    at
/u/hecht/Source/moz/aviary/mozilla/extensions/transformiix/source/xslt/txMozillaXSLTProcessor.cpp:382
#10 0x42b3ca33 in txMozillaXSLTProcessor::setStylesheet(txStylesheet*)
(this=0x87f5df0,
    aStylesheet=0x89898f8)
    at
/u/hecht/Source/moz/aviary/mozilla/extensions/transformiix/source/xslt/txMozillaXSLTProcessor.cpp:667
#11 0x42b2b230 in txCompileObserver::onDoneCompiling(txStylesheetCompiler*,
unsigned, unsigned short const*, unsigned short const*) (this=0x874cfe0,
aCompiler=0x87f1478, aResult=0, aErrorText=0x0,
    aParam=0x0)
    at
/u/hecht/Source/moz/aviary/mozilla/extensions/transformiix/source/xslt/txMozillaStylesheetCompiler.cpp:487

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Probably some code that doesn't properly 'transfer the ownership' of some output
handler. A minimal testcase would help as I can step through the code since i'm
without environment currently.

Comment 10

[Peter Van der Beken [:peterv]]

I see a failure in PathExpr::evaluate, called from txCopyOf::execute probably
because of the msxml:node-set function call. Might be related (we should be able
to recover from that).

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: Minimal testcase

Comment 12

[Peter Van der Beken [:peterv]]

We start off with mResultHandler and mOutputHandler holding the same
txUnknownHandler. txPushRTFHandler::execute pushes a new txRTFHandler, which
causes mResultHandler to be pushed on the resulthandler stack. txCopyOf::execute
returns an error because of the unknown function. We unroll and call end on the
execution state which calls txUnknownHandler::endDocument on the txUnkownHandler
in mOutputHandler (that is still in the resulthandler stack). When we try to
delete the handlers in the resulthandler stack from the execution state's
destructor we try to delete the same handler again.

Comment 13

[Peter Van der Beken [:peterv]]

I forgot to note that txUnknownHandler::endDocument has |delete this;| as its
last line.

Comment 14

[Peter Van der Beken [:peterv]]

We could call |delete this| only if the mResultHandler in the execution state ==
this. Note that we probably also leak the handler that is currently in
mResultHandler (the one created by txPushRTFHandler::execute).

Comment 15

[Peter Van der Beken [:peterv]]

Attachment: v1

This fixes it. I'm not sure yet if it doesn't break anything else.

Comment 16

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 17

[Peter Van der Beken [:peterv]]

Comment on attachment 160819 [details] [diff] [review]
v1.1

I don't think we need the same fix in startElement, if an error occured earlier
that left the txUnknownHandler in the stack we won't reach startElement.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

yay, this is complicated. Isn't the real problem here that we're calling
endDocument (or possibly just createHandlerAndFlush) in the first place when an
error has occured?

It feels like createHandlerAndFlush will mess up the handlerstack quite a bit
unless it's the only handler on the stack. For the purposes of deleting the
objects it shouldn't matter, but if we happend to do anything else we would be
in deep trouble.


I wonder if the right fix is for txUnknownHandler::endDocument to bail if |this
!= mEs->mResultHandler|.

I'm defenetly not certain though so feel free to disagree.

Comment 19

[Peter Van der Beken [:peterv]]

Attachment: v2

This is a much larger change, but it fixes the crash too *and* makes us show an
error message. I ran buster, no regressions.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 161903 [details] [diff] [review]
v2

>Index: extensions/transformiix/source/xslt/txMozillaTextOutput.cpp
>-void txMozillaTextOutput::endDocument()
>+void txMozillaTextOutput::endDocument(nsresult aResult)
> {
>-    nsCOMPtr<nsITransformObserver> observer = do_QueryReferent(mObserver);
>-    if (observer) {
>-        observer->OnTransformDone(NS_OK, mDocument);
>+    if (NS_SUCCEEDED(aResult)) {
>+        nsCOMPtr<nsITransformObserver> observer = do_QueryReferent(mObserver);
>+        if (observer) {
>+            observer->OnTransformDone(aResult, mDocument);
>+        }
>     }
> }

Looking at nsXMLContentSink::OnTransformDone it looks like we want to call that
function even if something failed. With a proper error of course.

>Index: extensions/transformiix/source/xslt/txMozillaXMLOutput.cpp
>-txTransformNotifier::SignalTransformEnd()
>+txTransformNotifier::SignalTransformEnd(nsresult aResult)
> {
>-    if (mInTransform || mScriptElements.Count() > 0 ||
>-        mStylesheets.Count() > 0) {
>+    if (mInTransform || (NS_SUCCEEDED(aResult) &&
>+        mScriptElements.Count() > 0 || mStylesheets.Count() > 0)) {
>         return;
>     }

Why this change?

>@@ -922,18 +925,26 @@ txTransformNotifier::SignalTransformEnd(
>     // we remove ourselfs from the scriptloader
>     nsCOMPtr<nsIScriptLoaderObserver> kungFuDeathGrip(this);
> 
>-    // XXX Need a better way to determine transform success/failure
>-    if (mDocument) {
>-        nsCOMPtr<nsIDocument> doc = do_QueryInterface(mDocument);
>-        nsIScriptLoader *loader = doc->GetScriptLoader();
>-        if (loader) {
>-            loader->RemoveObserver(this);
>+    nsCOMPtr<nsIDocument> doc = do_QueryInterface(mDocument);
>+    if (doc) {
>+        mStylesheets.Clear();
>+        mScriptElements.Clear();

Should these .Clear()s be moved outside the |if|?

(same function)
>+    if (NS_SUCCEEDED(aResult)) {
>+        mObserver->OnTransformDone(aResult, mDocument);
>     }

Again, should OnTransformDone always be called?

With that fixed or answered r=me

Comment 21

[Peter Van der Beken [:peterv]]

(In reply to comment #20)
> (From update of attachment 161903 [details] [diff] [review])
> Looking at nsXMLContentSink::OnTransformDone it looks like we want to call that
> function even if something failed. With a proper error of course.

txMozillaXSLTProcessor::notifyError will call OnTransformDone on failure.

> >Index: extensions/transformiix/source/xslt/txMozillaXMLOutput.cpp
> >-txTransformNotifier::SignalTransformEnd()
> >+txTransformNotifier::SignalTransformEnd(nsresult aResult)
> > {
> >-    if (mInTransform || mScriptElements.Count() > 0 ||
> >-        mStylesheets.Count() > 0) {
> >+    if (mInTransform || (NS_SUCCEEDED(aResult) &&
> >+        mScriptElements.Count() > 0 || mStylesheets.Count() > 0)) {
> >         return;
> >     }
> 
> Why this change?

I switched to not waiting for stylesheet and script loads if an error occurs.

> >@@ -922,18 +925,26 @@ txTransformNotifier::SignalTransformEnd(

> >+    nsCOMPtr<nsIDocument> doc = do_QueryInterface(mDocument);
> >+    if (doc) {
> >+        mStylesheets.Clear();
> >+        mScriptElements.Clear();
> 
> Should these .Clear()s be moved outside the |if|?

I guess they could.

> (same function)
> >+    if (NS_SUCCEEDED(aResult)) {
> >+        mObserver->OnTransformDone(aResult, mDocument);
> >     }
> 
> Again, should OnTransformDone always be called?

See above.

Comment 22

[Peter Van der Beken [:peterv]]

Attachment: v2

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 170028 [details] [diff] [review]
v2

- In txTransformNotifier::SignalTransformEnd():

+	 if (scriptLoader) {
+	     scriptLoader->RemoveObserver(this);
+	     // XXX Maybe we want to cancel script loads if NS_FAILED(rv)?

s/rv/aResult/

sr=jst