Closed
Bug 129006
Opened 22 years ago
Closed 22 years ago
Invalid xml crashes mozilla after reload [@nsXMLContentSink::Observe]
Categories
(Core :: XSLT, defect, P2)
Core
XSLT
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
|
jst
:
superreview+
asa
:
approval+
|
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.
Reporter | ||
Comment 1•22 years ago
|
||
TalkBack ID: TB3653801Q To crash load invalidate.xml and after that try load validate.xml from the same URL.
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.
Assignee | ||
Comment 6•22 years ago
|
||
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.
Reporter | ||
Comment 7•22 years ago
|
||
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.
Comment 8•22 years ago
|
||
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 10•22 years ago
|
||
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.
Assignee | ||
Updated•22 years ago
|
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]
Assignee | ||
Comment 13•22 years ago
|
||
Attachment #72781 -
Attachment is obsolete: true
Assignee | ||
Comment 14•22 years ago
|
||
Assignee | ||
Comment 15•22 years ago
|
||
Assignee | ||
Comment 16•22 years ago
|
||
Assignee | ||
Comment 17•22 years ago
|
||
Assignee | ||
Comment 18•22 years ago
|
||
Assignee | ||
Comment 19•22 years ago
|
||
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).
Comment 22•22 years ago
|
||
applied the patch, and valid source, invalid stylesheet doesn't display anything for me. The others are fine though.
Assignee | ||
Comment 23•22 years ago
|
||
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 24•22 years ago
|
||
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?
Updated•22 years ago
|
Whiteboard: [ADT2]
Assignee | ||
Comment 25•22 years ago
|
||
Looking for reviews.
Attachment #79734 -
Attachment is obsolete: true
Assignee | ||
Comment 26•22 years ago
|
||
Attachment #80484 -
Attachment is obsolete: true
Assignee | ||
Comment 27•22 years ago
|
||
Attachment #80702 -
Attachment is obsolete: true
Comment 28•22 years ago
|
||
Comment on attachment 80763 [details] [diff] [review] v2.3 r=axel@pike.org
Attachment #80763 -
Flags: review+
Assignee | ||
Comment 29•22 years ago
|
||
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 31•22 years ago
|
||
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+
Assignee | ||
Comment 32•22 years ago
|
||
Checked in on the trunk
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Keywords: adt1.0.0
Resolution: --- → FIXED
Whiteboard: [ADT2] → [ADT2][fixed on trunk]
Assignee | ||
Comment 34•22 years ago
|
||
Sent mail to drivers for checkin to the branch.
Comment 35•22 years ago
|
||
Chris - Can you take a look at this one, and make sure it resolves the bug without any regressions?
Keywords: approval
Comment 36•22 years ago
|
||
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.
Comment 37•22 years ago
|
||
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 38•22 years ago
|
||
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.
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).
Attachment #81239 -
Attachment is obsolete: true
Comment 44•22 years ago
|
||
Testcase in comment #43 works correctly. Verfied with Trunk Build ID 2002043010.
Comment 45•22 years ago
|
||
Let's get this one in after RC2. adt1.0.0- [adt2 RTM]
Comment 46•22 years ago
|
||
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.
Updated•22 years ago
|
Comment 47•22 years ago
|
||
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.
Comment 48•22 years ago
|
||
adding the + to match my previous comment.
Whiteboard: [ADT2 RTM][fixed on trunk] [ETA 05/15] → [ADT2 RTM][fixed on trunk] [ETA 05/15],custrtm-
Comment 50•22 years ago
|
||
*** Bug 155588 has been marked as a duplicate of this bug. ***
Comment 51•22 years ago
|
||
*** Bug 157408 has been marked as a duplicate of this bug. ***
Updated•13 years ago
|
Crash Signature: [@nsXMLContentSink::Observe]
You need to log in
before you can comment on or make changes to this bug.
Description
•