1157829 - Replace markAndScanString/Symbol with traverse

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: 10.0.2_replace_markAndScanFoo-v0.diff

These two bizarrely custom methods are now identical to traverse + one assertion, so we can just do that now.

Comment 1

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8596711 [details] [diff] [review]
10.0.2_replace_markAndScanFoo-v0.diff

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.

Comment 2

[Terrence Cole [:terrence]]

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.

Comment 3

[Terrence Cole [:terrence]]

Actually, I should probably just make the changes inline in the eagerlyMarkChildren changes stack. Will do that instead.

Comment 4

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d4c64043ab2

Comment 5

[Pulsebot]

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

Comment 6

[Carsten Book [:Tomcat]]

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