Closed
Bug 969410
Opened 11 years ago
Closed 11 years ago
Figure out what to do with JS_GetObjectId in a moving GC world
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
DUPLICATE
of bug 990290
People
(Reporter: bzbarsky, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
The current implementation returns the pointer, basically, but of course in a moving GC world that can change.
Comment 1•11 years ago
|
||
Hmm. The only call site I see is here:
https://mxr.mozilla.org/mozilla-central/source/dom/xbl/nsXBLBinding.cpp#957
and what that code wants is a WeakMap. For correctness reasons, too.
One potential fix, then is to make our C++ WeakMap template usable for XBL's use case.
Reporter | ||
Comment 2•11 years ago
|
||
I'm pretty sure we can nix our one in-tree caller, yes. I can't speak for other API consumers.
But yes, "remove this API" would be a viable thing to do, for sure.
Comment 3•11 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #1)
> Hmm. The only call site I see is here:
> https://mxr.mozilla.org/mozilla-central/source/dom/xbl/nsXBLBinding.cpp#957
> and what that code wants is a WeakMap. For correctness reasons, too.
Does it actually want to be weak? At the moment we basically just define as a prop on the global, which pegs its lifetime to that of the scope.
Getting rid of that behavior would be great, FWIW.
> One potential fix, then is to make our C++ WeakMap template usable for XBL's
> use case.
We're probably going to want to expose that stuff anyway to store expandos for Xray-to-JS wrappers.
Comment 4•11 years ago
|
||
My vote is for "kill with fire" and figure out something more reasonable for the consumer to do there.
Blocks: GenerationalGC
Comment 5•11 years ago
|
||
Another option that could at least unblock GGC in the near term is to unconditionally do a MinorGC before returning the address. This isn't going to help compacting GC though, so we will still need to kill this interface asap.
Comment 6•11 years ago
|
||
This should at least prevent catastrophe in the near term. I'll mark the bug leave-open so we don't forget to make a real fix here.
Attachment #8379201 -
Flags: review?(sphink)
Comment 7•11 years ago
|
||
Note that I did the leg-work here to expose WeakMaps to the browser in bug 973780.
Updated•11 years ago
|
Keywords: leave-open
Reporter | ||
Comment 8•11 years ago
|
||
Comment on attachment 8379201 [details] [diff] [review]
do_not_expose_nursery_objects_in_getobjectid-v0.diff
This seems like it can't possibly work, since "obj" is a JSObject*, not a handle, right? Probably need to handlify the API while we're here.
Comment 9•11 years ago
|
||
O_O
I'm really not sure how I managed to not notice that.
Comment 10•11 years ago
|
||
Attachment #8379201 -
Attachment is obsolete: true
Attachment #8379201 -
Flags: review?(sphink)
Attachment #8379805 -
Flags: review?(sphink)
Comment 11•11 years ago
|
||
Comment on attachment 8379805 [details] [diff] [review]
do_not_expose_nursery_objects_in_getobjectid-v1.diff
Review of attachment 8379805 [details] [diff] [review]:
-----------------------------------------------------------------
I guess it's ok to make this change since it's better than the current state, but I would vastly prefer removing the API call entirely. Having persistent object IDs would be a great thing, and enable all kinds of stuff (eg making heap diffs more reliable/useful/robust.) But we don't, and having JS_GetObjectId in JSAPI is an outright lie.
File a followup bug to remove it?
Attachment #8379805 -
Flags: review?(sphink) → review+
Comment 12•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f879284bd82
(In reply to Steve Fink [:sfink] from comment #11)
> File a followup bug to remove it?
I made this bug leave-open to do that work next. I wanted to land this fix first as a contingency because (1) I wasn't sure when Bobby was going to land WeakMapPtr (I've now lost this race) and (2) the caller of this function is a total disaster. If "fixing" this turns into a multi-week adventure, now at least it won't also be a transitive facepalm for GGC too.
Comment 13•11 years ago
|
||
Updated•11 years ago
|
Keywords: leave-open
Updated•11 years ago
|
Comment 14•11 years ago
|
||
Bobby removed JS_GetObjectId in bug 990290, so we're good here.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•