"ASSERTION: QueryInterface needed" with web audio memory report

RESOLVED FIXED in Firefox 45

Status

()

Core
Web Audio
P1
normal
Rank:
15
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Jesse Ruderman, Assigned: karlt)

Tracking

(Blocks: 1 bug)

Trunk
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8683890 [details]
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.)
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
Summary: "ASSERTION: QueryInterface needed" with web audio memory report → "ASSERTION: QueryInterface needed" when reporting memory
(Reporter)

Comment 2

2 years ago
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".
(Reporter)

Updated

2 years ago
Blocks: 875414
> 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.
Component: XPCOM → Audio/Video
Summary: "ASSERTION: QueryInterface needed" when reporting memory → "ASSERTION: QueryInterface needed" with web audio memory report
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.
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!
Rank: 15
Component: Audio/Video → Web Audio
Priority: -- → P1
(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?
(Assignee)

Comment 7

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

Need NS_INTERFACE_MAP_ENTRY(nsIMemoryReporterManager), I expect.
Whiteboard: [enable test_WebAudioMemoryReporting.html when fixed]
(Reporter)

Comment 8

2 years ago
I split out bug 1223177 for improving the assertion.
(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.
(Assignee)

Comment 10

2 years ago
Created attachment 8687737 [details] [diff] [review]
implement query interface to nsIMemoryReporter
Attachment #8687737 - Flags: review?(bzbarsky)
(Assignee)

Updated

2 years ago
Assignee: nobody → karlt
Status: NEW → ASSIGNED
Comment on attachment 8687737 [details] [diff] [review]
implement query interface to nsIMemoryReporter

r=me
Attachment #8687737 - Flags: review?(bzbarsky) → review+

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eb70b5be4f91
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Assignee)

Updated

2 years ago
Whiteboard: [enable test_WebAudioMemoryReporting.html when fixed]
You need to log in before you can comment on or make changes to this bug.