142108 - Crash when reloading XML+XSLT Trunk, M100, N70PR1, M11A [@ nsXMLContentSink::Observe][@ 0x00000001]

Status: VERIFIED FIXED

(Core :: XSLT, defect, P1)

Description

[Jason Johnston]

Reloading an XSL-transformed XML document crashes build 2002050304 on Win2K
(reproduced on 2 machines).  My weblog at http://12.239.72.33/~jj is one example
but I've seen it on other documents as well.

Steps:
1) Load http://12.239.72.33/~jj
2) Press the reload button.
3) Submit talkback report.

Talkback IDs:
  TB5908516X
  TB5908278M
  TB5908064Y
  TB5908030Z
  TB5900665E

Comment 1

[Ashley Bischoff (blog at handcoding.com)]

Confirming 2002050208 on Win2k. With this build, it took about 4 or 5 reloads to
get the crash (but, sure enough, it eventually did crash). Talkback ID for that
crash is TB5908747Y. 

CC: [e-mail] for talkback retrieval, please (TB5908747Y)

Comment 2

[Ashley Bischoff (blog at handcoding.com)]

Err, in comment #1:

s/2002050208/2002050308

Comment 3

[Jason Johnston]

Should've mentioned that these are trunk builds.  Haven't tried the branch
nightlies but RC1 is unaffected.

Comment 4

[[e-mail] (gone - use [e-mail] instead)]

nsXMLContentSink::Observe [nsXMLContentSink.cpp, line 449]
nsObserverService::NotifyObservers [nsObserverService.cpp, line 213]
XSLTProcessor::SignalTransformEnd [XSLTProcessor.cpp, line 2527]
XSLTProcessor::TransformDocument [XSLTProcessor.cpp, line 2472]
nsTransformMediator::TryToTransform [nsTransformMediator.cpp, line 113]
nsTransformMediator::SetTransformObserver [nsTransformMediator.cpp, line 186]
nsXMLContentSink::SetupTransformMediator [nsXMLContentSink.cpp, line 555]
nsXMLContentSink::DidBuildModel [nsXMLContentSink.cpp, line 393]
nsExpatDriver::DidBuildModel [nsExpatDriver.cpp, line 882]
nsParser::DidBuildModel [nsParser.cpp, line 1253]
nsParser::ResumeParse [nsParser.cpp, line 1790]
nsParser::OnStopRequest [nsParser.cpp, line 2419]
nsDocumentOpenInfo::OnStopRequest [nsURILoader.cpp, line 255]
nsHTTPChunkConv::OnStopRequest [nsHTTPChunkConv.cpp, line 125]
nsStreamListenerTee::OnStopRequest [nsStreamListenerTee.cpp, line 25]
nsHttpChannel::OnStopRequest [nsHttpChannel.cpp, line 2909]
nsOnStopRequestEvent::HandleEvent [nsRequestObserverProxy.cpp, line 213]
PL_HandleEvent [plevent.c, line 597]
PL_ProcessPendingEvents [plevent.c, line 530]
_md_EventReceiverProc [plevent.c, line 1078]
nsAppShellService::Run [nsAppShellService.cpp, line 451]
main1 [nsAppRunner.cpp, line 1472]
main [nsAppRunner.cpp, line 1808]
WinMain [nsAppRunner.cpp, line 1826]
WinMainCRTStartup()
KERNEL32.DLL + 0x17d08 (0x77e97d08) 

Comment 5

[Olivier Cahagne]

Related: bug 129006.

Comment 6

[Axel Hecht]

could reproduce this in viewer.
mXSLTransformMediator is null, and I have no idea how that could happen.

Comment 7

[Kamil Toman]

I think this is a platform independent bug. I'm seeing (load/reload) crashes on
Linux and WinXP - mozilla RC1.

There's also another weird thing, adjusting the mime-type in apache
configuration from:
-text/xml
		xml
+text/xml
		xml xsl

prevents crashing on WinXP (Linux build crashed). Mozilla also crashed if the
reported document type was text/plain instead of test/xml which is IMO a Bad
Thing(tm) - it should render it as plain text unless we want to emulate IE.

Comment 8

[Kamil Toman]

Attachment: testing xml document and xslt

