Use new Proxy instead of Proxy.create in SpecialPowers

RESOLVED FIXED in Firefox 47

Status

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

unspecified
mozilla47
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Description

5 years ago
Posted patch wip (obsolete) — Splinter Review
No description provided.
Assignee

Comment 1

5 years ago
Okay this doesn't actually work.
Posted patch wip2 (obsolete) — Splinter Review
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++?
Attachment #8398650 - Attachment is obsolete: true
Flags: needinfo?(bobbyholley)
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?
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.
Flags: needinfo?(bobbyholley)
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.
(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.
(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)
(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.
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.
Depends on: 1053271
Depends on: 787070
Assignee

Updated

3 years ago
Assignee: nobody → evilpies
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.
Assignee

Comment 11

3 years ago
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.
That's just bustage from my patches in bug 1069556, they got backed out on inbound.
Assignee

Comment 13

3 years ago
Posted patch WIP v3 (obsolete) — Splinter Review
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.
Attachment #8471472 - Attachment is obsolete: true
Assignee

Comment 15

3 years ago
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?
Assignee

Comment 16

3 years ago
Posted patch WIP v4 (obsolete) — Splinter Review
This version passes try, however I am currently working on a new patch to make this a whole bunch simpler.
Attachment #8712274 - Attachment is obsolete: true
Assignee

Comment 17

3 years ago
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
Attachment #8712799 - Attachment is obsolete: true
Attachment #8713172 - Flags: review?(bobbyholley)
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?
Attachment #8713172 - Flags: review?(bobbyholley) → review+

Comment 20

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ea83a7bd31db
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.