474992 - no check for a null pointer when QueryInterfacing

Status: RESOLVED INVALID

(Core :: DOM: Events, defect)

Description

[Walter Fettich]

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.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 2

[Walter Fettich]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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...

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Warning should be enough. We don't want to assert if someone
does eventTarget.dispatchEvent({});

Comment 8

[Boris Zbarsky [: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.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

There is no nsIDOMNSEvent or nsIPrivateDOMEvent anymore.