639675 - incomplete enumeration of purple roots in DEBUG_CC code

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12) Gecko/20100101 Firefox/4.0b12
Build Identifier: Trunk

When DEBUG_CC is on, the cycle collector (in nsCyclecollector.cpp) enumerates all of the purple roots, for the purpose of logging.  Before it does that, it skips over all of the roots added by the runtimes using the following code:

while (i++ < purpleStart) {
  queue.GetNext();
}

i and queue are supposed to be advanced in parallel, and both values are use subsequent to this loop, but because of the post-increment i gets incremented one too many times, causing a purple root to be missed.

It can be fixed by changing the loop to:
while (i < purpleStart) {
  queue.GetNext();
  ++i;
}
Also, NS_ASSERTION(queue.IsDone());  could be added after the next loop (after the purple loops have been enumerated) to prevent a regression.

Reproducible: Always

Comment 1

[Andreas Gal :gal]

Cool. Can you patch?

Comment 2

[Andrew McCreight [:mccr8]]

Yes, I just need to grab a clean tree first.  I'm a few weeks out of date.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: fix off by one error in purple object enumeration

Here's a patch for this bug.  I have compiled it (without DEBUG_CC on...), but I haven't tested it yet in an unmodified copy.  It only touches DEBUG_CC code so it shouldn't be a problem.

Comment 4

[Andreas Gal :gal]

Comment on attachment 517613 [details] [diff] [review]
fix off by one error in purple object enumeration

peterv, can you land this for Andrew? (he has no commit access yet)

Comment 5

[Andrew McCreight [:mccr8]]

My first patch isn't quite right because NS_ASSERTION takes two arguments. I'll upload a fixed patch once I make sure it actually compiles without new warnings with DEBUG_CC.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: revised version of the patch (fixed lack of NS_ASSERTION arg)

Revised version of the patch that has proper number of args to NS_ASSERTION.  It compiles both with and without DEBUG_CC without any additional warnings.

Comment 7

[Andreas Gal :gal]

Comment on attachment 517786 [details] [diff] [review]
revised version of the patch (fixed lack of NS_ASSERTION arg)

I will get you access to tracemonkey to land it.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: fix: assert would incorrectly fire when DEBUG_CC is on but pointer logging is off

The queue iterator wasn't being advanced when mParams.mLogPointers wasn't being set, which would lead to the assertion failing in that case if DEBUG_CC was on, if there are any purple nodes.

Comment 9

[Andrew McCreight [:mccr8]]

There's another instance of this in the cycle collector, near the end of BeginCollection.  I should patch that, too I guess.

Comment 10

[Andrew McCreight [:mccr8]]

RIP DEBUG_CC (bug 845441)