Closed Bug 767942 Opened 12 years ago Closed 3 years ago

Add public API to pull the global off a realm

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bholley, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [js:t])

Attachments

(1 obsolete file)

Bug 755186 landed, so we can do this now. I don't see a bug for it anywhere, so here we go. Patch coming right up.
Attachment #636281 - Flags: review?(luke)
No longer blocks: 767938
IIUC, JS_GetGlobalForCompartment should return the same thing as JS_GetGlobalForScopeChain (or is there some pathological case I'm missing)?  Now that the global isn't derived from the scope chain (with cx->globalObject fallback sadness), perhaps we could just rename JS_GetGlobalForScopeChain to JS_GetCurrentGlobal and have it return cx->compartment->global()?
I was thinking when I read this that you could get a global object from a compartment without being in it.  But that's obviously kinda nonsensy and semi-racy and all that.  JS_GetCurrentGlobal is a good idea, and nicely implies something about the current program state must be so for it to work.
Whiteboard: [js:t]
(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #3)
> I was thinking when I read this that you could get a global object from a
> compartment without being in it.  But that's obviously kinda nonsensy and
> semi-racy and all that.

Why, exactly?
If you're outside the compartment, you have to enter it and all that to coordinate with GC and such.  And you have to make the global object returned be a wrapper and all that.  Which, come to think of it, this doesn't do.  There are probably more reasons that'd come to mind if I thought about it more.
FWIW, in the patch, your declaration and definition don't match.
Comment on attachment 636281 [details] [diff] [review]
Add JS_GetGlobalForCompartment. v1

Ok, it sounds like we want to do JS_GetCurrentGlobal. I'm obsoleting this patch in the mean time, and will write that patch when I need it again (so that I can actually link against it and detect stupid mismatches - doh!). Anyone else who needs it is welcome to add the patch in the mean time.
Attachment #636281 - Attachment is obsolete: true
Attachment #636281 - Flags: review?(luke)
(In reply to Luke Wagner [:luke] from comment #2)
> IIUC, JS_GetGlobalForCompartment should return the same thing as
> JS_GetGlobalForScopeChain

Compare the signatures:

  JS_GetGlobalForCompartment(JSContext *cx, JSCompartment *c)
  JS_GetGlobalForScopeChain(JSContext *cx)

For my use case (bug 687724) it's crucial that I can pass in the JSCompartment*, because it's not the same as cx->compartment -- I just fake up a temporary JSContext.  I'm using IterateCompartmentCellsArenas while doing this, which sets rt->gcRunning, so maybe the races Waldo mentioned in comment 5 won't happen?
Hmm.  Alternatively, I could enter each compartment before I do my things with it;  that way, JS_GetCurrentGlobal() would suffice.  That would have the advantage of not having to expose a dangerous API function.  

One downside would be that JSAutoCompartment::enter() has an AssertNoGC() which will be violated because I'm doing this within IterateCompartmentsArenasCells().  That could be worked around with some effort, e.g. by making it "assert we're not in a GC, but being within a GC heap iterator is ok".

Another downside would be that having to enter a compartment in order to do anything with it is a bit naff when you're within a *compartment iterator*.  Maybe IterateCompartmentsArenasCells() could do the entering itself, but then it would need a JSContext* and it currently gets a JSRuntime*, so we've got the rt/cx impedance mismatch problem again.  I guess I could just change it to require a JSContext*.
I'm not working on this.
Assignee: bobbyholley+bmo → general
Assignee: general → nobody
This is probably wontfix as filed, given multiple-globals-per-compartment, but does this still make sense with s/Compartment/Realm/?
Flags: needinfo?(jdemooij)
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #11)
> This is probably wontfix as filed, given multiple-globals-per-compartment,
> but does this still make sense with s/Compartment/Realm/?

I guess we could still add it if we have a use for it. In practice I think API consumers usually have an object from the realm and get that object's global.

Internally we have Realm::maybeGlobal() because the global can be nullptr briefly during GC:

https://searchfox.org/mozilla-central/rev/0b8ed772d24605d7cb44c1af6d59e4ca023bd5f5/js/src/vm/Realm.h#520

If we add a JS::GetGlobalForRealm(realm) API we can probably just assert non-null instead.
Flags: needinfo?(jdemooij)
Summary: Add public API to pull the global off a compartment → Add public API to pull the global off a realm
Blocks: jsapi

We have a JS::GetRealmGlobalOrNull API now.

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED

JS::GetRealmGlobalOrNull was introduced in bug 1363200.

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

Attachment

General

Created:
Updated:
Size: