Closed Bug 129006 Opened 22 years ago Closed 22 years ago

Invalid xml crashes mozilla after reload [@nsXMLContentSink::Observe]

Categories

(Core :: XSLT, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.0

People

(Reporter: jomppa, Assigned: peterv)

References

()

Details

(Keywords: crash, Whiteboard: [ADT2 RTM][fixed on trunk] [ETA 05/15],custrtm-)

Crash Data

Attachments

(11 files, 6 obsolete files)

851 bytes, text/xml
Details
850 bytes, text/xml
Details
2.82 KB, text/xml
Details
2.82 KB, text/xml
Details
2.82 KB, text/xml
Details
2.82 KB, text/xml
Details
17.74 KB, patch
axel
: review+
Details | Diff | Splinter Review
20.28 KB, patch
Details | Diff | Splinter Review
60 bytes, text/css
Details
66 bytes, text/css
Details
1.40 KB, text/xml
Details
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:0.9.8+) Gecko/20020304
BuildID:    2002030403

When loading xml file, which is invalidate, the mozilla crashes to the next
validate xml file.

The invalidate file could be like

<comment name="foo>sometext</comment>
or
<item><comment name="foo">sometext</comment</item>

Reproducible: Always
Steps to Reproduce:
1.Generate an invalidate xml file 
2.Correct the file and press Reload
3.The mozilla crashes

Actual Results:  Crash

Expected Results:  In invalidate file Mozilla should tell user that file
couldn't be loaded (now nothing happends). The correct file should be loaded
without crash.

I think this could be duplicate to bug #123953.
TalkBack ID: TB3653801Q

To crash load invalidate.xml and after that try load validate.xml from the same URL.
Severity: major → critical
Keywords: crash
Confirming after I tested this on local disc. I did not see a crash from the
remote site. Download these files:

http://www.cs.tut.fi/~jomppa/invalidate.xml
http://www.cs.tut.fi/~jomppa/validate.xml
http://www.cs.tut.fi/~jomppa/importinfo.xsl

Then open invalidate.xml, next open validate.xsl and you get a crash here
because mXSLTransformMediator is null:

nsXMLContentSink::Observe(nsXMLContentSink * const 0x041aa9ac, nsISupports *
0x040d1bd8, const char * 0x056add70, const unsigned short * 0x00000000) line 435
+ 38 bytes
nsObserverService::NotifyObservers(nsObserverService * const 0x01226fd0,
nsISupports * 0x040d1bd8, const char * 0x056add70, const unsigned short *
0x00000000) line 213
XSLTProcessor::SignalTransformEnd() line 2495
XSLTProcessor::TransformDocument(XSLTProcessor * const 0x040a32f0, nsIDOMNode *
0x04147d14, nsIDOMNode * 0x0407e1f4, nsIDOMDocument * 0x0413f57c, nsIObserver *
0x040864c4) line 2439
nsTransformMediator::TryToTransform() line 109
nsTransformMediator::SetStyleSheetContentModel(nsTransformMediator * const
0x0414fe40, nsIDOMNode * 0x0407e1f4) line 133
nsXSLContentSink::DidBuildModel(nsXSLContentSink * const 0x0407e7a8, int 0) line 137
nsExpatDriver::DidBuildModel(nsExpatDriver * const 0x03663078, unsigned int 0,
int 1, nsIParser * 0x0407dfe0, nsIContentSink * 0x0407e7a8) line 847 + 23 bytes
nsParser::DidBuildModel(unsigned int 0) line 1388 + 41 bytes
nsParser::ResumeParse(int 1, int 1, int 1) line 1932
nsParser::OnStopRequest(nsParser * const 0x0407dfe4, nsIRequest * 0x041b3098,
nsISupports * 0x00000000, unsigned int 0) line 2542 + 21 bytes
nsFileChannel::OnStopRequest(nsFileChannel * const 0x041b30a0, nsIRequest *
0x041b31ec, nsISupports * 0x00000000, unsigned int 0) line 481 + 41 bytes
nsOnStopRequestEvent::HandleEvent() line 213
nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x041b2ffc) line 116
PL_HandleEvent(PLEvent * 0x041b2ffc) line 590 + 10 bytes

I also noticed that the server gives a wrong mime type for .xsl files and opened
an evangelism bug 129096 for it.
Assignee: heikki → kvisco
Status: UNCONFIRMED → NEW
Component: XML → XSLT
Ever confirmed: true
Keywords: nsbeta1+
QA Contact: petersen → kvisco
Should have been assigned to peterv..
Assignee: kvisco → peterv
Are you sure? my interpretation of the lines assuming bonsai is current
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xml/document/src/nsXMLContentSink.cpp&rev=1.206&mark=427,428,436#416

