1222202 - "ASSERTION: QueryInterface needed" with web audio memory report

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P1)

Description

[Jesse Ruderman]

Attachment: stack

Following the instructions in bug 1221855 triggers:

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

(It also crashes, but that's the subject of bug 1221855. erahm said I should split this assertion into its own bug in the XPCOM component.)

Comment 1

[Eric Rahm [:erahm]]

The assertion is coming from nsIMemoryReporterManager itself [1], odds are this happens with all memory reports, not just web audio.

[1] https://dxr.mozilla.org/mozilla-central/rev/61dcc13d0848230382d5c85cdcf6721a05ee37c6/xpcom/base/nsMemoryReporterManager.cpp#1578

Comment 2

[Jesse Ruderman]

If I just launch the browser and get a memory report, I don't hit the assertion. But this bug could still be more general than "weird web audio testcase".

Comment 3

[Boris Zbarsky [:bzbarsky]]

> The assertion is coming from nsIMemoryReporterManager itself [1]

Well, it's coming from the assignment to the nsCOMPtr.  The bug is in the relevant nsIMemoryReporter implementation.  That implementation, if you pull this up in a debugger, is mozilla::dom::AudioContext.

And indeed, AudioContext inherits from nsIMemoryReporter but does not QI to it; it's QI impl is all of:

  NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(AudioContext)
  NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper)

All the assertion is helpfully telling you is that your QI impl is broken and you should fix it.

Comment 4

[Andrew McCreight [:mccr8]]

I talked with erahm about this yesterday. He said there were a few classes like this.

Also, this assertion should really be fatal, and have a more obvious failure message.

Comment 5

[Maire Reavy [:mreavy]]

Paul -- Can I ask you to take this?  I believe this will be a very quick/easy patch for you, but I know you're busy.  So if you need me to find someone else, let me know.  Thanks!

Comment 6

[Eric Rahm [:erahm]]

(In reply to Boris Zbarsky [:bz] from comment #3)

bz, thank you for the help tracking this down and clarifying.

> > The assertion is coming from nsIMemoryReporterManager itself [1]
> 
> Well, it's coming from the assignment to the nsCOMPtr.  The bug is in the
> relevant nsIMemoryReporter implementation.  That implementation, if you pull
> this up in a debugger, is mozilla::dom::AudioContext.

Ah yes, we just weren't seeing this before because we weren't taking a strong ref. I assumed I just did something wrong.

> And indeed, AudioContext inherits from nsIMemoryReporter but does not QI to
> it; it's QI impl is all of:
> 
>   NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(AudioContext)
>   NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper)
> 
> All the assertion is helpfully telling you is that your QI impl is broken
> and you should fix it.

Helpful is relative here. I suppose it just comes down to how much institutional knowledge you have:
1) You have to know what QueryInterface means
2) You have to know that "QueryInterface needed" means "You are missing an interface entry for the interface you are currently trying to use. Go to the concrete implementation's NS_IMPL_ISUPPORTS* macro and add it"
3) As mccr8 mentioned it's non-fatal, so like all non-fatal log output it's generally ignored

As a follow up maybe would could make this message more explicit (detailing how to fix the problem rather than stating there is a problem) and make it a fatal assertion. Or we could create a MDN page with some details about what to do when you see this particular assertion.

* Even here, it probably needs to be NS_IMPL_ISUPPORTS_INHERITED, right? And do we have docs on when that is needed?

Comment 7

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d2ceb469ea6

Need NS_INTERFACE_MAP_ENTRY(nsIMemoryReporterManager), I expect.

Comment 8

[Jesse Ruderman]

I split out bug 1223177 for improving the assertion.

Comment 9

[Boris Zbarsky [:bzbarsky]]

(Sorry for the lag, wasn't actually cced here)

> Helpful is relative here.

Sure.  I was being semi-sarcastic.  I fully support making this assertion message more informative.

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: implement query interface to nsIMemoryReporter

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8687737 [details] [diff] [review]
implement query interface to nsIMemoryReporter

r=me

Comment 12

[Pulsebot]

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

Comment 13

[Carsten Book [:Tomcat]]

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