Bug 142934 – __defineGetter__/__defineSetter__ should throw exceptions

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Joseph Pecoraro]

* SUMMARY
__defineGetter__/__defineSetter__ should throw exceptions.

* TEST
debug("__defineGetter__ and __defineSetter__ should throw exceptions when acting on sealed objects");
var o14 = {a:14};
Object.seal(o14);
shouldThrow("o14.__defineGetter__('a', function(){})");
shouldThrow("o14.__defineGetter__('b', function(){})");
shouldThrow("o14.__defineSetter__('a', function(){})");
shouldThrow("o14.__defineSetter__('b', function(){})");

debug("__defineGetter__ and __defineSetter__ should throw exceptions when acting on frozen objects");
var o15 = {a:15};
Object.freeze(o15);
shouldThrow("o15.__defineGetter__('a', function(){})");
shouldThrow("o15.__defineGetter__('b', function(){})");
shouldThrow("o15.__defineSetter__('a', function(){})");
shouldThrow("o15.__defineSetter__('b', function(){})");

* NOTES
- WebKit fails silently
- Firefox throws exceptions
- Chrome fails silently

I think throwing an exception makes more sense. It at least lets the developer know something went wrong. I don't think there is any specification for these functions.

[Joseph Pecoraro]

Created attachment 249180 [details]
[PATCH] Proposed Fix

This is a potential breaking change, but I think it is for the better.

1. Non-configurable properties are rare.
2. Another browser (Firefox) already has this behavior.
3. This has the potential to help catch an issue where an error would happen silently.

[Geoffrey Garen]

Comment on attachment 249180 [details]
[PATCH] Proposed Fix

r=me

Would be nice to put that true into a local variable named "shouldThrow", so that it's clear at the call site what it means.

[Joseph Pecoraro]

http://trac.webkit.org/changeset/181868

[Joseph Pecoraro]

This caused Bug 144373.

I'm going to roll-out and re-approach this later after making IDL properties configurable, which in and of itself is a large enough change to make and see if it has fallout.

[Joseph Pecoraro]

See Bug 144373 for how this broke live.com.

Lets try re-enabling this after the following have been fixed:
<https://webkit.org/b/122213> WebIDL operations should be configurable
<https://webkit.org/b/134364> DOM attributes on prototypes should be configurable

[Geoffrey Garen]

Note: Also broke slack.com and Slack.app (<rdar://problem/20807563>).

[Joseph Pecoraro]

Blocker now fixed. I will retest this with Slack.app / live.com and see if we can re-land it.

[Joseph Pecoraro]

live.com issue no longer happens, it boiled down to:

    HTMLElementPrototype.__defineGetter__("innerText", function() { ... });

slack.com issue still exists, it boils down to:

    window.__defineGetter__("onhashchange", function() { ... });

Seems these "onfoo" properties are still not configurable but probably should be. I'll file a new bug and track that.

[Benjamin Poulain]

Created attachment 282516 [details]
Patch

[Mark Lam]

Comment on attachment 282516 [details]
Patch

r=me.

Has the slack.com compatibility issue outlined in https://bugs.webkit.org/show_bug.cgi?id=142934#c8 been resolved?

[Benjamin Poulain]

Comment on attachment 282516 [details]
Patch

(In reply to comment #10)
> Comment on attachment 282516 [details]
> Patch
> 
> r=me.
> 
> Has the slack.com compatibility issue outlined in
> https://bugs.webkit.org/show_bug.cgi?id=142934#c8 been resolved?

I believe so. One of the tests cover this particular case.

[WebKit Commit Bot]

Comment on attachment 282516 [details]
Patch

Clearing flags on attachment: 282516

Committed r202755: <http://trac.webkit.org/changeset/202755>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.