Closed Bug 1157829 Opened 6 years ago Closed 6 years ago

Replace markAndScanString/Symbol with traverse


(Core :: JavaScript: GC, defect)

Not set



Tracking Status
firefox40 --- fixed


(Reporter: terrence, Assigned: terrence)


(Blocks 2 open bugs)



(1 file)

These two bizarrely custom methods are now identical to traverse + one assertion, so we can just do that now.
Attachment #8596711 - Flags: review?(sphink)
Comment on attachment 8596711 [details] [diff] [review]

Review of attachment 8596711 [details] [diff] [review]:

::: js/src/gc/Marking.cpp
@@ +583,5 @@
>  } // namespace js
>  template <typename T>
> +void
> +GCMarker::traverseOwnedEdge(JSObject* source, T* target)

I know I'm dense, but what does "owned edge" mean here? I mean, I know about regular edges, and things like owned shapes, but I don't quite understand what it means here. Should it be traverseSameZoneEdge? (Though I guess that doesn't cover the atom case.) traverseCheckedEdge? Or just traverseChildEdge? (If this "owned" is different from shapes' "owned", I'd really rather not use the same term.)

No change necessary, really, just an explanation in the bug. Though if there *is* a useful comment to be made (or just a different function name), that would be good.
Attachment #8596711 - Flags: review?(sphink) → review+
Huh, I just meant "owned" in the traditional dictionary sense: e.g. belonging to. Normal traversals just specify the target. The callers I'm replacing here also pass the gcthing (really only objects) that logically contains the edge to the thing being traversed. Ideally we'd use this sort of traverse(owner, target) interface everywhere and make comprehensive cross-compartment edge checks on all edges, but that would be mildly inconvenient from most places so we currently only do it in the one place where we still have a stack reference to the owner: processMarkStackTop. Actually, now that I'm reflecting on it, that last sentence was a total lie. All the places we eagerlyMarkChildren we could trivially switch over to this interface. Also, the name is wrong. Hurm. :-/

I think it would be worth updating the name and checking this in as-is, as it doesn't change behavior. I'll file a follow-up to see how practical it would be to just make traverse take the source unconditionally.
Actually, I should probably just make the changes inline in the eagerlyMarkChildren changes stack. Will do that instead.
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40