Remove legacy __defineGetter__/__defineSetter__ this behavior

RESOLVED FIXED in Firefox 48

Status

()

Core
JavaScript: Standard Library
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: evilpie, 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

(URL)

Attachments

(3 attachments)

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.
Assignee: nobody → evilpies
Keywords: site-compat
Created attachment 8729148 [details] [diff] [review]
Remove legacy __defineGetter__/__defineSetter__ this behavior
Attachment #8729148 - Flags: review?(till)
Created attachment 8729150 [details] [diff] [review]
Add steps to legacy functions and a new test
Attachment #8729150 - Flags: review?(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.
Attachment #8729148 - Flags: review?(till) → review+
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.
Attachment #8729150 - Flags: review?(till) → review+
Created attachment 8729703 [details] [diff] [review]
Fix a DOM test that uses legacy define behavior
Attachment #8729703 - Flags: review?(bzbarsky)
Comment on attachment 8729703 [details] [diff] [review]
Fix a DOM test that uses legacy define behavior

r=me
Attachment #8729703 - Flags: review?(bzbarsky) → review+
(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

a year ago
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bd22586cac82
https://hg.mozilla.org/mozilla-central/rev/16b05a0fb83a
https://hg.mozilla.org/mozilla-central/rev/7b19afd04625
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Keywords: dev-doc-needed
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/
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...
Flags: needinfo?(kohei.yoshino)

Updated

a year ago
Depends on: 1256182
Thanks Boris! Yes, the draft was misleading. Corrected as follows:
https://github.com/fxsitecompat/www.fxsitecompat.com/commit/a981f7c
Flags: needinfo?(kohei.yoshino)
Ah, excellent.  I hadn't realized there was a github repo for fxsitecompat; I would have just filed an issue there.
(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 :)
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.