Closed Bug 1404885 Opened 7 years ago Closed 4 years ago

Figure out what to do with JSGetterOp and JSSetterOp

Categories

(Core :: JavaScript Engine, enhancement, P3)

Product:
Core
Component:
JavaScript Engine
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
89 Branch
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox89 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

(Whiteboard: [js:tech-debt])

Attachments

(2 files)

Bug 1404885 part 1 - Replace GetterOp/SetterOp with a property attribute. r?jonco!
(deleted), text/x-phabricator-request
Details
Bug 1404885 part 2 - Call putAccessorProperty directly, remove changeProperty. r?jonco!
(deleted), text/x-phabricator-request
Details
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.
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.
Flags: needinfo?(wmccloskey)
Flags: needinfo?(kmaglione+bmo)
(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.
Flags: needinfo?(kmaglione+bmo)
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.
(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).
> 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.
(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.
status-firefox57: --- → wontfix
Priority: -- → P3
Whiteboard: [js:tech-debt]
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.
Flags: needinfo?(wmccloskey)
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.
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).
Flags: needinfo?(kmaglione+bmo)
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.
Flags: needinfo?(kmaglione+bmo)
(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!
Depends on: 1412456
Unassigning myself for now. I may pick this up again after bug 1412456 lands.
Assignee: jdemooij → nobody
Status: ASSIGNED → NEW
Depends on: 1469217
Depends on: 1445551
No longer depends on: 1412456
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.
Depends on: 1470081
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.
Blocks: 1700052

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.

Assignee: nobody → jdemooij
Status: NEW → ASSIGNED

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

Pushed by jdemooij@mozilla.com: 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

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

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox89: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 89 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: