Closed Bug 1223734 Opened 4 years ago Closed 4 years ago

"Assertion failure: false (Should not have an existing pref cache for this address)" - 'dom.audiochannel.mutedByDefault' - #2

Categories

(Core :: Audio/Video, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: jruderman, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(5 files, 3 obsolete files)

Attached file testcase
This is basically the same testcase as bug 1187204, but with the statements in a different order.


1. Disable e10s
2. Load the testcase
3. Quit Firefox

Result:

Attempt to add a bool pref cache for preference 'dom.audiochannel.mutedByDefault' at address '0x10b4c34c8'was made. However, a pref was already cached at this address.

Assertion failure: false (Should not have an existing pref cache for this address), at /Users/jruderman/trees/mozilla-central/modules/libpref/Preferences.cpp:216
Attached file stack
Attached patch acs.patch (obsolete) — Splinter Review
Or... we should implement a NS_AreWeShuttingdown
Assignee: nobody → amarchesini
Attachment #8686082 - Flags: review?(bugs)
Comment on attachment 8686082 [details] [diff] [review]
acs.patch

So AudioChannelAgent::NotifyStoppedPlaying() will crash with this change based on the stack
Attachment #8686082 - Flags: review?(bugs) → review-
Attached patch acs.patch (obsolete) — Splinter Review
Attachment #8686082 - Attachment is obsolete: true
Attachment #8686150 - Flags: review?(bugs)
Comment on attachment 8686150 [details] [diff] [review]
acs.patch

So nothing guarantees the raw pointers in mSpeakerManager are clearer properly.
Attachment #8686150 - Flags: review?(bugs) → review+
Attached patch acs2.patchSplinter Review
Attachment #8686150 - Attachment is obsolete: true
Attachment #8686165 - Flags: review?(bugs)
Attached patch xpcom.patch (obsolete) — Splinter Review
Attachment #8686166 - Flags: review?(bugs)
Comment on attachment 8686166 [details] [diff] [review]
xpcom.patch

I do like something like this and have long wondered why we don't have this, but Nathan should review this.

+ * Return true if we are shutting down. Means if NS_ShutdownXPCOM has been
+ * called.
isn't quite accurate. gXPCOMShuttingDown is set to true quite late (oddly late, I'd say) during shutdown.
Attachment #8686166 - Flags: review?(nfroyd)
Attachment #8686166 - Flags: review?(bugs)
Attachment #8686166 - Flags: feedback+
Attachment #8686165 - Flags: review?(bugs) → review+
Attached patch xpcom.patchSplinter Review
Attachment #8686166 - Attachment is obsolete: true
Attachment #8686166 - Flags: review?(nfroyd)
Attachment #8686219 - Flags: review?(nfroyd)
Comment on attachment 8686219 [details] [diff] [review]
xpcom.patch

Review of attachment 8686219 [details] [diff] [review]:
-----------------------------------------------------------------

Looks to me like AudioChannelService is already listening for the xpcom-shutdown observer notification, so it should just be setting a flag that says we shouldn't create any more AudioChannelService instances?  That would also avoid the problem of there being a significant lag between xpcom-shutdown and gXPCOMShuttingDown/NS_IsXPCOMShuttingDown() and AudioChannelService being reinstantiated somewhere in the middle of that.
Attachment #8686219 - Flags: review?(nfroyd)
What Nathan says makes sense to me. I land a previous version of this patch, already reviewed by smaug:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b22d1abe0289
We discussed this on IRC a bit today. Jesse's take was that the testcase was likely to manifest as an intermittent failure at best when run as part of our test infrastructure (since we don't have a good story for testing shutdown issues and forcing a GC was enough to make this problem go away locally). And on Try, this testcase is indeed green across the board both before and after the patch in this bug landed.

That said, landing fuzzer testcases gives more fodder for future fuzzing, so I think the pros (as miniscule as they appear to be) outweigh the cons here. What do you think, Andrea?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=574b0aa54e8d&group_state=expanded
Attachment #8688117 - Flags: review?(amarchesini)
Attachment #8688117 - Attachment is patch: true
Comment on attachment 8688117 [details] [diff] [review]
crashtest-ized version of the testcase

Review of attachment 8688117 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with you. Nice to know I can write such tests.
Attachment #8688117 - Flags: review?(amarchesini) → review+
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/b22d1abe0289
Status: NEW → 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.