Closed Bug 892903 Opened 7 years ago Closed 4 years ago

Remove Proxy.create and Proxy.createFunction

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: bruant.d, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, dev-doc-complete, site-compat, Whiteboard: [DocArea=JS])

Attachments

(3 files, 1 obsolete file)

They won't be part of ES6 (replaced by direct proxies). People use them while they shouldn't. Chrome hiding them behind a flag is the only thing that doesn't make them de facto standards (which would be an very unfortunate thing).
FYI, it is also used in the test suite to cause deterministic bailouts in IonMonkey.

Ideally, we should replaced them by a js-shell primitive functions which will be replaced by a bailout.
(In reply to Nicolas B. Pierron [:nbp] from comment #1)
> FYI, it is also used in the test suite to cause deterministic bailouts in
> IonMonkey.
> 
> Ideally, we should replaced them by a js-shell primitive functions which
> will be replaced by a bailout.
By any chance, would the use of direct proxies (new Proxy(target, handler)) make the trick in the short term if a primitive bailout function takes to much time to add?
We should start simpler than just trying to remove these: make them emit a deprecation warning when called, linking to a page talking about how to migrate to |new Proxy|.  Once that's out, that'll cut down a lot on the seeming burden of existing uses.
Blocks: es6
(In reply to David Bruant from comment #0)
> They won't be part of ES6 (replaced by direct proxies). People use them
> while they shouldn't. Chrome hiding them behind a flag is the only thing
> that doesn't make them de facto standards (which would be an very
> unfortunate thing).

I'm glad the Web hasn't been filled with full of mozProxy, webkitProxy, msProxy, oProxy, blinkProxy, servoProxy, smProxy, v8Proxy, jscProxy, chakraProxy, carakanProxy, etc. Experimental flags certainly work.
(Of course, we should not have shipped Proxy.create without flags from the start.)
Depends on: 989426
Attached patch v1 (obsolete) — Splinter Review
Let's first warn about this.
Attachment #8398653 - Flags: review?(jwalden+bmo)
Comment on attachment 8398653 [details] [diff] [review]
v1

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

::: js/src/js.msg
@@ +226,5 @@
>  MSG_DEF(JSMSG_NOT_A_CODEPOINT,        173, 1, JSEXN_RANGEERR, "{0} is not a valid code point")
>  MSG_DEF(JSMSG_BRACKET_AFTER_ARRAY_COMPREHENSION, 174, 0, JSEXN_SYNTAXERR, "missing ] after array comprehension")
>  MSG_DEF(JSMSG_NESTING_GENERATOR,      175, 0, JSEXN_TYPEERR, "already executing generator")
>  MSG_DEF(JSMSG_PAREN_AFTER_FOR_OF_ITERABLE, 176, 0, JSEXN_SYNTAXERR, "missing ) after for-of iterable")
> +MSG_DEF(JSMSG_DEPRECATED_PROXY_CREATE,177, 0, JSEXN_TYPEERR, "Proxy.create and Proxy.createFunction are deprecated use new Proxy instead")

Add a semicolon after "deprecated".

::: js/src/vm/GlobalObject.h
@@ +110,5 @@
>      static const unsigned REGEXP_STATICS          = DATE_TIME_FORMAT_PROTO + 1;
>      static const unsigned WARNED_WATCH_DEPRECATED = REGEXP_STATICS + 1;
>      static const unsigned WARNED_PROTO_SETTING_SLOW = WARNED_WATCH_DEPRECATED + 1;
> +    static const unsigned WARNED_PROXY_CREATE_DEPRECATED = WARNED_PROTO_SETTING_SLOW + 1;
> +    static const unsigned RUNTIME_CODEGEN_ENABLED = WARNED_PROXY_CREATE_DEPRECATED + 1;

We used to have a FLAGS slot that held checks and stuff like this.  At some point it, er, grew to hold only a single flag, and then someone made it a boolean.  But now we're back to having a bunch of these.  Might be worth going back to having a single FLAGS slot in a separate patch.
Attachment #8398653 - Flags: review?(jwalden+bmo) → review+
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Assignee: general → nobody
No longer blocks: es6
Depends on: 1110457
Blocks: 1103158
Keywords: site-compat
Duplicate of this bug: 942569
Depends on: 1243805
Assignee: nobody → evilpies
Depends on: 1244442
Comment on attachment 8398653 [details] [diff] [review]
v1

Moved to bug 1244442.
Attachment #8398653 - Attachment is obsolete: true
Depends on: 1245141
I tried updating most handwritten tests where it made sense. However I just deleted a lot of fuzz tests, because making them fail or succeed in the same way was usually just not worth it.
Attachment #8721562 - Flags: review?(efaustbmo)
Same disclaimer.
Attachment #8721563 - Flags: review?(efaustbmo)
I am actually surprised how self-contained this code seems to be.

We can't quite land this yet, because there are still uses in the addon-sdk and in crash tests. I will probably have to fix those myself.
Attachment #8721564 - Flags: review?(efaustbmo)
Comment on attachment 8721562 [details] [diff] [review]
Remove Proxy.create from jit-tests

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

::: js/src/jit-test/tests/baseline/bug881461.js
@@ +1,1 @@
> +z = new Proxy({__proto__: (function(){})}, {});

why can't we just use the function directly, here, like the last test seemed to?

::: js/src/jit-test/tests/basic/bug1085464.js
@@ +1,2 @@
>  function *f() {
> +    var o = new Proxy({}, {

I guess this test doesn't rely on [[Call]] behavior.

::: js/src/jit-test/tests/basic/bug1113980.js
@@ +3,5 @@
>          return {value: 1, configurable: true, writable: true};
>      },
>      defineProperty: function() {
>      }
>  }, null);

what does this null do, in the new world? It's just ignored, right?

::: js/src/jit-test/tests/basic/bug787847.js
@@ +7,3 @@
>  }
>  
> +var p = new Proxy({}, { get: get } );

