Closed Bug 462787 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Layout, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: martijn.martijn, Assigned: mats)

References

Details

(Keywords: crash, testcase, verified1.9.1)

Crash Data

Attachments

(3 files)

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.
Attached patch Patch rev. 1Splinter Review
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?
Assignee: nobody → mats.palmgren
Attachment #345998 - Flags: superreview?(dbaron)
Attachment #345998 - Flags: review?(dbaron)
OS: Windows XP → All
Hardware: PC → All
Sorry, I filed bug 462789 separately, but that one also probably just needs a simple null check.
Blocks: 462789
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
Attachment #345998 - Flags: superreview?(dbaron)
Attachment #345998 - Flags: superreview+
Attachment #345998 - Flags: review?(dbaron)
Attachment #345998 - Flags: review+
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)
Flags: blocking1.9.1?
Yeah, the probably all should, and I tend to think GetRuleLine should return failure for both null checks...
Flags: blocking1.9.1? → wanted1.9.1+
Attachment #348701 - Flags: approval1.9.1?
Comment on attachment 348701 [details] [diff] [review]
Patch rev. 2 (nits fixed)

a191=beltzner
Attachment #348701 - Flags: approval1.9.1? → approval1.9.1+
Mats, can I land this on trunk for you? We should try to land it as soon as possible
http://hg.mozilla.org/mozilla-central/rev/cdf705dea843

-> FIXED
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla1.9.2a1
Whiteboard: [needs baking and 1.9.1 landing]
Pushed to 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4986823930a5
Keywords: fixed1.9.1
Whiteboard: [needs baking and 1.9.1 landing]
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081218 Minefield/3.2a1pre
Status: RESOLVED → VERIFIED
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
Crash Signature: [@ inDOMUtils::GetParentForNode] [@ nsINode::GetOwnerDoc]
You need to log in before you can comment on or make changes to this bug.