Closed Bug 1230092 Opened 4 years ago Closed 4 years ago

"ASSERTION: QueryInterface needed" when printing fails

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: jruderman, Assigned: bzbarsky)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(3 files, 1 obsolete file)

Attached file 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.
Attached file stack
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.
Component: DOM → DOM: Events
All the nsIDOM*Event interfaces inherit from nsIDOMEvent.
The printing code should just use nsRefPtr<CustomEvent> event.
(it is still a bit fragile, but hopefully we'll get rid of nsIDOM*Event interfaces at some point)
> 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).
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
In other cases we just seem to static_cast<Event*>() when passing CustomEvent to some dispatcher
> In other cases we just seem to static_cast<Event*>()

Sure.  I can remove that noise if you like, with the IDL change above...
yeah, please. it is just that other nsIDOM*Events inherit nsIDOMEvent, so making a special case there feels odd.
> 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.
Attachment #8695340 - Attachment is obsolete: true
Attachment #8695340 - Flags: review?(bugs)
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
Attachment #8695349 - Flags: review?(bugs) → review+
Depends on: 1230216
> 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?
> 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.  ;)
https://hg.mozilla.org/mozilla-central/rev/ac9aa8dab115
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.