1404885 - Figure out what to do with JSGetterOp and JSSetterOp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Jan de Mooij [:jandem]]

These are responsible for a lot of complexity. I think we have the following remaining (actual) consumers:

(1) array_length_getter/setter (setter seems dead but that doesn't change much)

(2) ArgumentsObject - UnmappedArg{G,S}etter, MappedArg{G,S}etter

(3) XPConnect writeToProto_getProperty and writeToProto_setProperty - here we could maybe use a proxy or convert to JSNative getters/setters.

---

The main question is what to do about (1) and (2) without hurting perf (no free lunch). Some options are:

(a) Convert to JSNative getters/setters with a new IsDataDescriptor property attribute.

(b) Use a tagged JSObject* pointer, where 0x1 means "data descriptor, what to do depends on holder->is<ArrayObject>() or holder->is<ArgumentsObject>()". This would be somewhat similar to TaggedProto - we could call it TaggedAccessor.

I'm leaning towards (b) because it's easier to assert correctness (and let's us use the type system for this), but I don't really know yet.

Comment 1

[Boris Zbarsky [:bzbarsky]]

For (3), I wonder whether we still need that machinery.  It was put in place for compartment-per-addon, iirc.  But I'm not sure whether webextensions actually use the machinery in question.  If they don't, then maybe we can just remove the machinery altogether, after checking whatever small number of things may still be using it

Using a proxy for (3) would be pretty hard, because the object involved is the sandbox global.  Using a JSNative would be doable with some hackery: we'd need to reify the JSFunction ourselves, give it extra slots, and store the jsid involved in one of those slots, I think.

Comment 2

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #1)
> For (3), I wonder whether we still need that machinery.  It was put in place
> for compartment-per-addon, iirc.  But I'm not sure whether webextensions
> actually use the machinery in question.  If they don't, then maybe we can
> just remove the machinery altogether, after checking whatever small number
> of things may still be using it

No, we don't need that machinery anymore. It might theoretically still be used if we have overlay-based system add-ons, but I think system add-ons can live without it.

We're eventually going to need to do something similar for WebExtensions for bug 1208775, but we're going to take a different approach there.

I think Bill promised to remove the Sandbox uses whenever we got to the point of removing these hooks.

Comment 3

[Tom S [:evilpie]]

I  think we can handle those properties like dense-elements. I.e the machinery we have around PropertyResult::setDenseOrTypedArrayElement. The obvious downside is how invasive this change is, but on the upside we don't need any kind of special getters/setter support anymore.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Tom Schuster [:evilpie] from comment #3)
> I  think we can handle those properties like dense-elements. I.e the
> machinery we have around PropertyResult::setDenseOrTypedArrayElement. The
> obvious downside is how invasive this change is, but on the upside we don't
> need any kind of special getters/setter support anymore.

It's an option but I don't want to make hot paths like jit::GetNativeDataProperty slower by adding extra checks. We could add a Shape flag to cover both the "resolve hook" and "data descriptor" cases, but it's still an extra case to consider when iterating and looking up properties and I'm worried it would be a new footgun. We'd also need to store the property attributes somewhere (assuming these properties can have non-default attributes).

Comment 5

[Boris Zbarsky [:bzbarsky]]

> assuming these properties can have non-default attributes

Array length is non-configurable and writable.

Arguments object indexed properties seem to be configurable, at first glance.  So yes, sounds like we need to keep track of all possible attributes.

Comment 6

[Jeff Walden [:Waldo]]

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #5)
> > assuming these properties can have non-default attributes
> 
> Array length is non-configurable and writable.

...but can be made non-writable, because TC39 in its infinite wisdom made it that way, so array length attributes can change.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'd love to get rid of the compartment-per-addon code, but I think we still kinda use it to decide whether it's okay to use CPOWs. We want to make it possible for tests to use CPOWs, but the browser itself should not be able to. There's probably a simpler way to do that than compartment-per-addon. But I think that the crud that segregates the different users of browser.xul globals is still necessary, and that's the really gross part here.

I'll try to think of another way to do it though. I'll also try to figure out how many of our tests still use CPOWs.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Bug 1412456 will get us to the point where we can remove the compartment-per-addon code. I should have it done in a couple days.

Comment 9

[Jan de Mooij [:jandem]]

Kris, do you know what the status is of comment 7 and comment 8?

If comment 0 is still correct, then this would allow us to remove PropertyOp getters/setters from the public API and it also lets us simplify a number of things (for instance, JSPROP_SHADOWABLE we could remove because the JS engine PropertyOps are shadowable).

Comment 10

[Kris Maglione [:kmag]]

We can probably remove those hooks without breaking anything, at this point. Bill didn't have a chance to finish removing the compartment-per-addon code before he left, so I'll probably have to finish it sometime this month. But we can probably remove the sandbox hooks sooner.

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #10)
> so I'll probably have to finish it sometime this month. But
> we can probably remove the sandbox hooks sooner.

"sometime this month" sounds great actually, thanks!

Comment 12

[Jan de Mooij [:jandem]]

Unassigning myself for now. I may pick this up again after bug 1412456 lands.

Comment 13

[Jan de Mooij [:jandem]]

Bug 1469217 does some cleanup in this area and will assert the only PropertyOp accessors are on ArrayObject and ArgumentsObject. In other words, (1) and (2) in comment 0.

IMO it makes sense to replace GetterOp/SetterOp with a class wrapping a JSObject* very similar to TaggedProto. It could use pointer value 0x1 for array.length, 0x2 for ArgumentsObject. Then the places where we call GetterOp/SetterOp just have to do a switch statement. We can then remove JSPROP_GETTER, JSPROP_SETTER, AutoRooterGetterSetter, etc.

Comment 14

[Jan de Mooij [:jandem]]

Bug 1470081 removes GetterOp/SetterOp from PropertyDescriptor and JSAPI so maybe comment 13 is overkill.. GetterOp/SetterOp are now only used in the NativeObject/Shape code so there might be a simpler design.

I don't have time to do more cleanup right now; maybe later this year if nobody beats me to it.

Comment 15

[Jan de Mooij [:jandem]]

Attachment: Bug 1404885 part 1 - Replace GetterOp/SetterOp with a property attribute. r?jonco!

At this point the old GetterOp/SetterOp mechanism is only used for the special
array.length and ArgumentsObject properties. This means we no longer need the
generic GetterOp/SetterOp function pointers.

Instead, we now flag these properties as JSPROP_CUSTOM_DATA_PROP and use a nullptr
getter and setter. At the point where we used to call the old GetterOp/SetterOp,
we now dispatch on the JSClass and call the getter/setter code directly.

This still isn't perfect, but it unblocks further cleanup and optimization work.
For example, this patch also cleans up the Shape code to use JS objects directly
for getters/setters, instead of casting these objects to GetterOp/SetterOp.

We still use addAccessorProperty and putAccessorProperty to add/redefine the
custom data properties on array and arguments objects. When we change how the
accessor properties are implemented, we can rename this to addCustomDataProperty
and putCustomDataProperty and simplify the code more.

Comment 16

[Jan de Mooij [:jandem]]

Attachment: Bug 1404885 part 2 - Call putAccessorProperty directly, remove changeProperty. r?jonco!

NativeObject::changeProperty has one caller and it can call putAccessorProperty
directly. This matches the similar code for the custom data properties on
arguments objects.

Depends on D109516

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2b8d51efc9c
part 1 - Replace GetterOp/SetterOp with a property attribute. r=jonco
https://hg.mozilla.org/integration/autoland/rev/7013a7369d6d
part 2 - Call putAccessorProperty directly, remove changeProperty. r=jonco

Comment 18

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/c2b8d51efc9c
https://hg.mozilla.org/mozilla-central/rev/7013a7369d6d