Closed Bug 1184199 Opened 9 years ago Closed 9 years ago

GC types explanations contain Rooted types

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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?
Assignee: nobody → jcoppeard
Attached patch poss-hazard-analysis-fix (obsolete) — Splinter Review
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.
Attachment #8636875 - Flags: review?(jcoppeard)
Assignee: jcoppeard → sphink
Status: NEW → ASSIGNED
This is the fix for the actual bug, where it was recording explanations for things that aren't true.
Attachment #8636876 - Flags: review?(jcoppeard)
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.
Attachment #8636875 - Flags: review?(jcoppeard) → review+
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.
Attachment #8636876 - Flags: review?(jcoppeard)
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.
Attachment #8636875 - Flags: review+
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.
Attachment #8636875 - Flags: review+
Attachment #8636876 - Flags: review+
Attachment #8636653 - Attachment is obsolete: true
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: