Bug 142934 - __defineGetter__/__defineSetter__ should throw exceptions
Summary: __defineGetter__/__defineSetter__ should throw exceptions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on: 149474 122213 134364
Blocks:
  Show dependency treegraph
 
Reported: 2015-03-21 00:08 PDT by Joseph Pecoraro
Modified: 2016-07-01 13:56 PDT (History)
9 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (6.58 KB, patch)
2015-03-21 17:38 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
Patch (15.86 KB, patch)
2016-07-01 00:09 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2015-03-21 00:08:35 PDT
* 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.
Comment 1 Joseph Pecoraro 2015-03-21 17:38:27 PDT
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.
Comment 2 Geoffrey Garen 2015-03-23 10:58:55 PDT
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.
Comment 3 Joseph Pecoraro 2015-03-23 13:50:28 PDT
http://trac.webkit.org/changeset/181868
Comment 4 Joseph Pecoraro 2015-04-28 20:30:30 PDT
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.
Comment 5 Joseph Pecoraro 2015-04-28 20:39:56 PDT
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
Comment 6 Geoffrey Garen 2015-05-07 11:36:12 PDT
Note: Also broke slack.com and Slack.app (<rdar://problem/20807563>).
Comment 7 Joseph Pecoraro 2015-09-22 13:17:07 PDT
Blocker now fixed. I will retest this with Slack.app / live.com and see if we can re-land it.
Comment 8 Joseph Pecoraro 2015-09-22 14:20:55 PDT
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.
Comment 9 Benjamin Poulain 2016-07-01 00:09:51 PDT
Created attachment 282516 [details]
Patch
Comment 10 Mark Lam 2016-07-01 09:45:46 PDT
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?
Comment 11 Benjamin Poulain 2016-07-01 13:34:46 PDT
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.
Comment 12 WebKit Commit Bot 2016-07-01 13:56:22 PDT
Comment on attachment 282516 [details]
Patch

Clearing flags on attachment: 282516

Committed r202755: <http://trac.webkit.org/changeset/202755>
Comment 13 WebKit Commit Bot 2016-07-01 13:56:27 PDT
All reviewed patches have been landed.  Closing bug.