989426 - Use new Proxy instead of Proxy.create in SpecialPowers

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Tom S [:evilpie]]

Attachment: wip

Comment 1

[Tom S [:evilpie]]

Okay this doesn't actually work.

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: wip2

This doesn't work because

121 INFO JavaScript error: http://mochi.test:8888/tests/SimpleTest/MemoryStats.js, line 32: proxy must report the same value for a non-writable, non-configurable property

The old ScriptableIndirectProxyHandler doesn't check the configurable & writable invariant.

Wouldn't it be easier if the SpecialPowers wrapper was implemented in C++?

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 8471472 [details] [diff] [review]
wip2

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

I mostly doubt this is easier to implement in C++.  And it's just a prop-up for testing purposes, so there's no reason to care about speed, or security-consciousness (too much), etc.

If you're suggesting easier-in-C++ specifically to violate the configurable/writable invariants, that's a no-go.  To the extent the JSAPI lets you violate that now in C++, it's buggy.

::: testing/specialpowers/content/specialpowersAPI.js
@@ +199,3 @@
>    // Handle our special API.
>    if (name == "SpecialPowers_wrappedObject")
> +    return { value: obj, writeable: false,

Uh, this should be "writable".  How does this work at all, and how is it not failing some sort of test?

Comment 4

[Bobby Holley (:bholley)]

Yeah, it's not going to be a simple and straightforward conversion. For what it's worth, it's likely to be _much_ simpler in a couple of months, once we:

(a) fix bug 787070 (making DOM Xrays have meaningful prototypes)
and
(b) Remove XPCWN Xrays

Is this blocking anything, or can we just wait?

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> ::: testing/specialpowers/content/specialpowersAPI.js
> @@ +199,3 @@
> >    // Handle our special API.
> >    if (name == "SpecialPowers_wrappedObject")
> > +    return { value: obj, writeable: false,
> 
> Uh, this should be "writable".  How does this work at all, and how is it not
> failing some sort of test?

It just gets ignored, and the property is reported as writable, right? It's not like we have exhaustive conformance tests for these wrappers.

Comment 5

[Jeff Walden [:Waldo]]

The defaults are *non*-enumerable, *non*-configurable, and for data properties, *non*-writable.  That said, I'm unsure how this object gets marshalled into what the JS engine sees here.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> The defaults are *non*-enumerable, *non*-configurable, and for data
> properties, *non*-writable.  That said, I'm unsure how this object gets
> marshalled into what the JS engine sees here.

Ah, yes. Then writeable: false is equivalent to writable: false.

Also, nobody ever writes to this property.

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Bobby Holley (:bholley) from comment #4)
> Yeah, it's not going to be a simple and straightforward conversion. For what
> it's worth, it's likely to be _much_ simpler in a couple of months, once we:
> 
> (a) fix bug 787070 (making DOM Xrays have meaningful prototypes)
> and
> (b) Remove XPCWN Xrays
> 
> Is this blocking anything, or can we just wait?

It doesn't block anything. I was trying to debug a SpecialPowers.wrap bug that reports "reference to undefined property x.SpecialPowers_wrappedObject" when used in nested content process (bug 1038620)

Comment 8

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> Comment on attachment 8471472 [details] [diff] [review]
> wip2
> 
> Review of attachment 8471472 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I mostly doubt this is easier to implement in C++.  And it's just a prop-up
> for testing purposes, so there's no reason to care about speed, or
> security-consciousness (too much), etc.
> 
> If you're suggesting easier-in-C++ specifically to violate the
> configurable/writable invariants, that's a no-go.  To the extent the JSAPI
> lets you violate that now in C++, it's buggy.

To violate the invariants is one reason. I thought the jsproxy C++ API already has many helper methods to implement wrappers and  a specialpowers wrapper would be a very thin wrapper on top of that, but that's only my guess.

Comment 9

[Bobby Holley (:bholley)]

Yeah, I think we definitely want this to remain in JS. Among other things, we try to have as little C++ automation glue as possible.

Comment 10

[Bobby Holley (:bholley)]

Note: DOM Xrays now have meaningful prototypes, and XPCWNs are rare enough that I think we can probably just not handle them, we should be able to reimplement this as a classic wet/dry membrane and everything should Just Work.

Comment 11

[Tom S [:evilpie]]

Doesn't look too bad if you actually look at the failures. https://treeherder.mozilla.org/#/jobs?repo=try&revision=44527068d9f1&selectedJob=15946190

Most of them are:
Exception: Error getting fileid for /builds/slave/test/build/application/firefox/libxul.so:
TEST-UNEXPECTED-FAIL | ShutdownLeaks | process() called before end of test suite 
But I am not sure if that is just some infra or fluke.

Comment 12

[(not currently active) Ted Mielczarek]

That's just bustage from my patches in bug 1069556, they got backed out on inbound.

Comment 13

[Tom S [:evilpie]]

Attachment: WIP v3

This seems to do pretty well. The basic trick here is that we create a Proxy around an empty object/function. And as long as we never make the dummy target object non-extensible or return a non-configurable descriptor we can basically do whatever we want.

Comment 14

[Tom S [:evilpie]]

Looks like we are pretty close now https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d04b5fae120&selectedJob=15966892

Comment 15

[Tom S [:evilpie]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=44dea134f920 declaring success. I have a few ideas how we could simplify the whole thing (especially getting rid of doGetPropertyDescriptor), do you want to review the current patch first and we can do that later?

Comment 16

[Tom S [:evilpie]]

Attachment: WIP v4

This version passes try, however I am currently working on a new patch to make this a whole bunch simpler.

Comment 17

[Tom S [:evilpie]]

Attachment: v1 Reimplement SpecialPowers with new Proxy()

So the basic idea here is that we create Proxy around some dummy object. The reason for this is that with the new proxy invariants, every property that is non-configurable and non-wirtable on the target, needs to be returned as is by the Proxy. This however means we could never create SpecialPower wrappers for such properties, if the proxy target was the real wrapped-object. To work around this we always pretend that all our properties are configurable. All proxy traps are forwarded to our real wrapped-object.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd25dcdc63cd

Comment 18

[Bobby Holley (:bholley)]

Comment on attachment 8713172 [details] [diff] [review]
v1 Reimplement SpecialPowers with new Proxy()

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

I didn't read the proxy handler implementation super closely, but it looks much more sane in general (and the semantics of this wrapper are basically "does it pass try?").

Really glad to see all the quickstubs and XPCWN weirdness go!

::: layout/base/tests/test_bug394057.html
@@ +47,5 @@
>  var monospaceIdx = 0;
>  var monospaceWidth, serifWidth;
>  monospaceWidth = table_width_for_font(monospaceFonts[monospaceIdx]);
>  for (serifIdx in serifFonts) {
> +  info("serifIdx: " + serifIdx);

Do we need this change?

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea83a7bd31db

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea83a7bd31db