Closed Bug 969410 Opened 6 years ago Closed 6 years ago

Figure out what to do with JS_GetObjectId in a moving GC world

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 990290

People

(Reporter: bzbarsky, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

The current implementation returns the pointer, basically, but of course in a moving GC world that can change.
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.
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.
(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.
My vote is for "kill with fire" and figure out something more reasonable for the consumer to do there.
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.
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)
Note that I did the leg-work here to expose WeakMaps to the browser in bug 973780.
Depends on: 973780
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.
O_O

I'm really not sure how I managed to not notice that.
Attachment #8379201 - Attachment is obsolete: true
Attachment #8379201 - Flags: review?(sphink)
Attachment #8379805 - Flags: review?(sphink)
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+
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.
Blocks: 875863
No longer blocks: GenerationalGC
Bobby removed JS_GetObjectId in bug 990290, so we're good here.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 990290
Blocks: 1507445
You need to log in before you can comment on or make changes to this bug.