514425 - [HTML5] Crash [@ nsHTMLFormElement::RemoveElement] with form and input

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Martijn Wargers (dead)]

Attachment: zipped up testcase

I'm sometimes getting this crash while browsing too (it seems, when I press the stop button, while the page is loading).
Anyway, I'm getting this crash with the zipped up testcase too.
To reproduce:
- Unzip the zipped up testcase, open the file named 'parentframe2.htm' and allow the enhanced privilege prompt. It usually crashes within 10 seconds or so.

http://crash-stats.mozilla.com/report/index/3218ccdc-e66a-4374-93ef-08a9b2090903?p=1
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:562
5 	xul.dll 	google_breakpad::ExceptionHandler::HandlePureVirtualCall 	toolkit/crashreporter/google-breakpad/src/client/windows/handler/exception_handler.cc:506
6 	mozcrt19.dll 	_purecall 	obj-firefox/memory/jemalloc/crtsrc/purevirt.c:47
7 	xul.dll 	nsHTMLFormElement::RemoveElement 	content/html/content/src/nsHTMLFormElement.cpp:1463
8 	xul.dll 	nsGenericHTMLFormElement::ClearForm 	content/html/content/src/nsGenericHTMLElement.cpp:2325
9 	mozcrt19.dll 	arena_dalloc 	obj-firefox/memory/jemalloc/crtsrc/jemalloc.c:4548
10 	xul.dll 	nsHTMLInputElement::`vector deleting destructor' 	
11 	xul.dll 	nsNodeUtils::LastRelease 	content/base/src/nsNodeUtils.cpp:259
12 	xul.dll 	nsGenericElement::Release 	content/base/src/nsGenericElement.cpp:4159
13 	xul.dll 	nsRefPtr<nsIDOMEventListener>::~nsRefPtr<nsIDOMEventListener> 	obj-firefox/xpcom/build/nsCOMPtr.cpp:81
14 	xul.dll 	nsHtml5TreeOperation::~nsHtml5TreeOperation 	parser/html/nsHtml5TreeOperation.cpp:60
15 	xul.dll 	nsHtml5TreeOperation::`scalar deleting destructor' 	
16 	xul.dll 	nsTArray<nsHtml5TreeOperation>::DestructRange 	obj-firefox/dist/include/nsTArray.h:862
17 	xul.dll 	nsTArray<nsHtml5TreeOperation>::RemoveElementsAt 	obj-firefox/dist/include/nsTArray.h:663
18 	xul.dll 	nsTArray<nsHtml5TreeOperation>::Clear 	obj-firefox/dist/include/nsTArray.h:674
19 	xul.dll 	nsHtml5TreeBuilder::end 	parser/html/nsHtml5TreeBuilderCppSupplement.h:280
20 	xul.dll 	nsHtml5TreeBuilder::endTokenization 	parser/html/nsHtml5TreeBuilder.cpp:555
21 	xul.dll 	nsHtml5Tokenizer::end 	parser/html/nsHtml5Tokenizer.cpp:3283
22 	xul.dll 	nsHtml5Parser::DidBuildModel 	parser/html/nsHtml5Parser.cpp:778
23 	xul.dll 	nsHtml5Parser::Terminate 	parser/html/nsHtml5Parser.cpp:440
24 	xul.dll 	xul.dll@0x353f0b 	
25 	xul.dll 	nsDocShell::Stop 	docshell/base/nsDocShell.cpp:3964
26 	xul.dll 	nsDocShell::Stop 	docshell/base/nsDocShell.cpp:3992
27 	xul.dll 	nsGlobalWindow::Stop 	dom/base/nsGlobalWindow.cpp:4437
28 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
29 	xul.dll 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:2710

Comment 1

[Martijn Wargers (dead)]

You might want to try and crash it online, but I haven't been able to: (not sure why not)
jar:https://bugzilla.mozilla.org/attachment.cgi?id=398391!/parentframe2.htm

Comment 2

[georgi - hopefully not receiving bugspam]

not crashing on linux with the bugzilla testcase.
got assertion:
###!!! ASSERTION: need to implement cancel when downloading: '!mIsPending', file /opt/pub/firefox-35/src/modules/libjar/nsJARChannel.cpp, line 397

Comment 3

[Brian Carpenter [:geeknik]]

I was pointed here from Bug 514589. Just received the following crash @ http://lite.facebook.com/geeknik with the 9/13 build of Minefield:

http://crash-stats.mozilla.com/report/index/808b0019-fc10-4b54-86a9-e11bc2090913

Comment 4

[PikeUK]

FWIW I've had this crash twice on trunk. Both crashes occurred after I closed (by middle-clicking) a tab that was still loading.

http://crash-stats.mozilla.com/report/index/bp-e14a8781-163f-4fe0-8130-5adf32090909
http://crash-stats.mozilla.com/report/index/80a3fc6b-fe6e-47c4-9075-b5fff2090921

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible fix

Martijn, does this fix the crash for you?  I tried reproducing over here using the testcase and couldn't....

Comment 7

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

Comment on attachment 402881 [details] [diff] [review]
Possible fix

Why is ClearForm virtual? It doesn't seem to be overridden anywhere. Though given the amount of code it calls out to it might be safest to still not call it from the dtor.

Comment 8

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

Oh, I guess it lives on nsIFormControl so it sort of makes sense to keep it virtual.

Comment 9

[georgi - hopefully not receiving bugspam]

> given the amount of code it calls out to it might be safest to still not call it from the dtor

if destructors calling too much code are a realistic threat, probably static analysis can search for them.

Comment 10

[Martijn Wargers (dead)]

(In reply to comment #6)
> Martijn, does this fix the crash for you?  I tried reproducing over here using
> the testcase and couldn't....

Yeah, it seems like it is already fixed somehow, I can't also reproduce anymore, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090922 Minefield/3.7a1pre (.NET CLR 3.5.30729)

Comment 11

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

(In reply to comment #9)
> > given the amount of code it calls out to it might be safest to still not call it from the dtor
> 
> if destructors calling too much code are a realistic threat, probably static
> analysis can search for them.

Depends on what you mean by "threat". I can't think of a way to exploit a pure virtual function call into a security problem. But it certainly can be a stability problem.

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK.  I guess I'll check this in and see whether I stop crashing with this stack....

Comment 13

[Martijn Wargers (dead)]

Fwiw, I did some more testing, but I don't seem to crash anymore with this stack trace.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/9f3d99c80da2

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 402881 [details] [diff] [review]
Possible fix

It might be worth taking this on 1.9.2 as part of our crash effort, though we know of no obvious way to trigger the bug there.  The code is clearly wrong...

Comment 16

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

Comment on attachment 402881 [details] [diff] [review]
Possible fix

Definitely!

Comment 17

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/23de544502df

Comment 18

[Tony Chung [:tchung]]

martijn, can you verify this on branch and trunk?   thanks.

Comment 19

[Martijn Wargers (dead)]

No, I couldn't reproduce this crash anymore, as I mentioned in comment 10.