Remove Proxy.create and Proxy.createFunction

RESOLVED FIXED in Firefox 48

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: David Bruant, Assigned: evilpie)

Tracking

(Blocks: 1 bug, {addon-compat, dev-doc-complete, site-compat})

unspecified
mozilla48
addon-compat, dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [DocArea=JS])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 2

4 years ago
(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.
(Reporter)

Updated

4 years ago
Blocks: 694100
(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.)
(Assignee)

Comment 5

4 years ago
Shumway bug: https://github.com/mozilla/shumway/issues/1232
(Assignee)

Updated

4 years ago
Depends on: 989426
(Assignee)

Comment 6

4 years ago
Created attachment 8398653 [details] [diff] [review]
v1

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: 694100
(Reporter)

Updated

3 years ago
Depends on: 1110457
(Assignee)

Updated

3 years ago
Blocks: 1103158

Updated

3 years ago
Keywords: site-compat
Depends on: 986294
Duplicate of this bug: 942569
Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/legacy-proxy-api-will-be-removed/
(Assignee)

Updated

2 years ago
Depends on: 1243805
(Assignee)

Updated

2 years ago
Assignee: nobody → evilpies
(Assignee)

Updated

2 years ago
Depends on: 1244442
(Assignee)

Comment 10

2 years ago
Comment on attachment 8398653 [details] [diff] [review]
v1

Moved to bug 1244442.
Attachment #8398653 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Depends on: 1245141
(Assignee)

Comment 11

2 years ago
Created attachment 8721562 [details] [diff] [review]
Remove Proxy.create from jit-tests

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)
(Assignee)

Comment 12

2 years ago
Created attachment 8721563 [details] [diff] [review]
Remove Proxy.create from jstests

Same disclaimer.
Attachment #8721563 - Flags: review?(efaustbmo)
(Assignee)

Comment 13

2 years ago
Created attachment 8721564 [details] [diff] [review]
Remove Proxy.create and Proxy.createFunction

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+

Updated

2 years ago
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+
(Assignee)

Updated

2 years ago
Depends on: 1253866

Comment 16

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f999b1995dc6
https://hg.mozilla.org/integration/mozilla-inbound/rev/346979205b0a
https://hg.mozilla.org/integration/mozilla-inbound/rev/0607c9f8df24
And I will dance on its grave. \o/ \o/ \o/
This is a great day.
(Assignee)

Updated

2 years ago
Keywords: addon-compat

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f999b1995dc6
https://hg.mozilla.org/mozilla-central/rev/346979205b0a
https://hg.mozilla.org/mozilla-central/rev/0607c9f8df24
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/legacy-proxy-api-has-been-removed/
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)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.