1010039 - Getter/delete/set technique doesn't actually work in JNI.jsm

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

JNI.jsm uses idioms like this,

>  get lib() {
>    delete this.lib;
>    return this.lib = ctypes.open("libxul.so");
>  },

But it doesn't work when lib is defined in the prototype, because you can't delete a prototype property from an instance. This is related to bug 839184.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Provide prototype prodecure producing properly processed property (v1)

Instead of trying to delete and then set the property, we can define a new property on the instance itself, which overrides the property defined in the prototype.

Comment 2

[Wesley Johnston (:wesj)]

Comment on attachment 8422581 [details] [diff] [review]
Provide prototype prodecure producing properly processed property (v1)

Review of attachment 8422581 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm.. Interesting I've never run into this before. You can delete the prototype property here (and assign it if you want). You'd have to do something like:

delete JNI.prototype.lib;
return JNI.prototype.lib = myLib;

However, in this case we probably do want this lib to be per-instance, so this is a good fix :)

::: mobile/android/modules/JNI.jsm
@@ +24,5 @@
> +  _defineProperty: function(prop, value) {
> +    // Override the original property defined in the prototype
> +    // by defining a property on the object instance itself.
> +    Object.defineProperty(this, prop, {
> +      get: () => value,

If we were going to use this, you should use value: here, but there are better ways to do this below.

@@ +33,2 @@
>    get lib() {
> +    return this._defineProperty("lib", ctypes.open("libxul.so"));

Hmm.. since we can't just define lib on the instance and use that (i.e. we can't kill the getter), I think I'd rather we just cached the value on the instance and rewrote THIS getter to return it if it exists. i.e.:

if (!this._lib) {
  this._lib = something;
}
return this._lib;

That's a little easier to read. I talked to one of the Dev tools guys and learned that's the pattern they'd use as well. They do that to try and optimize shapes (i.e. they want all their object to have the same memory layout so that SpiderMonkey can optimize them). Dynamically adding properties (or deleting them :( ) can upset those shapes and turn them into basic dictionary/hashtables.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Good points! I didn't know about the shapes optimization.

Closing as WONTFIX because we want bug 918309 instead.