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

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Travis Emmitt]

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.

Comment 1

[Thomas Ahlblom]

Reproduced:
Mozilla/5.0 (X11; Linux x86_64; rv:7.0a1) Gecko/20110626 Firefox/7.0a1


0 	libc-2.13.so 	libc-2.13.so@0x32405 	
1 	libc-2.13.so 	libc-2.13.so@0x3567f 	
2 	libxul.so 	nsTArray<txOutputTransaction*, nsTArrayDefaultAllocator>::~nsTArray 	nsTArray.h:374
3 	ld-2.13.so 	ld-2.13.so@0x55f 	
4 	libc-2.13.so 	libc-2.13.so@0x6b9f 	
5 	libm-2.13.so 	libm-2.13.so@0x281fff 	
6 	ld-2.13.so 	ld-2.13.so@0xd711 	
7 	libstdc++.so.6.0.16 	libstdc++.so.6.0.16@0xbee4c 	
8 	libpthread-2.13.so 	libpthread-2.13.so@0xe11c 	
9 	libstdc++.so.6.0.16 	libstdc++.so.6.0.16@0xbcff5 	
10 	libstdc++.so.6.0.16 	libstdc++.so.6.0.16@0xbd022 	
11 	libstdc++.so.6.0.16 	libstdc++.so.6.0.16@0xbda3e 	
12 	libxul.so 	txResultBuffer::flushToHandler 	content/xslt/src/xslt/txBufferingHandler.cpp:477
13 	libxul.so 	txCopyOf::execute 	content/xslt/src/xslt/txInstructions.cpp:432
14 	libxul.so 	txXSLTProcessor::execute 	content/xslt/src/xslt/txXSLTProcessor.cpp:104
15 	libxul.so 	txMozillaXSLTProcessor::TransformToDoc 	content/xslt/src/xslt/txMozillaXSLTProcessor.cpp:676
16 	libxul.so 	nsTransformBlockerEvent::Run 	content/xslt/src/xslt/txMozillaXSLTProcessor.cpp:558
17 	libxul.so 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:617
18 	libxul.so 	NS_ProcessNextEvent_P 	obj-firefox/xpcom/build/nsThreadUtils.cpp:245
19 	libxul.so 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:110
20 	libxul.so 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:218
21 	libxul.so 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:189
22 	libxul.so 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:222
23 	libxul.so 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3573
24 	firefox 	main 	browser/app/nsBrowserApp.cpp:198
25 	libc-2.13.so 	libc-2.13.so@0x1eeac 	
26 	firefox 	firefox@0x193f

Comment 2

[Thomas Ahlblom]

Last good nightly: 2011-04-08 First bad nightly: 2011-04-09

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=27d6a4a5e20f&tochange=7d90da136b2c

Comment 3

[Thomas Ahlblom]

bp-26a25d8c-953c-44d2-a39f-146542110626
[@ linux-gate.so@0x425 ] 

bp-650154ac-8a4d-4b10-984a-820e32110626
[@ linux-gate.so@0x425 ] 

bp-03f57bff-3748-4304-9c77-007d42110626
[@ linux-gate.so@0x425 ]

bp-2e307c77-cbec-465d-89aa-5d6b82110626
[@ linux-gate.so@0x425 ] 

bp-2bd4ebd1-eabe-4047-a868-4c7362110626
[@ libc-2.13.so@0x32405 ]

Comment 4

[Thomas Ahlblom]

0 	linux-gate.so 	linux-gate.so@0x425 	
1 	libc-2.13.so 	libc-2.13.so@0x2abb0 	
2 	libc-2.13.so 	libc-2.13.so@0x155ff3 	
3 	libc-2.13.so 	libc-2.13.so@0x2dfd1 	
4 	ld-2.13.so 	ld-2.13.so@0x9ba1 	
5 	libstdc++.so.6.0.16 	libstdc++.so.6.0.16@0xab4b 	
6 	ld-2.13.so 	ld-2.13.so@0x1da73 	
7 	libc-2.13.so 	libc-2.13.so@0xbf863 	
8 	libstdc++.so.6.0.16 	libstdc++.so.6.0.16@0x24440 	
9 	libstdc++.so.6.0.16 	libstdc++.so.6.0.16@0xc1607 	
10 	libc-2.13.so 	libc-2.13.so@0x15657f

Comment 5

[Thomas Ahlblom]

The first bad revision is:
changeset:   67678:a6822a5df633
user:        Peter Van der Beken <[e-mail]>
date:        Thu Dec 02 11:12:27 2010 -0500
summary:     Fix for bug 603844 (Leak txUnknownHandler+ with transformToDocument(textnode)). r=sicking.

http://hg.mozilla.org/mozilla-central/rev/a6822a5df633

Comment 6

[Thomas Ahlblom]

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.

Comment 7

[Travis Emmitt]

https://crash-stats.mozilla.com/report/pending/0b462f1f-b0c8-4b06-b9ec-6a9b12110626

Comment 8

[Thomas Ahlblom]

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/exception_handler.cc:763
5 	xul.dll 	google_breakpad::ExceptionHandler::HandlePureVirtualCall 	toolkit/crashreporter/google-breakpad/src/client/windows/handler/exception_handler.cc:707
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/message_loop.cc:219
17 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:202
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

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Peter, can you look at this?

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: v1

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.

Comment 14

[Peter Van der Beken [:peterv]]

Attachment: v1 (diff -w)

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

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.

Comment 16

[Daniel Veditz [:dveditz]]

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.

Comment 17

[Travis Emmitt]

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.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

There's a reviewed fix attached to the bug, so yes, the problem has been located.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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.

Comment 20

[Travis Emmitt]

Okay, thanks for the clarification!

Comment 21

[Peter Van der Beken [:peterv]]

Grmbl, I accidently checked in the testcases.

http://hg.mozilla.org/mozilla-central/rev/84f72e1d1fa2

Comment 22

[Peter Van der Beken [:peterv]]

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

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.

Comment 23

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 24

[Asa Dotzler [:asa]]

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

this is already in Aurora 7 but we'd like it in Beta 7. Setting approval-mozilla-beta+

Comment 25

[Peter Van der Beken [:peterv]]

(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?

Comment 26

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/releases/mozilla-beta/rev/018ff8037095

Comment 27

[u279076]

qa+ for fix verification in Firefox 7

Comment 28

[juan becerra [:juanb]]

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.

Comment 29

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 30

[u279076]

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.