473284 - Crash when calling advanceFocusIntoSubtree({})

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Null deref

Thread 0 Crashed:
0   nsINode::IsInDoc() const + 9 (nsINode.h:282)
1   nsINode::GetCurrentDoc() const + 17 (nsINode.h:293)
2   nsIContent::GetDocument() const + 17 (nsIContent.h:142)
3   nsFocusController::MoveFocus(int, nsIDOMElement*) + 81 (nsFocusController.cpp:297)
4   nsXULCommandDispatcher::AdvanceFocusIntoSubtree(nsIDOMElement*) + 59 (nsXULCommandDispatcher.cpp:234)
5   NS_InvokeByIndex_P + 99 (xptcinvoke_unixish_x86.cpp:179)
6   XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) + 6227 (xpcwrappednative.cpp:2424)
7   XPC_WN_CallMethod(JSContext*, JSObject*, unsigned int, long*, long*) + 399 (xpcwrappednativejsops.cpp:1477)
8   js_Invoke + 2283 (jsinterp.cpp:1318)
...

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: some more tests

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: some more tests

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: and more

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

I decided to not to change the nsIFocusController interface to use nsINode/nsIContent so that the patch can be taken to branch.
(Applies cleanly to 1.9.0, 1.9.1, trunk, and with some fuzz to 1.8.1 too)

And because Enn is fixing focus handling, I didn't want to change the internals
of focuscontroller too much.

Comment 5

[Jesse Ruderman]

It's hard to tell from looking at the test which statements are supposed to throw and which aren't.  I think the ones that aren't supposed to throw shouldn't be in try..catch.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Whenever using {} should throw in the testcases.

Comment 7

[Mats Palmgren (inactive)]

Attachment: and even more

Added tests for document.commandDispatcher.focusedWindow and made
the PASS condition exact.

Comment 8

[Mats Palmgren (inactive)]

Comment on attachment 356695 [details] [diff] [review]
patch

r+sr=mats

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch with test

Comment 10

[Jesse Ruderman]

http://hg.mozilla.org/mozilla-central/rev/b4b8272ac9da included tests.

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 362695 [details] [diff] [review]
patch with test

Approved for 1.9.0.8, a=dveditz for release-drivers

Comment 12

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

Verified for 1.9.0.8 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.8pre) Gecko/2009031905 GranParadiso/3.0.8pre (.NET CLR 3.5.30729). Verified crash with 1.9.0.7.

I note that it crashes 1.9.1 though with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090317 Shiretoko/3.1b4pre.

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 362695 [details] [diff] [review]
patch with test

a191=drivers

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8a3631b43eeb

Comment 15

[Aakash Desai [:aakashd]]

verified FIXED on builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090416 Minefield/3.6a1pre ID:20090416030845

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090417 Shiretoko/3.5b4pre ID:20090417030722