Closed Bug 667315 Opened 13 years ago Closed 13 years ago

XSLT crashes Firefox 5 and Firefox 7 (but not Firefox 4)


(Core :: XSLT, defect)

Not set



Tracking Status
firefox5 --- affected
firefox6 + fixed
firefox7 + fixed
status2.0 --- unaffected
status1.9.2 --- unaffected
status1.9.1 --- unaffected


(Reporter: travisemmitt, Assigned: peterv)




(Keywords: crash, regression, reproducible, Whiteboard: [sg:critical?][qa-])

Crash Data


(1 file, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:5.0) Gecko/20100101 Firefox/5.0
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:5.0) Gecko/20100101 Firefox/5.0

Some (but not all) XSLT crashes Firefox 5 on Windows 7 and XP.  This crash did not happen on Firefox 4 or earlier.

The crash happens on local XSLT files as well.

Reproducible: Always

Steps to Reproduce:
1. Load the page indicated in the url

Actual Results:  
Loading the XSLT at the cited URL crashes Firefox 5 but not 4 and earlier.

Expected Results:  
Firefox 5 should not crash, because Firefox 4 did not crash (and neither do IE or Chrome).

I haven't tried methodically whittling down the XSLT to find out whether it is a particular function or extension which is causing the crash.  If that is something that would be useful, I can do it; otherwise, I am hoping other people report similar problems.
Mozilla/5.0 (X11; Linux x86_64; rv:7.0a1) Gecko/20110626 Firefox/7.0a1

2 	nsTArray<txOutputTransaction*, nsTArrayDefaultAllocator>::~nsTArray 	nsTArray.h:374
12 	txResultBuffer::flushToHandler 	content/xslt/src/xslt/txBufferingHandler.cpp:477
13 	txCopyOf::execute 	content/xslt/src/xslt/txInstructions.cpp:432
14 	txXSLTProcessor::execute 	content/xslt/src/xslt/txXSLTProcessor.cpp:104
15 	txMozillaXSLTProcessor::TransformToDoc 	content/xslt/src/xslt/txMozillaXSLTProcessor.cpp:676
16 	nsTransformBlockerEvent::Run 	content/xslt/src/xslt/txMozillaXSLTProcessor.cpp:558
17 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:617
18 	NS_ProcessNextEvent_P 	obj-firefox/xpcom/build/nsThreadUtils.cpp:245
19 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:110
20 	MessageLoop::Run 	ipc/chromium/src/base/
21 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:189
22 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:222
23 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3573
24 	firefox 	main 	browser/app/nsBrowserApp.cpp:198
26 	firefox 	firefox@0x193f
Crash Signature: [@ ]
Keywords: crash
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
Last good nightly: 2011-04-08 First bad nightly: 2011-04-09

Keywords: regression
[@ ] 

[@ ] 

[@ ]

[@ ] 

[@ ]
Crash Signature: [@ ] → [@ ][@ ]
The first bad revision is:
changeset:   67678:a6822a5df633
user:        Peter Van der Beken <>
date:        Thu Dec 02 11:12:27 2010 -0500
summary:     Fix for bug 603844 (Leak txUnknownHandler+ with transformToDocument(textnode)). r=sicking.
Component: File Handling → XSLT
Product: Firefox → Core
QA Contact: file.handling → xslt
Depends on: 603844
Ever confirmed: true
Keywords: reproducible
Summary: XSLT crashes Firefox 5 (but not 4) → XSLT crashes Firefox 5 and Firefox 7 (but not Firefox 4)
Travis, are you able to post some related Report IDs from about:crashes, like I've done in comment 3? The important thing is "bp-" strings. My IDs are from Linux so it would be interesting to also see what they look like with Windows.
0 	ntdll.dll 	KiFastSystemCallRet 	
1 	ntdll.dll 	ZwWaitForSingleObject 	
2 	kernel32.dll 	WaitForSingleObjectEx 	
3 	kernel32.dll 	WaitForSingleObject 	
4 	xul.dll 	google_breakpad::ExceptionHandler::WriteMinidumpOnHandlerThread 	toolkit/crashreporter/google-breakpad/src/client/windows/handler/
5 	xul.dll 	google_breakpad::ExceptionHandler::HandlePureVirtualCall 	toolkit/crashreporter/google-breakpad/src/client/windows/handler/
6 	mozcrt19.dll 	_purecall 	obj-firefox/memory/jemalloc/crtsrc/purevirt.c:47
7 	xul.dll 	flushTransaction 	content/xslt/src/xslt/txBufferingHandler.cpp:477
8 	xul.dll 	txResultBuffer::flushToHandler 	content/xslt/src/xslt/txBufferingHandler.cpp:503
9 	xul.dll 	txResultTreeFragment::flushToHandler 	content/xslt/src/xslt/txRtfHandler.cpp:89
10 	xul.dll 	txCopyOf::execute 	content/xslt/src/xslt/txInstructions.cpp:432
11 	xul.dll 	txXSLTProcessor::execute 	content/xslt/src/xslt/txXSLTProcessor.cpp:104
12 	xul.dll 	txMozillaXSLTProcessor::TransformToDoc 	content/xslt/src/xslt/txMozillaXSLTProcessor.cpp:715
13 	xul.dll 	nsTransformBlockerEvent::Run 	content/xslt/src/xslt/txMozillaXSLTProcessor.cpp:595
14 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:618
15 	xul.dll 	TimerThread::RemoveTimer 	xpcom/threads/TimerThread.cpp:417
16 	xul.dll 	MessageLoop::RunInternal 	ipc/chromium/src/base/
17 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/
18 	mozcrt19.dll 	_VEC_memzero 	
19 	xul.dll 	xul.dll@0x37057f 	
20 	firefox.exe 	firefox.exe@0x1bb7 	
21 	kernel32.dll 	GetCodePageFileInfo 	
22 	kernel32.dll 	BaseProcessStart 	
23 	firefox.exe 	firefox.exe@0x186f
Crash Signature: [@ ][@ ] → [@ ][@ ][@ _purecall | flushTransaction ]
Blocks: 603844
No longer depends on: 603844
Peter, can you look at this?
Assignee: nobody → peterv
Attached patch v1 (obsolete) — Splinter Review
This needs the txUnknownHandler to flush while we're already flushing a different buffer to the result handler (flushing the rtf buffer for example). Apparently we didn't have a test for that.

