1389510 - Remove getter/setter ClassOps

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

There's quite a lot of complexity and overhead in the engine to support the getter/setter ClassOps. There are only 4 uses in Gecko, none of them really justifies keeping these hooks:

(1) XPConnect storage/ classes - AsyncStatementJSHelper, StatementJSHelper, StatementRow, AsyncStatementParams, StatementParams

Here I wonder if we could use proxies, or a resolve hook that just defines a getter/setter for the relevant properties...
 
(2) XPConnect classes using XPC_WN_MaybeResolvingSetPropertyStub and XPC_WN_CannotModifySetPropertyStub to disallow property modifications. I wonder if we could use Object.seal here instead...

(3) NPAPI - NPObjWrapper_GetProperty, NPObjWrapper_SetProperty, NPObjectMember_GetProperty.

Should we go in and convert these to proxies?

(4) CTypes - ArrayType::Getter/Setter

Probably also proxies or a resolve hook defining a plain getter/setter prop.

Comment 1

[Jan de Mooij [:jandem]]

bz, I was wondering if you had a few minutes to share your thoughts on these.

I'm happy to spend some time on it; these hooks incur some non-trivial perf overhead and complexity.

Comment 2

[Jan de Mooij [:jandem]]

Also, is this stuff exposed to content? The second one maybe? With the old extensions dying in 57, it might be a bit easier to change this.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> (1) XPConnect storage/ classes - AsyncStatementJSHelper, StatementJSHelper, StatementRow, AsyncStatementParams, StatementParams

We could try converting these to webidl proxies and off xpconnect altogether.  That's been on the list as something we might want to do anyway, to simplify xpconnect...

That said, looking at these consumers...  AsyncStatementJSHelper only implements a getter so it can do it for the string "params".  That's dumb; we could just have IDL for this bit with an nsISupports return type, even without converting to webidl.  I other words, there's no real reason I see to have AsyncStatement be a proxy at all; there's a bunch of pointless complexity here.  There might be some issues about threadsafety and making sure things get released on the right threads, but that can be handled just as easily with a cached nsIWhatever pointer as with the JS object holder bit.

StatementJSHelper... same deal.  It does "params" and "row", but both could be IDL getters.  Oh, and it has a resolve hook with a "step" thing that's deprecated and could probably die, but that's a sideshow here.  Anyway, this has no need of being a proxy or having a getter either.

StatementRow has an honest "many random strings" getter that would need to become a proxy, at least at first glance.  That said, it has a resolve hook too, and resolve the properties (to a value prop with value undefined), so it could probably define an accessor in the resolve hook too.

AsyncStatementParams could do the resolve hook with accessors thing, but really it should be a proxy conceptually: it supports arbitrary named and indexed sets.  The resolve hook with setter might just be simpler than rejiggering it to a proxy altogether.  Same thing for StatementParams.

> (2) XPConnect classes using XPC_WN_MaybeResolvingSetPropertyStub and XPC_WN_CannotModifySetPropertyStub

I'm not entirely sure whether we need these.  We have these things like that for addProperty already, which should cover the cases we really care about here, I think.  In particular, all consumers of XPC_SCRIPTABLE_USE_JSSTUB_FOR_ADDPROPERTY also use XPC_SCRIPTABLE_USE_JSSTUB_FOR_SETPROPERTY, so we can't have a case where addProperty is nullptr but setProperty needs to be one of those stubs....  I'm pretty sure we could just require that pattern and nuke these stubs, but it might be worth double-checking with bholley or peterv in case I missed something.

> (3) NPAPI - NPObjWrapper_GetProperty, NPObjWrapper_SetProperty, NPObjectMember_GetProperty.

I think using proxies here makes sense.  This just needs to forward through all sorts of stuff to underlying plugin bits.

> (4) CTypes - ArrayType::Getter/Setter

No opinion; haven't looked into it.

> Also, is this stuff exposed to content?

The NPAPI bits are.  For XPConnect stuff, it can only be exposed to content if it has the DOM_OBJECT flag in its classinfo.  That's set by stuff in nsDOMClassInfo.cpp, all of which set XPC_SCRIPTABLE_USE_JSSTUB_FOR_SETPROPERTY so get a null setProperty hook.  It's also set by Components.interfaces, Components.interfacesByID, Components.results, Components, .  So in theory chrome code could hand those out to content, but we don't normally expose them there.   We should drop DOM_OBJECT from here; I filed bug 1389581 on this.  Exception has this too, but uses webidl bindings.  I think it can also lose the classinfo.  XPCJSID has this; I'm not sure whether those are exposed and how.

I agree that with legacy extensions dying the constraints here are loosened; for example https://bugzilla.mozilla.org/show_bug.cgi?id=1245140 is automatically fixed.

Comment 4

[Jan de Mooij [:jandem]]

Thanks bz! That's super useful information.

I'll work on fixing some of these and then we can see what's left.

Comment 5

[Jan de Mooij [:jandem]]

I have patches in flight for almost all of these, fingers crossed. What's left is WebIDL-ification of (Async)StatementParams, but that should be similar to my StatementRow patch so I'm holding off on that.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Remove XPConnect getProperty/setProperty code

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Remove getProperty/setProperty uses

This removes getGetProperty and getSetProperty, the next (big) patch will actually remove the ClassOps fields.

Note that this is just the beginning, we can probably also remove JSFUN_STUB_GSOPS and the get/set property stubs now, and clean up the DefineProperty code. I'll file follow-up bugs for that.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Remove Class hooks

This is just a mechanical patch removing these fields from all Class definitions.

Comment 9

[Tom S [:evilpie]]

Comment on attachment 8900666 [details] [diff] [review]
Part 2 - Remove getProperty/setProperty uses

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

Very nice to have that gnarly behavior removed. Removing all those getProperty checks made me realize that we never seem to check for any of those ObjectOps like getProperty/lookupProperty.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff7f36a9dd90
part 1 - Remove getProperty/setProperty hooks from XPConnect. r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/6880dc2a3c29
part 2 - Remove checks for getProperty/setProperty hooks in SpiderMonkey. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c56761b02a1
part 3 - Remove getProperty/setProperty hooks from ClassOps. r=evilpie

Comment 11

[Nicholas Nethercote [inactive]]

Nice!

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ff7f36a9dd90
https://hg.mozilla.org/mozilla-central/rev/6880dc2a3c29
https://hg.mozilla.org/mozilla-central/rev/2c56761b02a1