Steps to reproduce a crash:
- extract the archive somewhere
- place the xml document (and nothing else) to a standalone directory
- load it to mozilla (file:///... in URL field)
- copy in that directory the xslt component
- hit reload
- reproducible crash (but this seems to be Linux specific way)

Comment 9

[Ashley Bischoff (blog at handcoding.com)]

-> All/All

Kamil: RC1/Linux is affected for you? Here on Win2k (for myself and the original
reporter), only the trunk builds are afffected (RC1 is ok).

Comment 10

[Kamil Toman]

Yes, I tested RC1 candidate on Linux and WinXP. Both crash. However I can only
reproduce it on the Linux machine. WinXP build crashes only on certain apache
servers which send 'text/plain' instead of 'text/xml'. I have no such world-wide
accessible server so I can't pass URL.

Comment 11

[Kamil Toman]

Relevant talkback IDs:
TB6105603H
TB6092658G

Comment 12

[Ashley Bischoff (blog at handcoding.com)]

CC: [e-mail] for talkback retrieval, please (TB6105603H)

Comment 13

[[e-mail] (gone - use [e-mail] instead)]

CNavDTD::DidBuildModel()
nsParser::DidBuildModel()
nsParser::ResumeParse()
nsParser::OnStopRequest()
nsStreamListenerTee::OnStopRequest()
nsHttpChannel::OnStopRequest()
nsOnStopRequestEvent::HandleEvent()
nsARequestObserverEvent::HandlePLEvent()
PL_HandleEvent()
PL_ProcessPendingEvents()
nsEventQueueImpl::ProcessPendingEvents()
event_processor_callback()
our_gdk_io_invoke()

Comment 14

[Corrado Berti]

Seems ok with
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0rc2) Gecko/20020504
(No crash after at least 50 refresh).

