Last Comment Bug 770737 - Make JSContext::global() return Handle<GlobalObject*>
: Make JSContext::global() return Handle<GlobalObject*>
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla16
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-03 16:35 PDT by Jason Orendorff [:jorendorff]
Modified: 2012-07-11 09:32 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (71 bytes, patch)
2012-07-10 16:05 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
patch (2.96 KB, patch)
2012-07-10 16:06 PDT, Luke Wagner [:luke]
wmccloskey: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2012-07-03 16:35:18 PDT
Suggested by Luke in bug 725909 comment 6.
Comment 1 Jason Orendorff [:jorendorff] 2012-07-03 17:57:02 PDT
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?
Comment 2 Bill McCloskey (:billm) 2012-07-03 18:08:16 PDT
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?
Comment 3 Luke Wagner [:luke] 2012-07-03 18:15:43 PDT
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*>.
Comment 4 Bill McCloskey (:billm) 2012-07-03 21:41:13 PDT
I just realized that toObject() thing wouldn't work anyway :-(.
Comment 5 Luke Wagner [:luke] 2012-07-03 22:39:19 PDT
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.
Comment 6 Jason Orendorff [:jorendorff] 2012-07-06 09:48:22 PDT
(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.
Comment 7 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-10 14:32:40 PDT
(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
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-10 15:07:01 PDT
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!  :-)
Comment 9 Luke Wagner [:luke] 2012-07-10 15:19:42 PDT
(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.
Comment 10 Bill McCloskey (:billm) 2012-07-10 15:29:55 PDT
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.
Comment 11 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-10 15:36:32 PDT
Gah! Please ignore me. Need more sleep.
Comment 12 Luke Wagner [:luke] 2012-07-10 16:05:31 PDT
Created attachment 640839 [details] [diff] [review]
patch
Comment 13 Luke Wagner [:luke] 2012-07-10 16:06:59 PDT
Created attachment 640841 [details] [diff] [review]
patch

qref'd
Comment 15 Ed Morley [:emorley] 2012-07-11 09:32:18 PDT
https://hg.mozilla.org/mozilla-central/rev/48b60be9e37f

Note You need to log in before you can comment on or make changes to this bug.