998532 - crash in nsEditor::DetermineCurrentDirection()

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Martijn Wargers (dead)]

I'm seeing this crash in current trunk build. I don't have a testcase, atm.

This bug was filed from the Socorro interface and is 
report bp-38f42156-dea1-4efc-bfff-743922140418.
=============================================================
0 	XUL 	nsEditor::DetermineCurrentDirection() 	obj-firefox/x86_64/dist/include/nsINode.h
1 	XUL 	nsEditor::SwitchTextDirection() 	editor/libeditor/base/nsEditor.cpp
2 	XUL 	NS_InvokeByIndex 	xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp
3 	XUL 	XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) 	js/xpconnect/src/XPCWrappedNative.cpp
4 	XUL 	XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp
5 	XUL 	js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) 	js/src/jscntxtinlines.h
6 	XUL 	Interpret 	js/src/vm/Interpreter.cpp
7 	XUL 	js::RunScript(JSContext*, js::RunState&) 	js/src/vm/Interpreter.cpp

Comment 1

[Martijn Wargers (dead)]

Let me know if you need a testcase, I have a case where it's crashing reproducibly, but getting a testcase from it, might be quite some work. I could test a patch for it, though, to see if it would fix the crash.

Comment 2

[(no longer active)]

Can you please attach the test case even if it's not minimal?  I think this is a simple missing null check, I can attach a patch in a sec.

Comment 3

[(no longer active)]

Attachment: Patch (v1)

Comment 4

[Martijn Wargers (dead)]

Sorry for the delay!
The testcase is not even not minimal, it's part of a fuzzing framework and it would take quite some time to get it even a little bit isolated.
But I'll try out the patch and see if it fixes this crash, ok?

Comment 5

[Martijn Wargers (dead)]

Comment on attachment 8409483 [details] [diff] [review]
Patch (v1)

Review of attachment 8409483 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this patch seems to fix the crash. I tested without and with the patch in a debug build on MacOS X.

Comment 6

[(no longer active)]

Comment on attachment 8409483 [details] [diff] [review]
Patch (v1)

Thanks Martijn.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8409483 [details] [diff] [review]
Patch (v1)

Review of attachment 8409483 [details] [diff] [review]:
-----------------------------------------------------------------

Seems like it should be fairly easy to come up with a testcase. Just remove the root element from the document and do some editing stuff that hits this code path.

Comment 8

[(no longer active)]

I tried to write a test and failed.  Will attach my patch so far for future reference...

Comment 9

[(no longer active)]

Attachment: My attempt at a test

Comment 10

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0857ad15c4d7

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0857ad15c4d7

Comment 12

[Martijn Wargers (dead)]

It doesn't crash anymore in today's trunk build on the unminimized testcase.