no check for a null pointer when QueryInterfacing

ASSIGNED
Assigned to

Status

()

Core
DOM: Events
ASSIGNED
9 years ago
9 years ago

People

(Reporter: Walter Fettich, Assigned: smaug)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
User-Agent:       Opera/9.27 (Windows NT 5.1; U; en)
Build Identifier: xulrunner-1.9.0.4-source

In file:
\xulrunner-1.9.0.4-source\mozilla\content\events\src\nsEventDispatcher.cpp

function: nsEventDispatcher::DispatchDOMEvent(..)

code:
PRBool trusted;
nsCOMPtr<nsIDOMNSEvent> nsevent(do_QueryInterface(privEvt));
nsevent->GetIsTrusted(&trusted);

There should be a non-null check for nsevent.

Reproducible: Always

Steps to Reproduce:
It's difficult, but the details section already says how to fix it.
There could be an assertion that nsevent isn't null, but why should we add
the null check? It is expected that any event which implements
nsIDOMEvent and nsIPrivateDOMEvent, implements also nsIDOMNSEvent.
(Reporter)

Comment 2

9 years ago
You might expect it. But not everyone who is trying to create a custom event component. It's a problem I ran into when doing this and I had no idea I needed to implement nsIDOMNSEvent.  I found out only after debugging the source using the source server and I thought this check would be necessary. 
There should be _something_ to guard against a null pointer there.
Smaug, I think we do want the null-check here (possibly with an NS_ERROR for good measure).  See the .embedding thread that led to this being filed.

Do you want to patch or review?
Status: UNCONFIRMED → NEW
Ever confirmed: true
In that thread the new event is created so that the event object contains 
inner mPrivateEvent. So that inner event already implements nsIDOMNSEvent.
To make the new class to support that is pretty easy;
make the class to inherit nsIDOMNSEvent,
add nsIDOMNSEvent to the QueryInterface implementation and
NS_FORWARD_NSIDOMNSEVENT(mPrivateEvent->) to the class definition.

I'm pretty sure that some code would break if an event doesn't have .isTrusted
or .originalTarget properties.

But if there is a good reason why the event shouldn't implement nsIDOMNSEvent,
I could sure write the patch.
And I know it would be great if adding new event types was easier.
Currently using an inner event is pretty much the only option.
It is used for example by nsXMLHttpProgressEvent.
Oh, the event in that thread _will_ implement nsIDOMNSEvent.  This bug is just saying that not implementing it should return an error nsresult, possibly with an informative assertion, instead of just crashing...
Created attachment 358416 [details] [diff] [review]
patch

Warning should be enough. We don't want to assert if someone
does eventTarget.dispatchEvent({});
Assignee: nobody → Olli.Pettay
Status: NEW → ASSIGNED
Attachment #358416 - Flags: superreview?(bzbarsky)
Attachment #358416 - Flags: review?(bzbarsky)
Comment on attachment 358416 [details] [diff] [review]
patch

That's fine for privEvent, but asserting for the other is fine: the consumer has implemented nsIPrivateDOMEvent (which JS can't even do), but not nsIDOMNSEvent...  Either way, though.
Attachment #358416 - Flags: superreview?(bzbarsky)
Attachment #358416 - Flags: superreview+
Attachment #358416 - Flags: review?(bzbarsky)
Attachment #358416 - Flags: review+
You need to log in before you can comment on or make changes to this bug.