See bug 1182428 comment 12. It seems to be claiming that something has a GC pointer even though it's a Rooted. Possibly because that struct is tagged as holding a GC pointer for another legitimate reason?

Currently testing this patch.

I have a patch series that fixes this. The bug here turned out to be really simple -- when checking whether a type was a GCType or GCPointer, the level of pointeredness was considered, but not when recording the explanations. However, on the way to figuring that out, I uncovered a bunch of other bugs and bogosities, which had surprisingly little impact (mostly it meant that our unnecessary rooting calculations were incomplete.)

I ran across a case where we had a field name 'constructor' somewhere, and when printing out the explanations that confused things because we did a lookup gcFields['constructor'], which returned {}.constructor (the Object constructor, I believe). That caused a crash because the code tries to call .get() on it. So this patch switches to using a Map. Less scary.

This is the fix for the actual bug, where it was recording explanations for things that aren't true.

Comment on attachment 8636875 [details] [diff] [review] Use a Map instead of a plain object to avoid the "constructor" property Review of attachment 8636875 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/devtools/rootAnalysis/computeGCTypes.js @@ +120,5 @@ > } > > + if (!gcFields.has(typeName)) > + gcFields.set(typeName, new Map()); > + gcFields.get(typeName).set(child, [ why, fieldPtrLevel ]); What is fieldPtrLevel? I don't see this defined anywhere.

Comment on attachment 8636876 [details] [diff] [review] Add an explanation only if there is something to explain Review of attachment 8636876 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/devtools/rootAnalysis/computeGCTypes.js @@ +118,5 @@ > gcPointers[typeName] = new Set(); > gcPointers[typeName].add(why); > } > > + if (ptrLevel < 2) { Wait, do you have other patches in your queue before this one? I don't see ptrLevel defined either.

Comment on attachment 8636875 [details] [diff] [review] Use a Map instead of a plain object to avoid the "constructor" property The map changes look fine, but cancelling review until I work out which version of the code this is based on.

Comment on attachment 8636875 [details] [diff] [review] Use a Map instead of a plain object to avoid the "constructor" property Review of attachment 8636875 [details] [diff] [review]: ----------------------------------------------------------------- r+ now I've seen the related patches in bug 1182428. This looks like it's out of order in the patch sequence because it's diffed against a version that still uses the old variable names, but whatever.

(In reply to Jon Coppeard (:jonco) from comment #8) > Comment on attachment 8636875 [details] [diff] [review] > Use a Map instead of a plain object to avoid the "constructor" property > > Review of attachment 8636875 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ now I've seen the related patches in bug 1182428. This looks like it's > out of order in the patch sequence because it's diffed against a version > that still uses the old variable names, but whatever. Oh! You're right, sorry, I messed up the reordering. This is the first patch in my queue, but it will break everything if I land it because it uses the new names. I need to fixup the whole queue. Sorry about that; I thought I had it set up where everything worked after every patch, but I looks like I mixed it up.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b389bcb4baf9 https://hg.mozilla.org/integration/mozilla-inbound/rev/ea1119a311db

https://hg.mozilla.org/mozilla-central/rev/b389bcb4baf9 https://hg.mozilla.org/mozilla-central/rev/ea1119a311db