as a side note, IE6 doesn't load the page at all
(talkin' about illegal char)

Comment 15

[Jason Johnston]

Corrado, this bug appears only on trunk builds; branch builds appear to be
unaffected.

[If you can figure out what IE6 is complaining about please let me know via
private email.  I'm not really concerned about appeasing IE's shoddy support,
but if it's something _I'm_ doing wrong I definitely want to know about it.]

Comment 16

[Jason Johnston]

Crash still occurring on Win2K trunk build 2002052008

I've also noticed that the crash always seems to follow a rendering with
incompletely applied CSS.  For instance, the page in the url field should show
the "Links" section in the upper right corner, but it will occasionally render
it in unstyled text at the bottom of the page.  Immediately following this
faulty rendering it will crash when reloaded.

Also, I have only been able to reproduce this in Linux once or twice.  The only
difference (besides OS) between my Linux and Win2K installations are that the
Linux box is on the same local network as the server, whereas my Windows box is
at a remote location.  So perhaps network speed plays a role?

Comment 17

[Matthias Versen [:Matti]]

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

Comment 18

[David Gerard]

No, this is showing up on the 1.0 branch. See Bug 146119 (a dupe of this one),
attachment 84595 [details] and attachment 84595 [details] (the xml and its xsl that caused a
reliable crash on 2002051606-1.0 and 2002052106-1.0 . (The document is the
draft 1.0 FAQ.)

We were hoping to use the 1.0 FAQ to showcase XML on the 1.0 start page.
Um ...

Comment 19

[David Gerard]

I meant, of course, attachment 84594 [details] (the XML that attachment 84595 [details] is the
XSL for).

Comment 20

[greer]

Attachment: Platforms, Releases, builds and comments for this crash

Here's a breakdown of where we are seeing these crashes.

Comment 21

[Peter Van der Beken [:peterv]]

Ok, I'll try Windows and Linux and a slow proxy. I haven't been able to
reproduce this. I hit it once.

Comment 22

[David Gerard]

Look at bug 146119 for a setup that helped induce the crash.

velvet.net's content-type misconfiguration has been fixed, but some people have
been getting crashes just loading the document locally (with only faqs.xsl
present, not the other XSLT files that should be there).

Comment 23

[Peter Van der Beken [:peterv]]

I have an idea of what might be causing this and looking at the talkback stacks
seems to confirm it. We're notifying the wrong content sink. I'll supply a patch
for this tomorrow.

Comment 24

[Peter Van der Beken [:peterv]]

Attachment: v1

I think this will fix the crash. Looking at the talkback stacks, we were
notifying the wrong nsXMLContentSink. The way this used to work is that we
registered the sink as an observer for the topic xslt-done. However, we never
removed it as an observer and we never checked if the notification that we got
was aimed at this content sink. I think somehow the content sink from a
previous load stayed around and received a notification that wasn't aimed at it
(and its transform mediator was null because the previous load had finished).
I've dropped the use of the observer service and just moved to calling the sink
directly when the transform is done. There's no need for using the observer
system, there's one sink that has to be notified of the end of the transform it
launched. I haven't hit the crash anymore with this patch, though I never
reproduced it reliably before.
Looking for reviews.

Comment 25

[Axel Hecht]

Comment on attachment 84906 [details] [diff] [review]
v1

patch applies like hell, different base dirs and adding files doesn't go
together well.

>Index: xml/content/src/nsXMLProcessingInstruction.cpp
do you really want this chunk in this patch?

>Index: xml/document/src/nsXMLContentSink.cpp
xml/document/src/Makefile.in needs content_xsl in the REQUIRES, 
makefile.win, too
same for xbl/src/Makefile.in

>RCS file: /cvsroot/mozilla/content/xml/document/src/nsXMLContentSink.cpp,v
>retrieving revision 1.224
>diff -u -3 -r1.224 nsXMLContentSink.cpp
>--- xml/document/src/nsXMLContentSink.cpp	22 May 2002 00:46:13 -0000	1.224
>+++ xml/document/src/nsXMLContentSink.cpp	24 May 2002 10:17:24 -0000
>@@ -402,87 +402,85 @@
>   return NS_OK;
> }
> 
>-// The observe method is called on completion of the transform.  The nsISupports argument is an
>-// nsIContent interface to the root node of the output content model.
> NS_IMETHODIMP
>-nsXMLContentSink::Observe(nsISupports *aSubject, const char *aTopic, const PRUnichar *someData)
>+nsXMLContentSink::OnTransformDone(nsIContent *aRootContent,
>+                                  nsresult aResult,
>+                                  const nsAString & aErrorMessage)
> {
>   nsresult rv = NS_OK;

the rv ain't used no more, just return NS_OK at the end.

>Index: transformiix/source/xslt/XSLTProcessor.cpp

I doubt that aRootContent is really the right thing to flag a failure.
We should use the aResult for this.
I'm not sure if there's a successful transformation not having the
root content of the document set, so that argument might not be needed at
all.
This interface might need to be changed once we do report errors in the
XSLTProcessor, I doubt that we're in the state to fix what we need for
the error report right now.

But this is definitly the way to go. We may want to have a 
OnTransformFailed, once we actually do errors.

Comment 26

[Axel Hecht]

You may want to fix some nits on nsIDocumentTransformer.idl. 
It's NPL :-(, nisheeth should be added to the contributors (as he drops out of
blame) and fix the indention

Comment 27

[Heikki Toivonen (remove -bugzilla when emailing directly)]

All checkins are visible in the log, so blame does not matter that much. I am
not opposing adding anyone to explicit contributors list, though.

Comment 28

[Axel Hecht]

as peterv moves the file, the log is lost. The log isn't really interesting though,
so I don't think we should copy the file in repository.

Comment 29

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Ok, in that case it makes sense. However, if you really want to conserve the log
(change history, blame and all) you could ask [e-mail] to copy/move the
files in the repository itself. That is a bit of a hassle so I only do it with
big changes and/or situations where I really want to retain the change information.

Comment 30

[Mikko Virkkilä]

I can confirm this on Linux with 2002052309 (RC3) and I submitted talkback info.
ID: TB6709662Y

Comment 31

[jn]

is v1 patch applied in nightly builds? It still crashes in 2002052621 talkback
incident: TB6716557G

Comment 32

[Peter Van der Beken [:peterv]]

Attachment: v2

This also fixes a circular ownership problem between the nsXSLContentSink and
the nsTransformMediator. I've cleared up the whole ownership model. The
nsXMLContentSink (and the nsXSLContentSink) owns the transform mediator. The
transform mediator holds a weak reference to the content sink, and passes that
to the XSLT processor, which also holds a weak reference. Since the content
sink owns the mediator and the mediator owns the processor, we're guaranteed
that the content sink held in the weak reference is alive when the mediator or
the processor use it. I've been running with this patch for two days, tried
XSLT transformations, error conditions (invalid source, invalid style),
couldn't get it to crash. If anyone else could test, I'd be grateful.

Comment 33

[Axel Hecht]

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

>Index: content/xml/document/src/nsXMLContentSink.cpp
>@@ -402,88 +401,87 @@
<...>
>+  nsCOMPtr<nsIDocShell> docShell(do_QueryInterface(mWebShell));
>+  nsCOMPtr<nsIContentViewer> contentViewer;
>+  docShell->GetContentViewer(getter_AddRefs(contentViewer));
do we need a null check here?

<...>
>+  // Reset the observer on the transform mediator
>+  mXSLTransformMediator->SetTransformObserver(nsnull);
>+  nsCOMPtr<nsITransformMediator> mediator = mXSLTransformMediator;
is this needed? It ain't used, but you wanted to check

>Index: content/xsl/document/public/nsIDocumentTransformer.idl
<...>
>+    void transformDocument(in nsIDOMNode aSourceDOM,
>+	                       in nsIDOMNode aStyleDOM,
>+						   in nsIDOMDocument aOutputDOC,
>+						   in nsITransformObserver aObserver);						  
Indention

>Index: content/xsl/document/src/makefile.win
>-REQUIRES	= xpcom \
<...>
>+REQUIRES = xpcom \
Indention

>Index: content/xsl/document/src/nsTransformMediator.cpp
>-#include "nsIComponentManager.h"
<...>
>+#include "nsComponentManagerUtils.h"
this file says you should include nsIComponentManager.h

Didn't test this yet, don't have the time to do this today

Comment 34

[Peter Van der Beken [:peterv]]

Attachment: v2.1

Comment 35

[Peter Van der Beken [:peterv]]

Attachment: v2.1 (without crap)

Comment 36

[Axel Hecht]

URL is no longer xml+xslt

Comment 37

[Axel Hecht]

Comment on attachment 86033 [details] [diff] [review]
v2.1 (without crap)

r=[e-mail]

Comment 38

[Jason Johnston]

Sorry for changing the testcase URL out from under you.  I've uploaded the
testcase exactly as it was prior to 11pm last night to
http://www.screaminweb.com/mozillatest/xslt-crash/index.xml
Still produces the crash reliably.

Comment 39

[Peter Van der Beken [:peterv]]

Attachment: v2.2

Changes between this and previous are in nsXMLContentSink.cpp,
nsXSLContentSink.cpp and XSLTProcessor.cpp. This also fixes two memory leaks,
by making sure we remove the nsXSLContentSink as script load observer and
remove nsXMLContentSink as script load observer from the right script loader
(of the original document). Looking for another review.

Comment 40

[Matthias Versen [:Matti]]

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

Comment 41

[greer]

Updating the summary. This one is crashing in large numbers under the 0x00000001
signature in the Talkback reports.

Peter, please re-target the milestone for this bug. Thanks.

Comment 42

[greer]

Doh! Really updating this time.

Comment 43

[Axel Hecht]

Comment on attachment 86506 [details] [diff] [review]
v2.2

r=[e-mail]
Sorry for taking so long, I've been sick and busy.
tested debug on solaris

Comment 44

[Johnny Stenback (:jst)]

Comment on attachment 86506 [details] [diff] [review]
v2.2

From nsITransformMediator.h:

+/**
+ * This interface represents a mediator between raptor and an external

Bah, "raptor"! "Gecko", dude! :-)

sr=jst

Comment 45

[Peter Van der Beken [:peterv]]

Checked in on the trunk. If people could verify that this fixed the problem, it
would help in getting this on the branch

Comment 46

[Jason Johnston]

Verified FIXED on Win2K build 2002062008.  No crash after dozens of reloads.
Need verification on other OSes.

Comment 47

[Heikki Toivonen (remove -bugzilla when emailing directly)]

ADT wants to see some talkback data confirming that this fixes the topcrash
before approving. Since this just landed, we need to wait a little to get the
data. Let's talk to shiva/jpatel tomorrow.

Comment 48

[Jay Patel [:jay]]

I was able to reproduce this crash with Jason's testcase with MozillaTrunk build
2002061908:
Incident ID 7556590
Stack Signature nsXMLContentSink::Observe 61de04a2
Email Address [e-mail]
Product ID MozillaTrunk
Build ID 2002061908
Trigger Time 2002-06-20 18:01:45
Platform Win32
Operating System Windows NT 4.0 build 1381
Module gkcontent.dll
URL visited http://www.screaminweb.com/mozillatest/xslt-crash/index.xml
User Comments Reproducing bug 142108...testcase:
http://www.screaminweb.com/mozillatest/xslt-crash/index.xml - Just loaded the
page and refreshed with F5...boom! Not happening on MozillaTrunk build
2002062008 (meaning this is fixed)
Trigger Reason Access violation
Source File Name
c:/builds/seamonkey/mozilla/content/xml/document/src/nsXMLContentSink.cpp
Trigger Line No. 422
Stack Trace
nsXMLContentSink::Observe
[c:/builds/seamonkey/mozilla/content/xml/document/src/nsXMLContentSink.cpp, line
422]
nsObserverService::NotifyObservers
[c:/builds/seamonkey/mozilla/xpcom/ds/nsObserverService.cpp, line 213]
XSLTProcessor::SignalTransformEnd
[c:/builds/seamonkey/mozilla/extensions/transformiix/source/xslt/XSLTProcessor.cpp,
line 2463]
XSLTProcessor::TransformDocument
[c:/builds/seamonkey/mozilla/extensions/transformiix/source/xslt/XSLTProcessor.cpp,
line 2408]
nsTransformMediator::TryToTransform
[c:/builds/seamonkey/mozilla/content/xsl/document/src/nsTransformMediator.cpp,
line 113]
nsTransformMediator::SetTransformObserver
[c:/builds/seamonkey/mozilla/content/xsl/document/src/nsTransformMediator.cpp,
line 186]
nsXMLContentSink::SetupTransformMediator
[c:/builds/seamonkey/mozilla/content/xml/document/src/nsXMLContentSink.cpp, line
528]
nsXMLContentSink::DidBuildModel
[c:/builds/seamonkey/mozilla/content/xml/document/src/nsXMLContentSink.cpp, line
366]
nsExpatDriver::DidBuildModel
[c:/builds/seamonkey/mozilla/htmlparser/src/nsExpatDriver.cpp, line 939]
nsParser::DidBuildModel
[c:/builds/seamonkey/mozilla/htmlparser/src/nsParser.cpp, line 1254]
nsParser::ResumeParse [c:/builds/seamonkey/mozilla/htmlparser/src/nsParser.cpp,
line 1796]
nsParser::OnStopRequest
[c:/builds/seamonkey/mozilla/htmlparser/src/nsParser.cpp, line 2425]
nsDocumentOpenInfo::OnStopRequest
[c:/builds/seamonkey/mozilla/uriloader/base/nsURILoader.cpp, line 256]
nsHttpChannel::OnStopRequest
[c:/builds/seamonkey/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp, line 2915]
nsOnStopRequestEvent::HandleEvent
[c:/builds/seamonkey/mozilla/netwerk/base/src/nsRequestObserverProxy.cpp, line 213]
PL_HandleEvent [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c, line 597]
PL_ProcessPendingEvents [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c,
line 530]
_md_EventReceiverProc [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c, line
1078] 

But I was NOT able to crash with today's MozillaTrunk build 2002062008 (with
Peter's fix checked in).  This looks ready for the branch.

Comment 49

[Jason Johnston]

Verified fixed on Linux as well, trunk build 2002062011.  Marking as such.

Comment 50

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on adt's behalf) approval for checkin to the 1.0 branch, pending
drivers' approval. pls check this in asap, then add the "fixed1.0.1" keyword.

Comment 51

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I just sent an email to drivers asking for approval.

Comment 52

[Peter Van der Beken [:peterv]]

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

Comment 53

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 54

[S-O Westberg]

I know this comment perhaps do not belong here. But would it be possible to set
build Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.1a+) Gecko/2002062508 as the
default download when you download 1.1a? I have also verifed that that build
fixes problems with the tabels on aftonbladet.se and columns on
http://java.sun.com/j2se/1.4/download.html .

Comment 55

[Axel Hecht]

yes, this is a pretty useless place to put it.
But, so far I can help, 1.1alpha will stay 1.1alpha. But there will be a 1.1beta, 
that's gonna have the bugfixes you're looking for.

Comment 56

[Peter Van der Beken [:peterv]]

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

Comment 57

[Axel Hecht]

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

Comment 58

[rbs]

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

Comment 59

[Axel Hecht]

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