427 mXSLTransformMediator->GetResultDocument(getter_AddRefs(resultDOMDoc));
// should we also early return here?
428 nsCOMPtr<nsIDocument> resultDoc = do_QueryInterface(resultDOMDoc);
// we should early return here if resultDoc is null
430 nsCOMPtr<nsIDocument> sourceDoc = mDocument;
431 NS_RELEASE(mDocument);
433 mDocument = resultDoc;
434 NS_ADDREF(mDocument);
435 nsCOMPtr<nsIContent> root;
436 mDocument->GetRootContent(getter_AddRefs(root));
// we probably crash here, mDocument == 0
Summary: Invalidate xml crashes mozilla after reload → Invalidate xml crashes mozilla after reload [@nsXMLContentSink::Observe]
Yes, I am sure. I have changes in my tree which is why the line numbers are
different.
timeless: the result document can't be null if this code gets called, the caller
needs it too. If it is null here, we're in serious trouble.
I build a debug version of Mozilla form nightly sources and I also found out 
that the crash occurs because mXSLTransformMediator is null in the valid file.

Heikki, you were absolutely right - I tested it with the local files and then 
copy them to the server. When I tested it afterwards, the mozilla didn't even 
show those files from the server* while IE6.0 did.

* nsXMLContentSink::Observe line 425 if (NS_SUCCEEDED(rv)) drops the execution 
to the else-path and nothing will happend.

When adding line 427:
if (!mXSLTransformMediator) return rv;
the crash didn't happend, but there might appear some other problems.
Attached patch don't transform the error doc (obsolete) — Splinter Review
I'm not sure this is the complete fix, or just part of wallpaper.
Nevertheless, this patch makes us display the error doc, and I guess that's
part of the game. I'm not sure if setting mXSLTransformMediator to 0 is enough,

do we have to talk to the documentViewer here, too?

Anyway, this patch has two good points, we display the parse error, and we
don't
crash.
Comment on attachment 72781 [details] [diff] [review]
don't transform the error doc

r=heikki

One style nit: please replace |0| with |nsnull|.
Attachment #72781 - Flags: review+
Comment on attachment 72781 [details] [diff] [review]
don't transform the error doc

