Closed Bug 1102541 Opened 8 years ago Closed 8 years ago

JS::ubi::RootList should have a method for manually adding ubi::Nodes as roots

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(1 file, 2 obsolete files)

If some ubi::Node isn't technically a GC root (like a debuggee global), we might want to pretend it is anyways, and it would be nice if RootList had an API to support that.
Depends on: 1108149
Attached patch add-root-to-root-list.patch (obsolete) — Splinter Review
Attachment #8534542 - Flags: review?(shu)
Comment on attachment 8534542 [details] [diff] [review]
add-root-to-root-list.patch

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

More straightforward, nice.

::: js/src/vm/Debugger.cpp
@@ +3575,5 @@
> +            /*
> +             * Ensure that each of our debuggee globals are in the root list.
> +             */
> +            for (GlobalObjectSet::Range r = dbg->allDebuggees(); !r.empty(); r.popFront()) {
> +                if (!rootList.addRoot(JS::ubi::Node(static_cast<JSObject *>(r.front()))))

Do you need the static_cast?

@@ +3578,5 @@
> +            for (GlobalObjectSet::Range r = dbg->allDebuggees(); !r.empty(); r.popFront()) {
> +                if (!rootList.addRoot(JS::ubi::Node(static_cast<JSObject *>(r.front()))))
> +                    return false;
> +            }
> +

Hm, why not put this logic into RootList::init(HandleObject)? That already expects a Debugger object and operates on its debuggee zones, so might as well just add the globals. I don't feel that strongly though.

::: js/src/vm/DebuggerMemory.cpp
@@ +779,5 @@
>              return false;
>  
> +        // Add all debuggee globals as roots.
> +        for (GlobalObjectSet::Range r = dbg->allDebuggees(); !r.empty(); r.popFront()) {
> +            if (!rootList.addRoot(JS::ubi::Node(static_cast<JSObject *>(r.front()))))

Ditto on if you need this static_cast.
Attachment #8534542 - Flags: review?(shu) → review+
(In reply to Shu-yu Guo [:shu] from comment #2)
> ::: js/src/vm/Debugger.cpp
> @@ +3575,5 @@
> > +            /*
> > +             * Ensure that each of our debuggee globals are in the root list.
> > +             */
> > +            for (GlobalObjectSet::Range r = dbg->allDebuggees(); !r.empty(); r.popFront()) {
> > +                if (!rootList.addRoot(JS::ubi::Node(static_cast<JSObject *>(r.front()))))
> 
> Do you need the static_cast?

Yup, I get compiler errors without it here and in the other location.

> @@ +3578,5 @@
> > +            for (GlobalObjectSet::Range r = dbg->allDebuggees(); !r.empty(); r.popFront()) {
> > +                if (!rootList.addRoot(JS::ubi::Node(static_cast<JSObject *>(r.front()))))
> > +                    return false;
> > +            }
> > +
> 
> Hm, why not put this logic into RootList::init(HandleObject)? That already
> expects a Debugger object and operates on its debuggee zones, so might as
> well just add the globals. I don't feel that strongly though.

Good idea, will do.
Attached patch add-root-to-root-list.patch (obsolete) — Splinter Review
Updated based on review comments. Carrying over shu's r+.

Tree is closed right now, so can't land it yet.
Attachment #8534542 - Attachment is obsolete: true
Attachment #8535222 - Flags: review+
I don't understand why the compiler on linux doesn't like this...

It is complaining about GlobalObjectSet:

> 12:39:47 INFO - /builds/slave/m-in-lx-d-00000000000000000000/build/src/js/src/vm/UbiNode.cpp: In member function 'bool JS::ubi::RootList::init(JS::HandleObject)':
> 12:39:47 INFO - /builds/slave/m-in-lx-d-00000000000000000000/build/src/js/src/vm/UbiNode.cpp:330:10: error: 'GlobalObjectSet' has not been declared 

But in GlobalObject.h GlobalObjectSet gets defined (http://dxr.mozilla.org/mozilla-central/source/js/src/vm/GlobalObject.h?from=GlobalObjectSet#809):

> typedef HashSet<GlobalObject *, DefaultHasher<GlobalObject *>, SystemAllocPolicy> GlobalObjectSet;

And in UbiNode.cpp we include that header (http://dxr.mozilla.org/mozilla-central/source/js/src/vm/UbiNode.cpp?from=UbiNode.cpp#24):

> #include "vm/GlobalObject.h"

Any idea what's up here, shu?
Flags: needinfo?(nfitzgerald) → needinfo?(shu)
GlobalObjectSet is in the js namespace, so maybe js::GlobalObjectSet?
(In reply to Jan de Mooij [:jandem] from comment #8)
> GlobalObjectSet is in the js namespace, so maybe js::GlobalObjectSet?

Sigh, thanks >_<
Flags: needinfo?(shu)
Updated patch from the green try push.
Attachment #8535222 - Attachment is obsolete: true
Attachment #8537310 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6b9ce705adaf
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.