Closed Bug 1041631 Opened 5 years ago Closed 5 years ago

Disable Symbols in Firefox 33

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox33 + fixed
firefox34 --- fixed
relnote-firefox --- -

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

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

Attachments

(5 files)

There's enough stuff that just isn't right that I think we should delay one more cycle.

   Bug 1041261 - Audit DOM proxies that use jsid for symbol support
   Bug 918828 - Use @@iterator symbol instead of placeholder string
   Bug 1039686 - Symbols break the developer console

It's easy to disable without ripping out a ton of code.
Also Bug 1037313 - Assertion failure with symbols and Object#watch()
See https://bugzilla.mozilla.org/show_bug.cgi?id=645416#c169 for docs possibly in need of version update.
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
[Tracking Requested - why for this release]: This is a new feature that apparently still has some unfixed regressions, so it would be good if we didn't accidentally ship it early.
OK. Thanks for bringing that to our attention.
When are you planning to disable it?
Flags: needinfo?(continuation)
Jason is the one working on Symbol.
Flags: needinfo?(continuation) → needinfo?(jorendorff)
To be precise for the record, I think we should turn off the Symbol constructor everywhere except in nightly builds.  The fuzzers in the last few days seem to have started to really get on a symbols-crash kick, and they're regularly finding new things.  This stream must end before we can in good faith turn this on to ship.  I don't see that happening for at least another cycle, probably even longer.  We shouldn't put ourselves in a position of having to move to disable every cycle, but rather we should bow to reality and turn it off everywhere except nightlies, such change to be removed only when (at a minimum) nightly fuzzing isn't revealing anything new.
A similar boring patch is necessary for FF34 (currently m-c). I'll be done with that in an hour or two, I think.
Assignee: nobody → jorendorff
Attachment #8476069 - Flags: review?(jwalden+bmo)
Flags: needinfo?(jorendorff)
Part 2 will be pushed to both aurora and inbound (once the inbound version of part 1 is ready).
Attachment #8476071 - Flags: review?(jwalden+bmo)
Mostly a straightforward port of the FF33 version.

I made this by:
-   applying the FF33 version, fixing conflicts
-   commenting out the bit in jsversion.h to produce a hacked Nightly js shell
    with Symbols disabled
-   running the tests and fixing breakage
Attachment #8476187 - Flags: review?(jwalden+bmo)
Comment on attachment 8476069 [details] [diff] [review]
Part 1 - Make tests pass when Symbol is not defined (for FF33 branch, currently aurora)

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

