1253016 - Remove legacy __defineGetter__/__defineSetter__ this behavior

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Tom S]

The amount of time this behavior is triggered is basically irrelevant. 1.48B vs 24 (http://mzl.la/1LW2fdo).

So with this change when invoking __define{GS}etter__ with null/undefined will throw. We have a few addons that trigger this by using __defineGetter__("a", function(){}) at the global scope.

Comment 1

[Tom S]

Attachment: Remove legacy __defineGetter__/__defineSetter__ this behavior

Comment 2

[Tom S]

Attachment: Add steps to legacy functions and a new test

Comment 3

[Till Schneidereit [:till]]

Comment on attachment 8729148 [details] [diff] [review]
Remove legacy __defineGetter__/__defineSetter__ this behavior

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

Do we still need the telemetry? Do you intend to check later on that we didn't make a mistake in removing this? If you don't, then please also remove the reporting, perhaps in a new patch.

::: js/src/jit-test/tests/ion/bug1076026.js
@@ +1,1 @@
> +var global = this;

This doesn't seem to be required.

Comment 4

[Till Schneidereit [:till]]

Comment on attachment 8729150 [details] [diff] [review]
Add steps to legacy functions and a new test

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

Can you change the patch description to include the actual code changes? Without that, this sort of hides the fact that there's more going on than a simple addition of comments. r=me with that, with or without feedback addressed.

::: js/src/builtin/Object.js
@@ +59,5 @@
>      // Step 2.
>      return callContentFunction(O.toString, O);
>  }
>  
> +// ES7 draft (2016 March 10) B.2.2.3

The draft is actually from March 8. It won't matter in this case, but theoretically there could be situations where relevant changes are committed between release of a draft and whenever the code comment is made.

@@ +89,4 @@
>      std_Object_defineProperty(object, key, desc);
> +
> +    // Step 6.
> +    return undefined;

I usually do `// Step 6 (implicit).` and omit the return, but up to you.

Comment 5

[Tom S]

Attachment: Fix a DOM test that uses legacy define behavior

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8729703 [details] [diff] [review]
Fix a DOM test that uses legacy define behavior

r=me

Comment 7

[Tom S]

(In reply to Till Schneidereit [:till] from comment #3)
> Do we still need the telemetry? Do you intend to check later on that we
> didn't make a mistake in removing this? If you don't, then please also
> remove the reporting, perhaps in a new patch.

I want to keep this one more version, so we still get Telemetry and can revert this change if something bad happens.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd22586cac82
https://hg.mozilla.org/integration/mozilla-inbound/rev/16b05a0fb83a
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b19afd04625

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bd22586cac82
https://hg.mozilla.org/mozilla-central/rev/16b05a0fb83a
https://hg.mozilla.org/mozilla-central/rev/7b19afd04625

Comment 10

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/definegetter-and-definesetter-can-no-longer-be-called-at-the-global-scope/

Comment 11

[Boris Zbarsky [:bzbarsky]]

Thanks for posting that!  One nit:

> since the legacy behaviour, where the associated object becomes null or undefined, is not allowed by ECMAScript 7

The legacy behavior is that undefined or null this means this gets set to the global.  That's backwards from what you have in the compat note...

Comment 12

[Kohei Yoshino]

Thanks Boris! Yes, the draft was misleading. Corrected as follows:
https://github.com/fxsitecompat/www.fxsitecompat.com/commit/a981f7c

Comment 13

[Boris Zbarsky [:bzbarsky]]

Ah, excellent.  I hadn't realized there was a github repo for fxsitecompat; I would have just filed an issue there.

Comment 14

[Kohei Yoshino]

(In reply to Boris Zbarsky [:bz] from comment #13)
> Ah, excellent.  I hadn't realized there was a github repo for fxsitecompat;
> I would have just filed an issue there.

There is also a link saying "Improve this page" at the bottom of each page so feel free to send pull requests :)

Comment 15

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/48#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/__defineGetter__#Compatibility_notes
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/__defineSetter__#Compatibility_notes