Closed Bug 770737 Opened 10 years ago Closed 10 years ago

Make JSContext::global() return Handle<GlobalObject*>

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: jorendorff, Assigned: luke)

Details

(Whiteboard: [js:t])

Attachments

(1 file, 1 obsolete file)

Suggested by Luke in bug 725909 comment 6.
Actually I'm not so sure about this anymore. The JSObject and thus its JSCompartment aren't necessarily rooted. Even if they are rooted, the rules on whether I should be able to count on their staying rooted seem unclear to me. What I definitely don't want is to have the C++ type system telling me something is definitely safe when it's only almost always safe.

Bill?
Yeah, I don't think this is safe. It's too bad we can't do something like:

Handle<GlobalObject *>
Rooted<JSObject *>::global()
{ ... }

In general, it would be nice to be able to attach additional methods to specific instantiations of Rooted. For example, it's annoying that we can't use methods like isObject() on RootedValues. And it would be nice if toObject() on a RootedValue could return a HandleObject.

Luke, can you think of a way to do this?
Oops, I made the comment before bug 687724 made me really think about it.

comment 2 looks great.  We can do that with template specialization, unless I'm missing something.  The challenge is to avoid code duplication between Rooted<T> and Rooted<JSObject*>.
I just realized that toObject() thing wouldn't work anyway :-(.
I think JSContext::global() could return a Handle<GlobalObject*>: any active compartment should have its global marked.  Alternatively we could stop ordinary object-creation paths from taking a parent (so that they always used cx->global()) and have the JSAPI paths, where you can specific a parent, just call setParent explicitly.
(In reply to Luke Wagner [:luke] from comment #5)
> I think JSContext::global() could return a Handle<GlobalObject*>: any active
> compartment should have its global marked.  Alternatively we could stop
> ordinary object-creation paths from taking a parent (so that they always
> used cx->global()) and have the JSAPI paths, where you can specific a
> parent, just call setParent explicitly.

I love the alternative idea.
Whiteboard: [js:t]
(In reply to Luke Wagner [:luke] from comment #5)
> I think JSContext::global() could return a Handle<GlobalObject*>: any active
> compartment should have its global marked.

It's not for ensuring that the global object gets marked. It's for ensuring that all pointers to the global object get marked through, so that they can be updated for a moving GC.

  Handle<GlobalObject*> global = cx->global();
  MovingGC();
  global->anything(); // kaboom! global was hiding in an unrooted stack slot
Isn't global.ptr just &cx->compartment->global_?  Why wouldn't that be a root when a request is entered on the compartment/global in question?  Please tell me why my questions are dumb!  :-)
(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #8)
> Isn't global.ptr just &cx->compartment->global_?

That's what I was thinking; that should make any such handle moving-gc-safe.
As long as there's a context with a non-zero request count, then that context's globalObject field will be rooted. And, in a world with moving collection, that will keep the compartment alive, and the global_ field of the compartment will be kept in sync if the global moves. So this should be safe.
Gah! Please ignore me. Need more sleep.
Summary: Make JSObject::global() return Handle<GlobalObject*> → Make JSContext::global() return Handle<GlobalObject*>
Attached patch patch (obsolete) — Splinter Review
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #640839 - Flags: review?(wmccloskey)
Attached patch patchSplinter Review
qref'd
Attachment #640839 - Attachment is obsolete: true
Attachment #640839 - Flags: review?(wmccloskey)
Attachment #640841 - Flags: review?(wmccloskey)
Attachment #640841 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/48b60be9e37f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.