::: js/src/jit-test/tests/basic/symbol-in-loop.js
@@ +1,2 @@
>  function f() {
>      return Object(Symbol());

I'd put the if here instead, and avoid indenting the entire loop.

::: js/src/jit-test/tests/proxy/testDirectProxyDefineProperty2.js
@@ +32,3 @@
>  assertEq(log.length, 2);
>  assertEq(log[0], 'foo');
> +assertEq(log[1], quux);

Symbol.for("quux") appears maybe to have been testing the registry stuff.  So have

function quux() { return typeof Symbol === "function" ? Symbol.for('quux') : 'quux'; }

and use quux() both places, instead.

::: js/src/jit-test/tests/symbol/bug-1033856.js
@@ +1,4 @@
> +if (typeof Symbol === "function") {
> +    function f(x, y) {
> +        return x == y;
> +    }

A nested function statement is materially different from what this used to be.  Add a helper function instead:

function sym()
{
  return typeof Symbol === "function" ? Symbol() : "non-symbol";
}

...f(1, sym());

::: js/src/jit-test/tests/symbol/not.js
@@ +5,5 @@
> +    var a = [0, 0, 0, 0, 0, Symbol(), Symbol()];
> +    var b = [];
> +    function f(i, v) {
> +        b[i] = !v;
> +    }

Nested function statement.  Use a minimal change instead:

var sym = this.Symbol || () => "truthy";

::: js/src/jit-test/tests/symbol/truthiness.js
@@ +5,5 @@
> +    var a = [0, 0, 0, 0, 0, Symbol(), Symbol()];
> +    var b = [];
> +    function f(i, v) {
> +        b[i] = v ? "yes" : "no";
> +    }

Nested function again.  Use a sym helper instead:

var sym = this.Symbol || () => "truthy";

::: js/src/jit-test/tests/symbol/typed-arrays.js
@@ +14,5 @@
>  
> +    function copy(arr, big) {
> +        for (var i = 0; i < LENGTH; i++)
> +            arr[i] = big[i];
> +    }

Nested function statement.  I'd fix using this function instead of Symbol.for("comet"):

var sym = this.Symbol ? () => Symbol.for("comet") : () => NaN;

::: js/src/jit-test/tests/symbol/typeof.js
@@ +2,5 @@
> +    var a = [0, 0, 0, 0, 0, Symbol(), Symbol()];
> +    var b = [];
> +    function f(i, v) {
> +        b[i] = typeof v;
> +    }

Nested function statement, not same as before.

function sym()
{
  return typeof Symbol === "function" ? Symbol() : "symbol";
}

function typ()
{
  return typeof Symbol === "function" ? "symbol" : "string";
}

::: js/src/tests/ecma_6/Object/getOwnPropertySymbols-proxy.js
@@ +18,5 @@
>  
> +if (typeof Symbol === "function") {
> +    // getOwnPropertySymbols(proxy) calls the getOwnPropertyNames hook (only).
> +
> +    var symbols = [Symbol(), Symbol("moon"), Symbol.for("sun"), Symbol.iterator];

A little screwball having this declared nestedly when only used lexically outside this block, but it works.

::: js/src/tests/ecma_6/Symbol/as-base-value.js
@@ +20,5 @@
> +    Object.defineProperty(Symbol.prototype, "prop", {
> +        get: function () {
> +            "use strict";
> +            gets++;
> +            assertEq(typeof this, "object");

I don't think this is right, but bug 603201 isn't fixed yet (it's very very high on my list right now), so whatever.

@@ +28,5 @@
> +        },
> +        set: function (v) {
> +            "use strict";
> +            sets++;
> +            assertEq(typeof this, "object");

Ditto.

::: js/src/tests/ecma_6/Symbol/conversions.js
@@ +10,2 @@
>  
> +    if (Symbol.toPrimitive in Symbol.prototype) {

Hmm, does this test really possibly want to just check for Symbol.toPrimitive at all?  I mean, I guess it works to check for "undefined" in Symbol.prototype too, as long as @@toPrimitive implementation coincides with its proper definition for symbols.  But that kind of begs the question, doesn't it?  Seems like this is not really right.

::: js/src/tests/ecma_6/Symbol/for-in-order.js
@@ +24,5 @@
> +    // Test with more properties.
> +    for (var i = 0; i < 1000; i++)
> +        obj[Symbol(i)] = i;
> +    obj.w = 1000;
> +    keys = []

Tack on a ; while you're changing.
Attachment #8476069 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8476071 [details] [diff] [review]
Part 2 - Make ES6 Symbols Nightly-only

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

::: js/src/jsversion.h
@@ +43,5 @@
>   * support likely to be made opt-in at some future time.
>   */
>  #define JS_OLD_GETTER_SETTER_METHODS    1
>  
> +/* Support for Symbols - Nightly-only until bug 918828 lands. */

I'd add " and we're sure it's stable and symbols are properly handled everywhere a symbol might be passed" or something.  I don't mind that being a prerequisite to enabling this (tho I probably wouldn't make it so), but it is certainly not the only prerequisite, or the most important one.
Attachment #8476071 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8476187 [details] [diff] [review]
Part 1 - Make tests pass when Symbol is not defined (for FF34 branch, currently mozilla-central)

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

Comments on the other patch transfer to here as well.

::: js/src/jit-test/tests/ion/bug1054753.js
@@ +1,4 @@
>  // |jit-test| error: TypeError
> +if (typeof Symbol === "function") {
> +    g = (function() {
> +        var Int32ArrayView = Int32Array();

Someone's going to have to add a new here eventually, feel free to do so or not if you want.

::: js/src/jit-test/tests/proxy/testDirectProxyHas6.js
@@ +9,5 @@
>  
>  for (let p of [new Proxy(target, handler), Proxy.revocable(target, handler).proxy]) {
>      assertEq('foo' in p, false);
> +    if (typeof Symbol === "function")
> +        assertEq(Symbol.iterator in p, false);

Oh, maybe this is the why for the JS_HAS_SYMBOLS prereq.  Could also check for "iterator" in Symbol to break it.

::: js/src/tests/ecma_6/Symbol/constructor.js
@@ +20,5 @@
> +    var hits = 0;
> +    var obj = {
> +        toString: function () {
> +            hits++;
> +            return "ponies";

You mispelled "poniez".

::: js/src/tests/ecma_6/Symbol/property-accessor.js
@@ +7,2 @@
>  
> +    var gets = 0;

Not sure about the point of gets if it's never going to be tested...
Attachment #8476187 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #11)
> > +/* Support for Symbols - Nightly-only until bug 918828 lands. */
> 
> I'd add " and we're sure it's stable and symbols are properly handled
> everywhere a symbol might be passed" or something.

I just changed it to "for now".
The link in comment 14 is a try server run for the patches I want to push to aurora.
The link in comment 15 is for inbound.

Jeff suggested several changes that we'll want to keep even after reverting these patches. I'll make those in a separate patch which I'll prepare tonight; unlike the rest, it's not urgent that they make this particular train (and there's no point making those changes in aurora at all).
Comment on attachment 8476069 [details] [diff] [review]
Part 1 - Make tests pass when Symbol is not defined (for FF33 branch, currently aurora)

These comments apply to both patches together. Part 1 (fixing tests) is meaningless without part 2 (disabling symbols)

Approval Request Comment
[Feature/regressing bug #]: bug 645416
[User impact if declined]: We would be shipping a new feature that isn't ready yet. (ES6 Symbols aren't stable; the fuzzers are still finding bugs. It's quite possible there are exploitable bugs.)
[Describe test coverage new/current, TBPL]: This is mainly a backout.
[Risks and why]: There's a remote possibility that this could introduce new bugs, since disabling Symbols is not exactly the same as backing out all the Symbols patches. Not likely though.
[String/UUID change made/needed]: None.
Attachment #8476069 - Flags: approval-mozilla-aurora?
Comment on attachment 8476069 [details] [diff] [review]
Part 1 - Make tests pass when Symbol is not defined (for FF33 branch, currently aurora)

Actually this half is testonly, no approval needed.
Attachment #8476069 - Flags: approval-mozilla-aurora?
Release Note Request (optional, but appreciated)
[Why is this notable]: (already relnoted, update required)
[Suggested wording]: Symbols have been implemented, disabled by default.
[Links (documentation, blog post, etc)]: (see Bug 645416 comment 169)
relnote-firefox: --- → ?
Flags: needinfo?(sledru)
We will add this to the release notes but once it ships. We don't add "disabled by default" items in the release notes. Thanks!
Flags: needinfo?(sledru)
https://hg.mozilla.org/mozilla-central/rev/9cfdbe321c17
https://hg.mozilla.org/mozilla-central/rev/15687eac5aa2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
Removed from Firefox 33 for developers release notes.

Added a note about only being available in Nightly at
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol#Browser_compatibility

Please add dev-doc-needed, when we are going to enable it again.
Attachment #8481601 - Flags: review?(efaustbmo)
Comment on attachment 8481601 [details] [diff] [review]
bug-1041631-part-1-followup-3-v1.patch

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

Yeah. We should make a note maybe to clean up all this conditional pass nonsense when they make it into the real world.
Attachment #8481601 - Flags: review?(efaustbmo) → review+
(In reply to Sylvestre Ledru [:sylvestre] from comment #22)
> We will add this to the release notes but once it ships. We don't add
> "disabled by default" items in the release notes. Thanks!

Then please delete the current entry in the Fx 33 release notes. Thanks!
Flags: needinfo?(sledru)
OK, I forgot that we already had them in 33 release notes.
I added it in aurora 34 release notes then.
I updated bug 645416 for the info
Flags: needinfo?(sledru)
OK, one more. I don't think `mach test browser/devtools/webconsole/test` runs those tests correctly, so I'll have to rely on the try server to see if this is the last patch. I think so though.
Attachment #8482042 - Flags: review?(efaustbmo)
Comment on attachment 8482042 [details] [diff] [review]
bug-1041631-part-1-followup-4-v1.patch

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

Works for me.

::: browser/devtools/webconsole/test/browser_webconsole_output_01.js
@@ +73,5 @@
>    {
>      input: "/foobar/",
>      output: "/foobar/",
>      inspectable: true,
>    },

should this trailing comma go?
Attachment #8482042 - Flags: review?(efaustbmo) → review+
(In reply to Eric Faust [:efaust] from comment #40)
> ::: browser/devtools/webconsole/test/browser_webconsole_output_01.js
> @@ +73,5 @@
> >    {
> >      input: "/foobar/",
> >      output: "/foobar/",
> >      inspectable: true,
> >    },
> 
> should this trailing comma go?

No, because there was a trailing comma in the original (I'm sure intentionally) and it's not my code!

Anyway, we're going to back this patch out as soon as symbols are enabled for real.

Thanks for the quick review! Try server is green. Pushing it.
Correction, inbound is closed. Everything's closed. I'll try to remember to do it later today.
OK, symbols are nightly-only in FF34; that part's done. It still remains to disable them in FF33 (using ports of the same patches).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8476071 [details] [diff] [review]
Part 2 - Make ES6 Symbols Nightly-only

Requesting approval to land this in the FF33 branch (currently beta).

Approval Request Comment
[Feature/regressing bug #]: bug 645416
[User impact if declined]: We would be shipping a new feature that isn't ready yet. (ES6 Symbols aren't stable; the fuzzers are still finding bugs. It's quite possible there are exploitable bugs.)
[Describe test coverage new/current, TBPL]: Patch is already in Aurora. See https://tbpl.mozilla.org/?tree=Try&rev=f5bec7e3a6b7 for a green try run on the Beta branch. Since this is just disabling a feature, the only test change needed is to make them pass when Symbol isn't defined.
[Risks and why]: There's a remote possibility that this could introduce new bugs, since disabling Symbols is not exactly the same as backing out all the Symbols patches. Not likely though.
[String/UUID change made/needed]: None.
Comment on attachment 8476071 [details] [diff] [review]
Part 2 - Make ES6 Symbols Nightly-only

Approval Request Comment - see comment 47
Attachment #8476071 - Flags: approval-mozilla-beta?
I checked all the testonly patches that I wrote for the FF34 branch (there were four followups to part 1) and most of those tests don't even exist, or don't contain the Symbol-related parts, in FF33. The only change still needed is:
  https://hg.mozilla.org/try/rev/980090c510ce
which I will land a=testonly along with part 2.
Attachment #8476071 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/337d96ca1194
https://hg.mozilla.org/releases/mozilla-beta/rev/eee93220473c
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Symbol is still listed as supported in mobile section of 33 release notes. I think this may result in some confusion.

See: https://www.mozilla.org/en-US/mobile/33.0/releasenotes/
Flags: needinfo?(sledru)
Inded. thanks for the info. removed!
Flags: needinfo?(sledru)
Depends on: 1129756
You need to log in before you can comment on or make changes to this bug.