Closed
Bug 767942
Opened 12 years ago
Closed 4 years ago
Add public API to pull the global off a realm
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
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.
Reporter | ||
Comment 1•12 years ago
|
||
Attachment #636281 -
Flags: review?(luke)
Comment 2•12 years ago
|
||
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()?
Comment 3•12 years ago
|
||
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.
Updated•12 years ago
|
Whiteboard: [js:t]
Reporter | ||
Comment 4•12 years ago
|
||
(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?
Comment 5•12 years ago
|
||
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.
Comment 6•12 years ago
|
||
FWIW, in the patch, your declaration and definition don't match.
Reporter | ||
Comment 7•12 years ago
|
||
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)
Comment 8•12 years ago
|
||
(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?
Comment 9•12 years ago
|
||
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*.
Assignee | ||
Updated•10 years ago
|
Assignee: general → nobody
Comment 11•6 years ago
|
||
This is probably wontfix as filed, given multiple-globals-per-compartment, but does this still make sense with s/Compartment/Realm/?
Flags: needinfo?(jdemooij)
Comment 12•6 years ago
|
||
(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
Comment 13•4 years ago
|
||
We have a JS::GetRealmGlobalOrNull
API now.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Comment 14•4 years ago
|
||
JS::GetRealmGlobalOrNull
was introduced in bug 1363200.
You need to log in
before you can comment on or make changes to this bug.
Description
•