This patch avoids the |delete this|'s that we have in the txUnknownHandler. Instead we forward the remaining hooks to the new result handler from the txUnknownHandler and keep the txUnknownHandler alive until the execution state dies. We clear the buffer so this shouldn't give too much bloat.
Attachment #542900 - Flags: review?(jonas)
Attached patch v1 (diff -w) (obsolete) — Splinter Review
Comment on attachment 542900 [details] [diff] [review]

Review of attachment 542900 [details] [diff] [review]:

Please also add tests to not only ensure that we don't crash, but that we also output the correct result. For example that if the variable starts with <html> that we go into HTML mode etc. You can just use XSLTProcessor.transformToDocument to do that.

r=me with that

::: content/xslt/src/xslt/txUnknownHandler.cpp
@@ +107,5 @@
> +        // leak mEs->mResultHandler in createHandlerAndFlush and we'll crash on
> +        // the last line (delete this).
> +        NS_ASSERTION(mEs->mResultHandler == this,
> +                     "We're leaking mEs->mResultHandler and are going to "
> +                     "crash.");

The crash part sounds like it might no longer be true. At least that part of the comment is wrong. Maybe just say that we might crash since we're in unknown territory.
Attachment #542900 - Flags: review?(jonas) → review+
Group: core-security
XSLT is an area of recent interest amongst security researchers. If this instance was found on a live web server isn't it likely to be rediscovered? How common is the condition that causes the crash?

It would be much better to get this fix into Firefox 6 unless this is a risky patch.
Whiteboard: [sg:critical?]
The crash happens on my local copy of the file as well, so I don't want anyone thinking the web server configuration has anything to do with it.  Like I said, if you guys need help reducing the XSLT to the simplest possible code that creates a crash, I can do that.  I suspect the crash has something to do with either an XSLT extension or perhaps my use of the document() function, although it might be something else entirely.

Did you guys find the cause of the problem?

As for priority, all I can say is that I personally won't be upgrading to Firefox 5 until the crash is fixed, because I use Firefox to test my XML-based sites before rendering them into XHTML and pushing live.  Until I can safely use Firefox 5, I won't be able to recommend it to others, and might have to start recommending Chrome or IE9.  So if your goal is to maximize Firefox users, my recommendation is to fix the crash in Firefox 5 so that long-time developers like myself can continue to use (and promote) Firefox.
There's a reviewed fix attached to the bug, so yes, the problem has been located.
Travis, the suggestion in comment 16 to fix this for Firefox 6 means to fix it for the scheduled security update to Firefox 5 (scheduled for August 16).  Fixing it before that would involve doing a special release just for this fix, and while this is definitely a problem it's not clear that it's serious enough for that.  Typically we would only do that for serious compatibility regressions or security bugs being actively exploited.
Okay, thanks for the clarification!
Grmbl, I accidently checked in the testcases.
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Comment on attachment 542900 [details] [diff] [review]

This is a fairly safe patch, we delay deleting an object while there might be callers that have a reference to it (and forward all uses of that object to the object that replaced it). We used to delete it right away and callers potentially used a dangling pointer. We should take this for Firefox 6.
Attachment #542900 - Flags: approval-mozilla-aurora?
Attached patch v1Splinter Review
Attachment #542900 - Attachment is obsolete: true
Attachment #542903 - Attachment is obsolete: true
Attachment #543814 - Flags: review+
Attachment #543814 - Flags: approval-mozilla-aurora?
Attachment #542900 - Flags: approval-mozilla-aurora?
Comment on attachment 543814 [details] [diff] [review]

this is already in Aurora 7 but we'd like it in Beta 7. Setting approval-mozilla-beta+
Attachment #543814 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
(In reply to comment #24)
> this is already in Aurora 7 but we'd like it in Beta 7. Setting
> approval-mozilla-beta+

I don't understand, did you mean Beta 6?
qa+ for fix verification in Firefox 7
Whiteboard: [sg:critical?] → [sg:critical?][qa+]
I wasn't able to reproduce this problem on versions version 5.0.1 on XP, nor 5.0.1, 6.0.2, 7.09rc, nightly on Linux using the information on comment #0, so I am not able to verify it.
I notice that the URL of the testcase of the bug is behind authorization now. There is no file attached to the bug.

QA cannot verify.

Since the checked in testcase is passing, I'm going to assume it is good but QA cannot check manually.
Based on recent comments and my own failed attempt, QA cannot verify this fix due to an inaccessible testcase. Please provide a working testcase or simply mark VERIFIED if the checked-in test is enough.
Whiteboard: [sg:critical?][qa+] → [sg:critical?][qa-]
Group: core-security
Keywords: testcase-wanted
You need to log in before you can comment on or make changes to this bug.