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

VERIFIED FIXED in mozilla1.0.1

Status

()

Core
XSLT
P1
critical
VERIFIED FIXED
16 years ago
15 years ago

People

(Reporter: Jason Johnston, Assigned: peterv)

Tracking

(4 keywords)

Trunk
mozilla1.0.1
crash, regression, testcase, topcrash+
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2 RTM][fixed on trunk 6/20] [ETA 06/24], crash signature, URL)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

16 years ago
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
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: stephend@netscape.com for talkback retrieval, please (TB5908747Y)
Keywords: crash
Err, in comment #1:

s/2002050208/2002050308
(Reporter)

Comment 3

16 years ago
Should've mentioned that these are trunk builds.  Haven't tried the branch
nightlies but RC1 is unaffected.
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

16 years ago
Related: bug 129006.

Comment 6

16 years ago
could reproduce this in viewer.
mXSLTransformMediator is null, and I have no idea how that could happen.
Keywords: regression

Comment 7

16 years ago
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

16 years ago
Created attachment 82911 [details]
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)
-> 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).
OS: Windows 2000 → All
Hardware: PC → All

Comment 10

16 years ago
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

16 years ago
Relevant talkback IDs:
TB6105603H
TB6092658G
CC: stephend@netscape.com for talkback retrieval, please (TB6105603H)

Updated

16 years ago
Depends on: 143265
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

16 years ago
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)
(Reporter)

Comment 15

16 years ago
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.]
(Reporter)

Comment 16

16 years ago
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?
*** Bug 146119 has been marked as a duplicate of this bug. ***

Comment 18

16 years ago
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

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

Updated

16 years ago
Status: NEW → ASSIGNED
Keywords: nsbeta1
Priority: -- → P1
Target Milestone: --- → mozilla1.0

Updated

16 years ago
Keywords: mozilla1.0

Updated

16 years ago
Keywords: testcase, topcrash+
Summary: Crash when reloading XML+XSLT → Crash when reloading XML+XSLT Trunk M1RC3 [@ nsXMLContentSink::Observe]

Comment 20

16 years ago
Created attachment 84661 [details]
Platforms, Releases, builds and comments for this crash

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

Comment 21

16 years ago
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

16 years ago
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).
(Assignee)

Comment 23

16 years ago
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.
(Assignee)

Comment 24

16 years ago
Created attachment 84906 [details] [diff] [review]
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

16 years ago
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

16 years ago
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
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

16 years ago
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.
Ok, in that case it makes sense. However, if you really want to conserve the log
(change history, blame and all) you could ask leaf@mozilla.org 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

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

Comment 31

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

Updated

16 years ago
Blocks: 143047
Whiteboard: [adt2 RTM]
(Assignee)

Comment 32

16 years ago
Created attachment 85579 [details] [diff] [review]
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.
Attachment #84906 - Attachment is obsolete: true

Comment 33

16 years ago
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
(Assignee)

Comment 34

16 years ago
Created attachment 85834 [details] [diff] [review]
v2.1
Attachment #85579 - Attachment is obsolete: true
(Assignee)

Comment 35

16 years ago
Created attachment 86033 [details] [diff] [review]
v2.1 (without crap)
Attachment #85834 - Attachment is obsolete: true

Comment 36

16 years ago
URL is no longer xml+xslt

Comment 37

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

r=axel@pike.org
Attachment #86033 - Flags: review+
(Reporter)

Comment 38

16 years ago
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.
(Assignee)

Comment 39

16 years ago
Created attachment 86506 [details] [diff] [review]
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.
Attachment #86033 - Attachment is obsolete: true
*** Bug 149433 has been marked as a duplicate of this bug. ***

Comment 41

16 years ago
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

16 years ago
Doh! Really updating this time.
Summary: Crash when reloading XML+XSLT Trunk M1RC3 [@ nsXMLContentSink::Observe] → Crash when reloading XML+XSLT Trunk, M100, N70PR1, M11A [@ nsXMLContentSink::Observe][@ 0x00000001]

Comment 43

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

r=axel@pike.org
Sorry for taking so long, I've been sick and busy.
tested debug on solaris
Attachment #86506 - Flags: review+

Updated

16 years ago
Attachment #86506 - Flags: superreview+
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
(Assignee)

Comment 45

16 years ago
Checked in on the trunk. If people could verify that this fixed the problem, it
would help in getting this on the branch
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
No longer depends on: 143265
Resolution: --- → FIXED
(Reporter)

Comment 46

16 years ago
Verified FIXED on Win2K build 2002062008.  No crash after dozens of reloads.
Need verification on other OSes.
Keywords: nsbeta1 → adt1.0.1, nsbeta1+
Whiteboard: [adt2 RTM] → [adt2 RTM][fixed on trunk 6/20]
Target Milestone: mozilla1.0 → mozilla1.0.1
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

16 years ago
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 jpatel@netscape.com
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.
(Reporter)

Comment 49

16 years ago
Verified fixed on Linux as well, trunk build 2002062011.  Marking as such.
Status: RESOLVED → VERIFIED

Updated

16 years ago
Keywords: adt1.0.1, mozilla1.0 → adt1.0.1+, mozilla1.0.1
Whiteboard: [adt2 RTM][fixed on trunk 6/20] → [adt2 RTM][fixed on trunk 6/20] [ETA 06/24]

Comment 50

16 years ago
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.
I just sent an email to drivers asking for approval.
(Assignee)

Comment 52

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

Updated

16 years ago
Attachment #86506 - Flags: approval+

Comment 53

16 years ago
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.
Keywords: mozilla1.0.1 → mozilla1.0.1+

Comment 54

16 years ago
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

16 years ago
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.
(Assignee)

Updated

16 years ago
Keywords: mozilla1.0.1+ → fixed1.0.1
(Assignee)

Comment 56

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

Comment 57

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

Comment 58

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

Comment 59

16 years ago
*** Bug 165138 has been marked as a duplicate of this bug. ***
Crash Signature: [@ nsXMLContentSink::Observe] [@ 0x00000001]
You need to log in before you can comment on or make changes to this bug.