Last Comment Bug 682827 - Crash [@ nsINode::GetOwnerDoc]
: Crash [@ nsINode::GetOwnerDoc]
Status: RESOLVED FIXED
: crash
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 Windows 7
: -- critical (vote)
: mozilla9
Assigned To: alexander :surkov
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-29 05:48 PDT by Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
Modified: 2011-09-01 13:52 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.85 KB, patch)
2011-08-29 09:05 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Review

Description Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2011-08-29 05:48:39 PDT
https://crash-stats.mozilla.com/report/index/bp-5e75067d-f2dc-4c1e-80b6-12ba22110828
0 	xul.dll 	nsINode::GetOwnerDoc 	obj-firefox/dist/include/nsINode.h:420
1 	xul.dll 	nsAccessibilityService::GetAccessible 	accessible/src/base/nsAccessibilityService.cpp:854
2 	xul.dll 	nsAccessibilityService::GetAccessibleFor 	accessible/src/base/nsAccessibilityService.cpp:646
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:1599
5 		@0x41b2e80 	
6 	mozjs.dll 	js::mjit::EnterMethodJIT 	js/src/methodjit/MethodJIT.cpp:687
7 	mozjs.dll 	js::mjit::JaegerShot 	js/src/methodjit/MethodJIT.cpp:734
8 	mozjs.dll 	js::RunScript 	js/src/jsinterp.cpp:611
9 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:687
10 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:809
11 	mozjs.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5019
12 	xul.dll 	nsJSContext::CallEventHandler 	dom/base/nsJSEnvironment.cpp:1902
13 	xul.dll 	nsGlobalWindow::RunTimeout 	dom/base/nsGlobalWindow.cpp:9247
14 	xul.dll 	nsGlobalWindow::TimerCallback 	dom/base/nsGlobalWindow.cpp:9685
15 	xul.dll 	nsTimerImpl::Fire 	xpcom/threads/nsTimerImpl.cpp:424
16 	xul.dll 	nsTimerEvent::Run 	xpcom/threads/nsTimerImpl.cpp:520
17 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:631
18 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:110
19 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:201
20 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:175
etc...
Comment 1 alexander :surkov 2011-08-29 06:39:17 PDT
Am I reading the stack right that mNodeInfo of nsINode is null? If so then how that could be possible?
Comment 2 Olli Pettay [:smaug] 2011-08-29 06:46:46 PDT
Looks like aNode is null, since 0x8 is offset from null.
Comment 3 alexander :surkov 2011-08-29 06:50:33 PDT
(In reply to Olli Pettay [:smaug] from comment #2)
> Looks like aNode is null, since 0x8 is offset from null.

Can you think of example when nsIDOMNode can't be queried to nsINode (perhaps except the case when they passed pure JS object that implements nsIDOMNode only)?

> since 0x8 is offset from null.

Could you share please more info how to read this right, maybe a link or few hints?
Comment 4 Olli Pettay [:smaug] 2011-08-29 07:51:59 PDT
(In reply to alexander surkov from comment #3)
> (In reply to Olli Pettay [:smaug] from comment #2)
> > Looks like aNode is null, since 0x8 is offset from null.
> 
> Can you think of example when nsIDOMNode can't be queried to nsINode
No, expect when nsIDOMNode is also null.
But in this case JS is calling a11y service, so perhaps 
GetAccessibleFor is called with {} or using other dummy object.


> 
> Could you share please more info how to read this right, maybe a link or few
> hints?
GetOwnerDoc() is inline method. So the caller takes the address of aNode+offset_to_mNodeInfo to
call mNodeInfo->GetDocument().
Comment 5 alexander :surkov 2011-08-29 08:05:18 PDT
(In reply to Olli Pettay [:smaug] from comment #4)
> (In reply to alexander surkov from comment #3)
> > (In reply to Olli Pettay [:smaug] from comment #2)
> > > Looks like aNode is null, since 0x8 is offset from null.
> > 
> > Can you think of example when nsIDOMNode can't be queried to nsINode
> No, expect when nsIDOMNode is also null.
> But in this case JS is calling a11y service, so perhaps 
> GetAccessibleFor is called with {} or using other dummy object.

Ok, so it was a reasonable assumption.

> > Could you share please more info how to read this right, maybe a link or few
> > hints?
> GetOwnerDoc() is inline method. So the caller takes the address of
> aNode+offset_to_mNodeInfo to
> call mNodeInfo->GetDocument().

offset is 0x8, mNodeInfo is first member, nsINode is inherited from two classes having virtual functions, so are first two bytes of the object are spend for vt?
Comment 6 Olli Pettay [:smaug] 2011-08-29 08:08:38 PDT
Something like that. In this kind of cases I don't usually count exactly the address, but
since it is very close to 0 anyway, it is a strong hint that the crash is actually a null
pointer crash.
And based on the code, passing JS object to the method does cause this kind of crash.
Comment 7 alexander :surkov 2011-08-29 09:05:26 PDT
Created attachment 556567 [details] [diff] [review]
patch
Comment 8 Trevor Saunders (:tbsaunde) 2011-08-29 12:05:55 PDT
(In reply to alexander surkov from comment #5)
> (In reply to Olli Pettay [:smaug] from comment #4)
> > (In reply to alexander surkov from comment #3)
> > > (In reply to Olli Pettay [:smaug] from comment #2)
> > > > Looks like aNode is null, since 0x8 is offset from null.
> > > 
> > > Can you think of example when nsIDOMNode can't be queried to nsINode
> > No, expect when nsIDOMNode is also null.
> > But in this case JS is calling a11y service, so perhaps 
> > GetAccessibleFor is called with {} or using other dummy object.

Such a dummy object would have to implement nsIDOMNode right?

> Ok, so it was a reasonable assumption.

Do we really want to support js implementations of nsIDOMNode? this seems unfortunate
Comment 9 alexander :surkov 2011-08-29 20:11:48 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #8)

> > Ok, so it was a reasonable assumption.
> 
> Do we really want to support js implementations of nsIDOMNode? this seems
> unfortunate

We don't want and we don't do.
Comment 10 Trevor Saunders (:tbsaunde) 2011-08-29 20:20:48 PDT
(In reply to alexander surkov from comment #9)
> (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> 
> > > Ok, so it was a reasonable assumption.
> > 
> > Do we really want to support js implementations of nsIDOMNode? this seems
> > unfortunate
> 
> We don't want and we don't do.

Then why should we make sure to not crash if its done?  I mean as opposed to asking whatever adon is doing this to get fixed, and calling this bug wontfix?
Comment 11 alexander :surkov 2011-08-29 20:27:04 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #10)

> > We don't want and we don't do.
> 
> Then why should we make sure to not crash if its done?  I mean as opposed to
> asking whatever adon is doing this to get fixed, and calling this bug
> wontfix?

By saying we don't support that I meant we should return null accessible in this case. Addons is something that is out of our control and if addon call into us make us crash then this should be resolved on our side.
Comment 12 Trevor Saunders (:tbsaunde) 2011-08-30 21:13:13 PDT
(In reply to alexander surkov from comment #11)
> (In reply to Trevor Saunders (:tbsaunde) from comment #10)
> 
> > > We don't want and we don't do.
> > 
> > Then why should we make sure to not crash if its done?  I mean as opposed to
> > asking whatever adon is doing this to get fixed, and calling this bug
> > wontfix?
> 
> By saying we don't support that I meant we should return null accessible in
> this case. Addons is something that is out of our control and if addon call
> into us make us crash then this should be resolved on our side.

I thought it was acceptable for trusted  js to be able to  cause a crash if it did something crazy like this?
Comment 13 Trevor Saunders (:tbsaunde) 2011-08-30 21:24:49 PDT
Comment on attachment 556567 [details] [diff] [review]
patch

can we just prevent js from implementing nsIDOMNode? is there a legitimate reason for it to be able to?

(I'm still not thrilled with supporting this case, but if we need to do it then this is correct modulo my nits below)

>   nsCOMPtr<nsINode> node(do_QueryInterface(aNode));
>+  if (!node)
>+    return NS_OK;

I'd claim this is an error, and we should return invalid arg or unexpected or plain failure.

>+
>   NS_IF_ADDREF(*aAccessible = GetAccessible(node));

If you really want to return ok if the qi fails why not do if (node) NS_ADDREF(blah);

>   nsCOMPtr<nsINode> node(do_QueryInterface(aNode));
>+  if (!node)
>+    return NS_OK;

same, I'd return error here.
Comment 14 alexander :surkov 2011-08-30 21:33:52 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #12)

> I thought it was acceptable for trusted  js to be able to  cause a crash if
> it did something crazy like this?

crash is never acceptable, returning a failure (exception in js) is right thing here as you suggested in comment #13.
Comment 15 alexander :surkov 2011-08-30 21:35:50 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #13)
> Comment on attachment 556567 [details] [diff] [review]
> patch
> 
> can we just prevent js from implementing nsIDOMNode? is there a legitimate
> reason for it to be able to?

JS can implement XPCOM interfaces and that's the whole idea of JS communication with c++ part. That's how, for example, XUL trees work.
Comment 16 alexander :surkov 2011-09-01 00:34:38 PDT
inbound land http://hg.mozilla.org/integration/mozilla-inbound/rev/4e3d2bb73029
Comment 17 Ed Morley [:emorley] 2011-09-01 13:52:53 PDT
http://hg.mozilla.org/mozilla-central/rev/4e3d2bb73029

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