Last Comment Bug 667396 - Crash [@ nsINode::GetOwnerDoc()] with getAccessibleFor(null)
: Crash [@ nsINode::GetOwnerDoc()] with getAccessibleFor(null)
Status: RESOLVED FIXED
: crash, testcase
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla7
Assigned To: Trevor Saunders (:tbsaunde)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-27 01:14 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-09-01 09:34 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (uses enhanced privileges) (327 bytes, text/html)
2011-06-27 01:14 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
trivial patch (811 bytes, patch)
2011-06-27 22:17 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review
patch v2 (838 bytes, patch)
2011-06-28 15:42 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2011-06-27 01:14:38 PDT
Created attachment 542107 [details]
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) 2011-06-27 22:17:11 PDT
Created attachment 542377 [details] [diff] [review]
trivial patch

null check the node arg

Review: surkov
Comment 2 alexander :surkov 2011-06-27 23:13:19 PDT
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) 2011-06-28 15:39:26 PDT
(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) 2011-06-28 15:42:58 PDT
Created attachment 542631 [details] [diff] [review]
patch v2
Comment 5 alexander :surkov 2011-06-28 19:22:56 PDT
(In reply to comment #3)

> we need to do *aAccessible = nsnull first 

right, thank you
Comment 6 Trevor Saunders (:tbsaunde) 2011-06-30 10:40:42 PDT
landed http://hg.mozilla.org/mozilla-central/rev/98285b0ee9a1
Comment 7 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-06-30 10:52:59 PDT
Would this fix bug 667398 too?

Note You need to log in before you can comment on or make changes to this bug.