682827 - Crash [@ nsINode::GetOwnerDoc]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Martijn Wargers (dead)]

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 (:asurkov)]

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][[e-mail]]]

Looks like aNode is null, since 0x8 is offset from null.

Comment 3

[alexander :surkov (:asurkov)]

(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][[e-mail]]]

(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 (:asurkov)]

(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][[e-mail]]]

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 (:asurkov)]

Attachment: patch

Comment 8

[Trevor Saunders (:tbsaunde)]

(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 (:asurkov)]

(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)]

(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 (:asurkov)]

(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)]

(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)]

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 (:asurkov)]

(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 (:asurkov)]

(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 (:asurkov)]

inbound land http://hg.mozilla.org/integration/mozilla-inbound/rev/4e3d2bb73029

Comment 17

[Ed Morley [:emorley]]

http://hg.mozilla.org/mozilla-central/rev/4e3d2bb73029