1108149 - [jsdbg2] Debugger.prototype.findObjects should use JS::ubi::RootList

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

It's currently doing the old-style manual addition of CCW edges and all of that.

It should also do some referent abandoning in the traversal.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: find-objects-use-root-list.patch

Comment 2

[Shu-yu Guo [:shu]]

Comment on attachment 8534064 [details] [diff] [review]
find-objects-use-root-list.patch

Review of attachment 8534064 [details] [diff] [review]:
-----------------------------------------------------------------

This looks much better to me. I'd like to know why the CCWs no longer need to be added though.

Could we add a test case as well, that we're doing filtering properly? Or did that get added with the other bug that did the post-traversal filtering?

::: js/src/vm/Debugger.cpp
@@ +3564,5 @@
> +        // Ensure that all of our debuggee globals are rooted so that they are
> +        // visible in the RootList.
> +        JS::AutoObjectVector debuggees(cx);
> +        for (GlobalObjectSet::Range r = dbg->allDebuggees(); !r.empty(); r.popFront()) {
> +            if (!debuggees.append(static_cast<JSObject *>(r.front())))

Is this static_cast needed?

@@ +3572,2 @@
>          {
> +            Maybe<JS::AutoCheckCannotGC> maybeNoGC;

I'd preserve the old comment about not tolerating moving GC.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Shu-yu Guo [:shu] from comment #2)
> Comment on attachment 8534064 [details] [diff] [review]
> find-objects-use-root-list.patch
> 
> Review of attachment 8534064 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks much better to me. I'd like to know why the CCWs no longer need
> to be added though.

RootList handles incoming CCWs for us: http://dxr.mozilla.org/mozilla-central/source/js/src/vm/UbiNode.cpp#291

> 
> Could we add a test case as well, that we're doing filtering properly? Or
> did that get added with the other bug that did the post-traversal filtering?

We have some test coverage via the fuzzer test cases, but nothing explicit. We also have debug/Memory-takeCensus-{04,05}.js which test that RootList finds GC roots via CCW and objects currently on the stack via a census traversal. If you want, I could exercise RootList the same way here, but via findObjects.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3644e0bac5b6

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3644e0bac5b6