Closed
Bug 462787
Opened 16 years ago
Closed 16 years ago
Crash [@ inDOMUtils::GetParentForNode][@ nsINode::GetOwnerDoc] with passing null arguments in domutils functions
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
VERIFIED
FIXED
mozilla1.9.2a1
People
(Reporter: martijn.martijn, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: crash, testcase, verified1.9.1)
Crash Data
Attachments
(3 files)
885 bytes,
text/html
|
Details | |
9.82 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
8.98 KB,
patch
|
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
OS: Windows XP → All
Hardware: PC → All
Reporter | ||
Comment 2•16 years ago
|
||
Sorry, I filed bug 462789 separately, but that one also probably just needs a simple null check.
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+
Assignee | ||
Comment 4•16 years ago
|
||
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)
Reporter | ||
Updated•16 years ago
|
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+
Assignee | ||
Comment 6•16 years ago
|
||
Attachment #348701 -
Flags: approval1.9.1?
Comment 7•16 years ago
|
||
Comment on attachment 348701 [details] [diff] [review] Patch rev. 2 (nits fixed) a191=beltzner
Attachment #348701 -
Flags: approval1.9.1? → approval1.9.1+
Whiteboard: [needs landing]
Mats, can I land this on trunk for you? We should try to land it as soon as possible
Assignee | ||
Comment 9•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/cdf705dea843 -> FIXED
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla1.9.2a1
Whiteboard: [needs baking and 1.9.1 landing]
Assignee | ||
Comment 10•16 years ago
|
||
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]
Reporter | ||
Comment 11•16 years ago
|
||
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
Comment 12•15 years ago
|
||
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
Keywords: fixed1.9.1 → verified1.9.1
Updated•13 years ago
|
Crash Signature: [@ inDOMUtils::GetParentForNode]
[@ nsINode::GetOwnerDoc]
You need to log in
before you can comment on or make changes to this bug.
Description
•