The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jorendorff, Assigned: luke)

Tracking

Other Branch
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Suggested by Luke in bug 725909 comment 6.
(Reporter)

Comment 1

5 years ago
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?
(Assignee)

Comment 3

5 years ago
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 :-(.
(Assignee)

Comment 5

5 years ago
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.
(Reporter)

Comment 6

5 years ago
(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!  :-)
(Assignee)

Comment 9

5 years ago
(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.
(Assignee)

Updated

5 years ago
Summary: Make JSObject::global() return Handle<GlobalObject*> → Make JSContext::global() return Handle<GlobalObject*>
(Assignee)

Comment 12

5 years ago
Created attachment 640839 [details] [diff] [review]
patch
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #640839 - Flags: review?(wmccloskey)
(Assignee)

Comment 13

5 years ago
Created attachment 640841 [details] [diff] [review]
patch

qref'd
Attachment #640839 - Attachment is obsolete: true
Attachment #640839 - Flags: review?(wmccloskey)
Attachment #640841 - Flags: review?(wmccloskey)
Attachment #640841 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/48b60be9e37f
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/48b60be9e37f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.