1189122 - Assert if Suspect is run when mScanInProgress.

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight (out of office until 8/21) [:mccr8]]

nsCycleCollector::Suspect says: "Re-entering ::Suspect during collection used to be a fatal error, but we are canonicalizing nsISupports pointers using QI, so we will see some spurious refcount traffic here." However, the QI to the magic cycle collector values doesn't AddRef, so we may be able to ban this entirely.

One instance of this I know of causes a very difficult to analyze leak, bug 1074416. This leak didn't even show up in the XPCOM leak detector because the refcount of the leaking object ended up at zero, it just doesn't get deleted.

Comment 1

[Andrew McCreight (out of office until 8/21) [:mccr8]]

This can cause leaks.

Comment 2

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: Assert when we Suspect() when a CC scan is in progress.

This can cause leaks that are invisible to our XPCOM leak detection system.

To avoid this, classes should not addref or release in their Traverse methods.

With the patch in the blocking bug, try is green. If it doesn't cause intermittent issues, I'll make it a release assert. I can't actually think of a reason why it is bad to call Suspect except when we're actually iterating over the purple buffer, but I guess it doesn't hurt to be more restrictive.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=992761a5cdc3

Comment 3

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

Comment on attachment 8640884 [details] [diff] [review]
Assert when we Suspect() when a CC scan is in progress.

I think more restrictive is a good thing here.
If you need to do some extra Addref/Release during scan, you're probably doing something wrong, as bug 1074416 showed us.

Comment 4

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Bug 1074416 has merged around to inbound now, so this should be okay to land.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b0596910897

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/2b0596910897

Comment 7

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Comment on attachment 8640884 [details] [diff] [review]
Assert when we Suspect() when a CC scan is in progress.

Approval Request Comment
[Feature/regressing bug #]: old
[User impact if declined]: none, the goal is to verify that bug 1074416 is actually working
[Describe test coverage new/current, TreeHerder]: there is lots of existing testing of this code.
[Risks and why]: very low, this is a debug-only change
[String/UUID change made/needed]: none

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8640884 [details] [diff] [review]
Assert when we Suspect() when a CC scan is in progress.

Given that this fix helps verify bug 1074416, let's uplift to Aurora.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6db8beae5355