1230092 - "ASSERTION: QueryInterface needed" when printing fails

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: QueryInterface needed: 'query_result.get() == mRawPtr', file nsCOMPtr.h, line 415

Based on the stack, I think there are two things going wrong here:

1) When there's a <canvas height="0">, printing fails.
2) When printing fails, "ASSERTION: QueryInterface needed".

This bug is about #2. I'll file a bug on #1 once this is fixed.

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Boris Zbarsky [:bzbarsky]]

The printing code is creating an event via NS_NewDOMCustomEvent.  This asserts when assigned to nsIDOMEvent.

The reason for that is that CustomEvent multiply-inherits from nsIDOMEvent.  One chain goes through nsIDOMCustomEvent.  The other chain goes through Event.

The QI impl for CustomEvent delegates everything except nsIDOMCustomEvent to Event.  So the canonical nsIDOMEvent for a CustomEvent is the one Event inherits from.  But the printing code directly passes its nsIDOMCustomEvent to AsyncEventDispatcher, which effectively static_casts it to the non-canonical nsIDOMEvent.

We should either make nsIDOMCustomEvent not inherit from nsIDOMEvent or ... something.  Changing the QI impl of CustomEvent would help with this case, but not with a case where someone else has an Event and assigns it to nsIDOMEvent and that Event turns out to be a CustomEvent.

Comment 3

[Olli Pettay [:smaug]]

All the nsIDOM*Event interfaces inherit from nsIDOMEvent.
The printing code should just use nsRefPtr<CustomEvent> event.

Comment 4

[Olli Pettay [:smaug]]

(it is still a bit fragile, but hopefully we'll get rid of nsIDOM*Event interfaces at some point)

Comment 5

[Boris Zbarsky [:bzbarsky]]

> The printing code should just use nsRefPtr<CustomEvent> event.

Well, plus explicit casting to nsIDOMEvent via Event or explicit QI to nsIDOMEvent, right?  I have a patch that just nixes the inheritance (and uses CustomEvent directly in the print code).

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Make nsIDOMCustomEvent no longer inherit from nsIDOMEvent, so that CustomEvent doesn't end up with multiple inheritance from nsIDOMEvent

Comment 7

[Olli Pettay [:smaug]]

In other cases we just seem to static_cast<Event*>() when passing CustomEvent to some dispatcher

Comment 8

[Boris Zbarsky [:bzbarsky]]

> In other cases we just seem to static_cast<Event*>()

Sure.  I can remove that noise if you like, with the IDL change above...

Comment 9

[Olli Pettay [:smaug]]

yeah, please. it is just that other nsIDOM*Events inherit nsIDOMEvent, so making a special case there feels odd.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> it is just that other nsIDOM*Events inherit nsIDOMEvent

We should, imo, fix that too, but some of the others might take a bit more work.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Make nsIDOMCustomEvent no longer inherit from nsIDOMEvent, so that CustomEvent doesn't end up with multiple inheritance from nsIDOMEvent

Comment 12

[Olli Pettay [:smaug]]

Comment on attachment 8695349 [details] [diff] [review]
Make nsIDOMCustomEvent no longer inherit from nsIDOMEvent, so that CustomEvent doesn't end up with multiple inheritance from nsIDOMEvent

ok, fine. I'll file a followup to clean up other interfaces or get rid of nsIDOM*Events

Comment 13

[Jesse Ruderman]

> The reason for that is that CustomEvent multiply-inherits from nsIDOMEvent.

Can we catch these bugs earlier? Or show a more precise error message than "QueryInterface needed"? Is it always a bug when a class multiply-inherits from an XPIDL or WebIDL interface?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Can we catch these bugs earlier?

Not very easily.

> Or show a more precise error message than "QueryInterface needed"?

Well.  We can show the type of interface involved, but you can see that anyway from the nsCOMPtr<whatever> listed in the stack.  Showing the concrete type of the thing we got assigned is hard without RTTI, right?

> Is it always a bug when a class multiply-inherits from an XPIDL 

No. In fact, it's quite common with nsISupports, for example (though nsCOMPtr<nsISupports> is, I believe, specialized to not assert this way).  In most other cases it's not great, but also pretty hard to avoid for some of the old nsIDOM* APIs.  The right answer is to get rid of all those APIs.

> or WebIDL interface?

There is no concept of a C++ class inheriting from a WebIDL interface.  ;)

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac9aa8dab115

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ac9aa8dab115