Firefox bug at https://bugzilla.mozilla.org/show_bug.cgi?id=1301754

Franziska has been working on better-supporting Object.defineProperty when called on API objects.

I'll have a look. 

We actually don't abort in 2.2.2.1 but define the property. I guess if we aborted, we might throw?

> Object.getOwnPropertyDescriptor(x, 'nosuchpropanywhere')
< Object {set: undefined, enumerable: false, configurable: false}

bzbarsky, thanks for reporting this. The fix is easy (see https://codereview.chromium.org/2331223004), but it would break too many other things. 

Since Chrome, Safari, Edge, and Firefox all expose this behavior, I was hoping we could change the spec. Is a PR the right way to do this? 

What would it break?

Changing the spec is doable, but the resulting behavior is completely broken (it just uses the "value" from a non-value descriptor and converts it to a string!), so I don't see how anyone could be depending on it right now...

My fix would result in an exception in the Node.js interactive shell (REPL) every time defineProperty() with an accessor descriptor is called. Because REPL (or rather the vm module) defines NamedSetterCallbacks on the global object.

Why do we not allow to set properties on platform objects to accessor descriptors? Is that an artifact from when defineProperty() wasn't around? (Sorry, I'm not very familiar with Web IDL spec). 

> My fix would result in an exception in the Node.js interactive shell

That sounds like a Node-specific situation that shouldn't affect the behavior of Chrome.  There's no a priori reason changing Chrome's behavior here should require changing Node.js.

> defines NamedSetterCallbacks on the global object

You don't have to apply this behavior to any objects that are not WebIDL objects with named setters, obviously.  If you're applying it to other objects, that sounds like a problem with your fix...

> Why do we not allow to set properties on platform objects to accessor descriptors?

Because it's a nonsensical operation for an object with a named setter.  The whole point of a named setter is that all property definition is diverted to the named setter.  But it makes no sense to do that with a non-value descriptor.

Note that this only affects objects with named setters, which in practice means just DOMStringMap.

Now we could spec the "just use undefined" behavior, but I don't see how it would help authors any: instead of an exception that tells them their property did not get defined, they would just get silent failure to define it as they tried (and no functional getters or setters anyway, note!).

I would really like to make progress on this, but don't want to make changes to Gecko that Chrome isn't going to follow.  If you don't plan to implement the spec as written, have you raised a spec issue with your proposal for what the spec should say instead?

+yukishiino@ as he's been helping a lot in making our bindings more compliant.

I tend to agree with Boris that allowing this operation is nonsensical and that any Node.js-specific problems should be worked around, Node.js code isn't actually doing a defineProperty with an accessor descriptor on a DOM object, since there are no DOM objects in Node.js, so maybe we can fix e.g. by making it so the mechanism Node.js uses isn't conflated with the DOM (Web IDL) named setter mechanism.

re #11: IIUC, the step 2.2.2.1 shouldn't be applied if the object is [Global] or [PrimaryGlobal].  "legacy platform object" is not [Global] nor [PrimaryGlobal] by definition at all, right?  Then, your fix shouldn't be applied to the case of "REPL defines NamedSetterCallbacks on the global object." because it's the global object.

Do we really want to fix this? We would have different behavior than Firefox.

Firefox would like to change its behavior here, but not if we're the only ones doing it.  So if you did make this change you would not end up with different behavior from Firefox.

I should have taken much closer look one year ago.  I now understand the issue and solution clearly.

For indexed properties, Blink already implements "IsDataDescriptor(Desc) is false, then return false."
https://cs.chromium.org/chromium/src/third_party/blink/renderer/bindings/templates/interface.cpp.tmpl?l=340&rcl=32efca451ff8fcdecb2733d873d24a977ddf2b28

For named properties, Blink should use v8::GenericNamedPropertyDefinerCallback where we can tell whether the property being defined is an accessor property or data property.  Issue 764633 is related to use of "definer callback".

Once we become to use "definer callback", then this issue will be fixed with a pretty straightforward way.  I don't see difficulty on this issue.  Of course, Blink-local change shouldn't affect Node.js.

rakuco@, are you interested in this issue and/or Issue 764633 ?  If you're busy with other projects, I will try to find time for this issue.

I'd like to work on that one; I think the change is pretty straightforward, the problem is that we've been blocked on https://bugs.chromium.org/p/v8/issues/detail?id=7612 in V8 for a while now.

I see.  Let's fix V8 first.
Let me assign this issue to rakuco@ for the time being.