talked with peterv, missing points (at least):
unsetting the mediator in the docviewer
stopping the load of the stylesheet
Attachment #72781 - Flags: needs-work+
Yeah, I realized I should have checked what was set up prior to this point after
I was going home :(

Anyway this is important. Moving to peterv's 1.0 list but if someone else is
working on this you'd better take ownership.
Priority: -- → P2
Target Milestone: --- → mozilla1.0
btw, if the xslt doc contains a parse-error we currently show that error-page 
(which IMHO is great). Though this could be because the parse-error is 
interpreted as an LRE stylesheet.
Status: NEW → ASSIGNED
OS: Windows NT → All
Hardware: PC → All
Summary: Invalidate xml crashes mozilla after reload [@nsXMLContentSink::Observe] → Invalid xml crashes mozilla after reload [@nsXMLContentSink::Observe]
Attached file Valid stylesheet
Attachment #72781 - Attachment is obsolete: true
Attached file Invalid stylesheet
Attached patch v1 (obsolete) — Splinter Review
Should fix the crash, but doesn't stop the stylesheet load for invalid source.
Still looking how to do that.
Does it really matter much if we don't stop loading of the stylesheet? I mean,
yeah, it is a bug but the user probably would never even know...? If this is the
case I would suggest filing a new bug for that and fixing this bug with the
patch you have now.
I agree as long as we don't style the errorous xml or the error-page (or at 
least don't show the result).
applied the patch, and valid source, invalid stylesheet doesn't display anything
for me.
The others are fine though.
Attached patch v2 (obsolete) — Splinter Review
This works for the four testcases (apart from some history bug which prevents
going back after a transform, I'll verify in the trunk if this patch causes it
but I doubt it). If the stylesheet is invalid, we wait until the source is
done: if the source is invalid too we show the error page for the source, if
the source is valid we copy the error message from the stylesheet to the result
document and notify the observer as if we did a transform which will show the
error message we just copied.
Attachment #78556 - Attachment is obsolete: true
Comment on attachment 79734 [details] [diff] [review]
v2

>@@ -465,33 +463,41 @@
> 
>       // Start the layout process
>       StartLayout();
>+      ScrollToRef();

Shouldn't this scroll only for non-history, too? If so, #if 0 like below.
Is there a bug number for this?

>Index: xsl/document/src/nsTransformMediator.cpp
...
>+      if (originalRoot) {
>+        mResultDoc->ReplaceChild(originalRoot, newRoot, getter_AddRefs(root));

ReplaceChild returns the old node, not the new one. Which you want, right?
Attached patch v2.1 (obsolete) — Splinter Review
Looking for reviews.
Attachment #79734 - Attachment is obsolete: true
Attached patch v2.2 (obsolete) — Splinter Review
Attachment #80484 - Attachment is obsolete: true
Attached patch v2.3Splinter Review
Attachment #80702 - Attachment is obsolete: true
Attached patch v2.4Splinter Review
Address jst's "but it can be faster". Only QI to nsIStyleLinkingElement for
html:style and html:link.
But doesn't xml-stylesheet PI also implement nsIStyleSheetLinkingElement, this
code would break PIs, no?
Comment on attachment 80831 [details] [diff] [review]
v2.4

The code that checks the tag name is in element specific code, so no, it won't
affect PI's.

sr=jst
Attachment #80831 - Flags: superreview+
Checked in on the trunk
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Keywords: adt1.0.0
Resolution: --- → FIXED
Whiteboard: [ADT2] → [ADT2][fixed on trunk]
verified on the trunk
Status: RESOLVED → VERIFIED
Sent mail to drivers for checkin to the branch.
Chris - Can you take a look at this one, and make sure it resolves the bug
without any regressions?
Keywords: approval
I checked on this on the OS X trunk build (2002-04-25-08). None of the url's
produce a crash for me. I cc'd Rakesh M to check on Windows and Linux trunk
builds. He will add his comments when finished.
Checked on this with the attachments "Invalid stylesheet" and "Valid stylesheet"
on the trunk build 2002-04-24-12-trunk on  Windows 2000 and Linux trunk build
2002-04-25-10-trunk. None of them produce a crash.
Comment on attachment 80831 [details] [diff] [review]
v2.4

a=asa (on behalf of drivers) for checkin to the 1.0 branch
Attachment #80831 - Flags: approval+
Comment on attachment 80831 [details] [diff] [review]
v2.4

>Index: mozilla/content/base/src/nsDocumentViewer.cpp
>===================================================================
>Index: mozilla/content/xml/document/src/nsXMLContentSink.cpp
>===================================================================
>+#if 0 /* Disable until this works for XML */
>+      //  Scroll to Anchor only if the document was *not* loaded through history means. 
>+      PRUint32 documentLoadType = 0;
>+      docShell->GetLoadType(&documentLoadType);
>+      if (!(documentLoadType & nsIDocShell::LOAD_CMD_HISTORY)) {
>+        ScrollToRef();
>+      }
>+#else
>+      ScrollToRef();
>+#endif

Maybe your source tree is not up to date, I thought these were already checked
in?
Or was this XSLT specific that I did not do earlier?

>Index: mozilla/content/xml/document/src/nsXMLContentSink.h
>===================================================================
>Index: mozilla/content/xsl/document/src/nsITransformMediator.h
>===================================================================
>Index: mozilla/content/xsl/document/src/nsTransformMediator.cpp
>===================================================================
>Index: mozilla/content/xsl/document/src/nsTransformMediator.h
>===================================================================
>   PRBool mEnabled;
>+  PRBool mStyleInvalid;

You could save a little space by changing these to PRPackedBool.

>Index: mozilla/content/xsl/document/src/nsXSLContentSink.cpp
>===================================================================
>+  rv = AddContentAsLeaf(node);
>+  return rv;

You could just 

return AddContentAsLeaf(node);

>Index: mozilla/content/xsl/document/src/nsXSLContentSink.h
>===================================================================

Up to you if you want to make these changes/attach new patch.
Attached file testcase: CSS style tests (obsolete) —
This is meant for regression testing. This testcase should still work after
this patch (testing implied style, XHTML inline style element, XHTML style
attribute, XHTML link stylesheet, xml-stylesheet PI).
Testcase in comment #43 works correctly. Verfied with Trunk Build ID 2002043010.
Let's get this one in after RC2. adt1.0.0- [adt2 RTM]
Keywords: adt1.0.0adt1.0.0-
Whiteboard: [ADT2][fixed on trunk] → [ADT2 RTM][fixed on trunk] [ETA 05/15]
adding adt1.0.0+ for checkin to the 1.0 branch.  Please get drivers approval
again since it's been more than 3 days since the last approval and afterwards
check into the 1.0 branch.
Comment on attachment 80831 [details] [diff] [review]
v2.4

a=chofmann,brendan
please check in to 1.0 branch asap to get this in rc3.
adding the + to match my previous comment.
Keywords: adt1.0.0adt1.0.0+
Fix checked in on the mozilla1.0 branch.
Keywords: fixed1.0.0
Whiteboard: [ADT2 RTM][fixed on trunk] [ETA 05/15] → [ADT2 RTM][fixed on trunk] [ETA 05/15],custrtm-
*** Bug 155588 has been marked as a duplicate of this bug. ***
*** Bug 157408 has been marked as a duplicate of this bug. ***
Crash Signature: [@nsXMLContentSink::Observe]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: