Last Comment Bug 1253016 - Remove legacy __defineGetter__/__defineSetter__ this behavior
: Remove legacy __defineGetter__/__defineSetter__ this behavior
Status: RESOLVED FIXED
: addon-compat, dev-doc-complete, site-compat
Product: Core
Classification: Components
Component: JavaScript: Standard Library (show other bugs)
: unspecified
: Unspecified Unspecified
-- normal (vote)
: mozilla48
Assigned To: Tom Schuster [:evilpie]
:
: Jason Orendorff [:jorendorff]
Mentors:
https://github.com/tc39/ecma262/pull/381
Depends on: 1249123 1256182
Blocks: es7
  Show dependency treegraph
 
Reported: 2016-03-02 13:07 PST by Tom Schuster [:evilpie]
Modified: 2016-04-01 14:22 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Remove legacy __defineGetter__/__defineSetter__ this behavior (7.16 KB, patch)
2016-03-10 11:57 PST, Tom Schuster [:evilpie]
till: review+
Details | Diff | Splinter Review
Add steps to legacy functions and a new test (7.35 KB, patch)
2016-03-10 11:58 PST, Tom Schuster [:evilpie]
till: review+
Details | Diff | Splinter Review
Fix a DOM test that uses legacy define behavior (1.09 KB, patch)
2016-03-11 13:57 PST, Tom Schuster [:evilpie]
bzbarsky: review+
Details | Diff | Splinter Review

Description User image Tom Schuster [:evilpie] 2016-03-02 13:07:08 PST
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 User image Tom Schuster [:evilpie] 2016-03-10 11:57:56 PST
Created attachment 8729148 [details] [diff] [review]
Remove legacy __defineGetter__/__defineSetter__ this behavior
Comment 2 User image Tom Schuster [:evilpie] 2016-03-10 11:58:50 PST
Created attachment 8729150 [details] [diff] [review]
Add steps to legacy functions and a new test
Comment 3 User image Till Schneidereit [till] 2016-03-10 12:44:42 PST
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 User image Till Schneidereit [till] 2016-03-10 12:49:46 PST
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 User image Tom Schuster [:evilpie] 2016-03-11 13:57:03 PST
Created attachment 8729703 [details] [diff] [review]
Fix a DOM test that uses legacy define behavior
Comment 6 User image Boris Zbarsky [:bz] (still a bit busy) 2016-03-11 21:19:43 PST
Comment on attachment 8729703 [details] [diff] [review]
Fix a DOM test that uses legacy define behavior

r=me
Comment 7 User image Tom Schuster [:evilpie] 2016-03-12 05:48:40 PST
(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 11 User image Boris Zbarsky [:bz] (still a bit busy) 2016-03-12 18:14:31 PST
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 User image Kohei Yoshino [:kohei] 2016-03-14 01:44:21 PDT
Thanks Boris! Yes, the draft was misleading. Corrected as follows:
https://github.com/fxsitecompat/www.fxsitecompat.com/commit/a981f7c
Comment 13 User image Boris Zbarsky [:bz] (still a bit busy) 2016-03-14 07:33:46 PDT
Ah, excellent.  I hadn't realized there was a github repo for fxsitecompat; I would have just filed an issue there.
Comment 14 User image Kohei Yoshino [:kohei] 2016-03-14 07:38:17 PDT
(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 :)

Note You need to log in before you can comment on or make changes to this bug.