do we need to change this trap? There's still a getOwnProperty, or something, right?

::: js/src/jit-test/tests/basic/splice-check-steps.js
@@ +21,2 @@
>      },
> +    // derived traps

there's no longer any such thing as a "derived trap". Do we need this comment?

::: js/src/jit-test/tests/basic/testCrossCompartmentTransparency.js
@@ +8,4 @@
>  "    defineProperty: () =>assertEq(true,false),            "+
> +"    deleteProperty: () =>assertEq(true,false),            "+
> +"    get: () =>assertEq(true,false),                       "+
> +"    set: () =>assertEq(true,false),                       "+

"fix" is preventExtensions, right? Do we want do add that here, also?

::: js/src/jit-test/tests/basic/testProxyDefinePropertyWithMissingSetter.js
@@ +9,4 @@
>        Object.defineProperty(x, name, desc)
>      },
> +});
> +

is the target here not eval? Shouldn't we maintain that?

::: js/src/jit-test/tests/debug/Object-callable.js
@@ +13,5 @@
>  g.eval("f({}, false);");
>  g.eval("f(Function.prototype, true);");
>  g.eval("f(f, true);");
> +g.eval("f(new Proxy({}, {}), false);");
> +g.eval("f(new Proxy(f, {}), true);");

I assume that omitting f here (and below) as the handler is sensible, since it has no trap properties. If so, no objections.

::: js/src/jit-test/tests/debug/Object-name-02.js
@@ +11,5 @@
>  
>  g.eval("f({});");
>  g.eval("f(/a*/);");
>  g.eval("f({name: 'bad'});");
> +g.eval("f(new Proxy({}, {}));");

sure, though I mildly prefer sticking with the function target

::: js/src/jit-test/tests/for-of/semantics-11.js
@@ +5,5 @@
>  var s = '';
>  
>  var i = 0;
> +var next_fn = new Proxy(function() {}, {
> +    apply() {

why is it better here (and below) to have he trap instead of just putting this body inside the target function?
Attachment #8721562 - Flags: review?(efaustbmo) → review+
Attachment #8721563 - Flags: review?(efaustbmo) → review+
Comment on attachment 8721564 [details] [diff] [review]
Remove Proxy.create and Proxy.createFunction

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

And I'll dance on its grave. r=me with prejudice.
Attachment #8721564 - Flags: review?(efaustbmo) → review+
Depends on: 1253866
And I will dance on its grave. \o/ \o/ \o/
This is a great day.
Keywords: addon-compat
Developer release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/48#JavaScript
(there are no real docs for the legacy Proxy API anymore, standard Proxy is documented, though)
Duplicate of this bug: 586168
You need to log in before you can comment on or make changes to this bug.