462787 - Crash [@ inDOMUtils::GetParentForNode][@ nsINode::GetOwnerDoc] with passing null arguments in domutils functions

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase (needs enhanced privileges)

See testcase, which crashes current trunk build when clicking on the buttons.

http://crash-stats.mozilla.com/report/index/1887ae9c-a91e-11dd-9698-001cc4e2bf68?p=1
0  	xul.dll  	nsINode::GetOwnerDoc  	 obj-firefox/dist/include/content/nsINode.h:267
1 	xul.dll 	nsInspectorCSSUtils::GetBindingURLs 	layout/style/nsInspectorCSSUtils.cpp:192
2 	xul.dll 	inDOMUtils::GetBindingURLs 	layout/inspector/src/inDOMUtils.cpp:210
3 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
4 	xul.dll 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:2402

http://crash-stats.mozilla.com/report/index/e60c3a38-a91c-11dd-b155-001a4bd43ed6?p=1
0  	xul.dll  	inDOMUtils::GetParentForNode  	 layout/inspector/src/inDOMUtils.cpp:143
1 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
2 	xul.dll 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:2402

Those methods probably need some null checks or something.

Comment 1

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

1. add missing null-checks for GetBindingURLs, GetParentForNode and
   IsIgnorableWhitespace.

GetRuleLine() returns NS_OK for null --
should it be NS_ERROR_NULL_POINTER like the others?

Comment 2

[Martijn Wargers (dead)]

Sorry, I filed bug 462789 separately, but that one also probably just needs a simple null check.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 345998 [details] [diff] [review]
Patch rev. 1

>-  NS_PRECONDITION(aDataNode, "Must have a character data node");
>   NS_PRECONDITION(aReturn, "Must have an out parameter");
>+
>+  if (!aDataNode)
>+    return NS_ERROR_NULL_POINTER;

Just use NS_ENSURE_ARG_POINTER(aDataNode) instead.

> NS_IMETHODIMP
> inDOMUtils::GetParentForNode(nsIDOMNode* aNode,
>                              PRBool aShowingAnonymousContent,
>                              nsIDOMNode** aParent)
> {
>-    // First do the special cases -- document nodes and anonymous content
>+  if (!aNode)
>+    return NS_ERROR_NULL_POINTER;

Same here: NS_ENSURE_ARG_POINTER(aNode).

> NS_IMETHODIMP
> inDOMUtils::GetCSSStyleRules(nsIDOMElement *aElement,
>                              nsISupportsArray **_retval)
> {
>-  if (!aElement) return NS_ERROR_NULL_POINTER;
>+  if (!aElement)
>+    return NS_ERROR_NULL_POINTER;

NS_ENSURE_ARG_POINTER(aElement)

> NS_IMETHODIMP 
> inDOMUtils::GetBindingURLs(nsIDOMElement *aElement, nsIArray **_retval)
> {
>+  if (!aElement)
>+    return NS_ERROR_NULL_POINTER;
>+  

NS_ENSURE_ARG_POINTER(aElement)

>diff --git a/layout/inspector/tests/Makefile.in b/layout/inspector/tests/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/layout/inspector/tests/Makefile.in

Put what you now have in layout/inspector/tests/mochitest/ directly in layout/inspector/tests (don't forget to adjust the DEPTH and relativesrcdir in the makefile, though).

r+sr=dbaron with that

Comment 4

[Mats Palmgren (inactive)]

David, you meant that they all should use NS_ENSURE_ARG_POINTER, right?
(i.e. GetRuleLine, SetContentState, GetContentState too)

Specifically, should GetRuleLine() continue to return NS_OK for null?
(unlike the others)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yeah, the probably all should, and I tend to think GetRuleLine should return failure for both null checks...

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2 (nits fixed)

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 348701 [details] [diff] [review]
Patch rev. 2 (nits fixed)

a191=beltzner

Comment 8

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

Mats, can I land this on trunk for you? We should try to land it as soon as possible

Comment 9

[Mats Palmgren (inactive)]

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

-> FIXED

Comment 10

[Mats Palmgren (inactive)]

Pushed to 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4986823930a5

Comment 11

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081218 Minefield/3.2a1pre

Comment 12

[Aakash Desai [:aakashd]]

verified FIXED on Shiretoko: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090422 Shiretoko/3.5b4pre ID:20090422042031