Closed Bug 474992 Opened 14 years ago Closed 3 months ago

no check for a null pointer when QueryInterfacing


(Core :: DOM: Events, defect)

Windows XP
Not set





(Reporter: wfettich, Assigned: smaug)



(1 file)

User-Agent:       Opera/9.27 (Windows NT 5.1; U; en)
Build Identifier: xulrunner-

In file:

function: nsEventDispatcher::DispatchDOMEvent(..)

PRBool trusted;
nsCOMPtr<nsIDOMNSEvent> nsevent(do_QueryInterface(privEvt));

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.
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?
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...
Attached patch patchSplinter Review
Warning should be enough. We don't want to assert if someone
does eventTarget.dispatchEvent({});
Assignee: nobody → Olli.Pettay
Attachment #358416 - Flags: superreview?(bzbarsky)
Attachment #358416 - Flags: review?(bzbarsky)
Comment on attachment 358416 [details] [diff] [review]

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+

There is no nsIDOMNSEvent or nsIPrivateDOMEvent anymore.

Closed: 3 months ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.