667396 - Crash [@ nsINode::GetOwnerDoc()] with getAccessibleFor(null)

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase (uses enhanced privileges)

See testcase, which crashes current trunk build. The testcase uses enhanced privileges.

https://crash-stats.mozilla.com/report/index/bp-426175f8-445a-45bc-a46e-3c58d2110627
0 	xul.dll 	nsINode::GetOwnerDoc 	obj-firefox/dist/include/nsINode.h:415
1 	xul.dll 	nsAccessibilityService::GetAccessible 	accessible/src/base/nsAccessibilityService.cpp:848
2 	xul.dll 	nsAccessibilityService::GetAccessibleFor 	accessible/src/base/nsAccessibilityService.cpp:640
3 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
4 	xul.dll 	XPC_WN_CallMethod 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1607
5 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:656
6 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:4550
7 	mozjs.dll 	JSCompartment::wrap 	js/src/jscompartment.cpp:224
8 	mozjs.dll 	js::ExternalExecute 	js/src/jsinterp.cpp:944
9 	mozjs.dll 	EvaluateUCScriptForPrincipalsCommon 	js/src/jsapi.cpp:4984
10 	mozjs.dll 	JS_EvaluateUCScriptForPrincipalsVersion 	js/src/jsapi.cpp:5000
11 	xul.dll 	nsJSContext::EvaluateString 	dom/base/nsJSEnvironment.cpp:1453
12 	xul.dll 	nsScriptLoader::EvaluateScript 	content/base/src/nsScriptLoader.cpp:906
13 	xul.dll 	nsScriptLoader::ProcessRequest 	content/base/src/nsScriptLoader.cpp:799
14 	xul.dll 	nsScriptLoader::ProcessScriptElement 	content/base/src/nsScriptLoader.cpp:745
15 	xul.dll 	nsScriptElement::MaybeProcessScript 	content/base/src/nsScriptElement.cpp:182
16 	xul.dll 	nsHTMLScriptElement::MaybeProcessScript 	content/html/content/src/nsHTMLScriptElement.cpp:586
17 	xul.dll 	nsHTMLScriptElement::DoneAddingChildren 	content/html/content/src/nsHTMLScriptElement.cpp:513
18 	xul.dll 	nsHtml5TreeOpExecutor::RunScript 	parser/html/nsHtml5TreeOpExecutor.cpp:730
19 	xul.dll 	nsHtml5TreeOpExecutor::RunFlushLoop 	parser/html/nsHtml5TreeOpExecutor.cpp:525
20 	xul.dll 	nsHtml5ExecutorFlusher::Run 	parser/html/nsHtml5StreamParser.cpp:156
etc..

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: trivial patch

null check the node arg

Review: surkov

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 542377 [details] [diff] [review]
trivial patch

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

r=me with comment addressed

::: accessible/src/base/nsAccessibilityService.cpp
@@ +639,1 @@
>  

I think we should fail peaceful. I don't think JS needs exception if null is passed. Null as accessible is suitable answer for null as DOM node.

How about

if (!aNode)
  return NS_OK;

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to comment #2)
> Comment on attachment 542377 [details] [diff] [review] [review]
> trivial patch
> 
> Review of attachment 542377 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> r=me with comment addressed
> 
> ::: accessible/src/base/nsAccessibilityService.cpp
> @@ +639,1 @@
> >  
> 
> I think we should fail peaceful. I don't think JS needs exception if null is
> passed. Null as accessible is suitable answer for null as DOM node.

ok, that doesn't seem to unreasonable to me.

we need to do *aAccessible = nsnull first otherwise we're returning  whatever random value was in that pointer, but  otherwise  I'm fine with this.

> if (!aNode)
>   return NS_OK;

Comment 4

[Trevor Saunders (:tbsaunde)]

Attachment: patch v2

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #3)

> we need to do *aAccessible = nsnull first 

right, thank you

Comment 6

[Trevor Saunders (:tbsaunde)]

landed http://hg.mozilla.org/mozilla-central/rev/98285b0ee9a1

Comment 7

[Martijn Wargers (dead)]

Would this fix bug 667398 too?