Boris Zbarsky [:bzbarsky] Reporter
	Description • 11 years ago

The current implementation returns the pointer, basically, but of course in a moving GC world that can change.

	Jason Orendorff [:jorendorff]
	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.

	Boris Zbarsky [:bzbarsky] 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.

	Bobby Holley (:bholley)
	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.

	Terrence Cole [:terrence]
	Comment 4 • 11 years ago

My vote is for "kill with fire" and figure out something more reasonable for the consumer to do there.

	Terrence Cole [:terrence]
	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.

	Terrence Cole [:terrence]
	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.

	Bobby Holley (:bholley)
	Comment 7 • 11 years ago

Note that I did the leg-work here to expose WeakMaps to the browser in bug 973780.

	Boris Zbarsky [:bzbarsky] 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.

	Terrence Cole [:terrence]
	Comment 9 • 11 years ago

O_O

I'm really not sure how I managed to not notice that.

	Steve Fink [:sfink] [:s:]
	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?

	Terrence Cole [:terrence]
	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.

	Phil Ringnalda (:philor)
	Comment 13 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/8f879284bd82

	Terrence Cole [:terrence]
	Comment 14 • 10 years ago

Bobby removed JS_GetObjectId in bug 990290, so we're good here.