793382 - crash in mozilla::dom::DOMRequest::Init

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

This bug was filed from the Socorro interface and is 
report bp-95b84569-d652-48a3-92ed-fed222120922 .
============================================================= 
0 	xul.dll 	mozilla::dom::DOMRequest::Init 	dom/base/DOMRequest.cpp:42
1 	xul.dll 	mozilla::dom::DOMRequest::DOMRequest 	dom/base/DOMRequest.cpp:25
2 	xul.dll 	mozilla::dom::DOMRequestService::CreateRequest 	dom/base/DOMRequest.cpp:201
3 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:70
4 	xul.dll 	XPCWrappedNative::CallMethod 	js/xpconnect/src/XPCWrappedNative.cpp:2405
5 	xul.dll 	XPC_WN_CallMethod 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1469
6 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:367
7 	mozjs.dll 	js::Invoke 	js/src/jsinterp.h:109
8 	mozjs.dll 	js_fun_apply 	js/src/jsfun.cpp:951
9 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:367
10 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:2454

I get this crash with SpecialPowers.getDOMRequestService().createRequest({});

Comment 1

[Josh Matthews [:jdm]]

We don't check the return value of do_QueryInterface in DOMRequest::Init. We should probably just return early.

Comment 2

[Ting-Yuan Huang]

Hi Matthews and Wargers,

I'm new to Mozilla and interested in this problem. I'm trying to contribute and need your help.

In this bug, the argument is invalid in the test case; createRequest() expects a window while the actual argument is an empty object. As noted by Matthews, this eventually results in dereferencing a null pointer. Since createRequest() is not exposed as a public API, should this be a fatal condition? If so, should we catch this kind of problems as early as possible? Maybe all we have to do is to add an assertion to shutdown immediately and gracefully.

Propagate the error to the JavaScript side is also possible but I'm afraid that the exceptions might sometimes be ignored and result in delayed bombs.

Would you please give me some advices? Thanks in advance!

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We could "fix" this crash by doing what you say and throw an exception if createRequest is passed something that isn't a valid Window. However that wouldn't actually fix the bug here since the code still wouldn't function as intended, right?

So detecting the lack of a valid Window wouldn't actually fix the bug, just wallpaper over the crash.

Precisely because this isn't a public function do we rather crash than wallpaper over the problem. This way we can detect this bug through things like Socorro.

So what we should do here is to find the code which is calling createRequest without providing a valid window and then fix it to find a proper window.

Comment 4

[Ting-Yuan Huang]

Hi Sicking,

I totally agree with you. I'm sorry that I may not describe it clearly. The problem here is that in the test case there is only one line (of JavaScript):

SpecialPowers.getDOMRequestService().createRequest({});

The argument in question, {}, is hard-coded, given by the test case directly. So it may be arguable that the test case itself is legal or not.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That code looks totally wrong to me. If the only way to reproduce this crash is using that testcase then this bug should be closed as INVALID.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Making it so. The code in comment 0 is buggy, you need to provide a valid window.