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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jesse Ruderman]

Attachment: 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

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Andrea Marchesini [:baku]]

Attachment: acs.patch

Or... we should implement a NS_AreWeShuttingdown

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8686082 [details] [diff] [review]
acs.patch

So AudioChannelAgent::NotifyStoppedPlaying() will crash with this change based on the stack

Comment 4

[Andrea Marchesini [:baku]]

Attachment: acs.patch

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8686150 [details] [diff] [review]
acs.patch

So nothing guarantees the raw pointers in mSpeakerManager are clearer properly.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: acs2.patch

Comment 7

[Andrea Marchesini [:baku]]

Attachment: xpcom.patch

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: xpcom.patch

Comment 10

[Nathan Froyd [:froydnj]]

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.

Comment 11

[Andrea Marchesini [:baku]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Attachment: crashtest-ized version of the testcase

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

Comment 13

[Andrea Marchesini [:baku]]

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.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/806939abdd25

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/806939abdd25