686979 - Native setter not called when JS getter defined (assigning to read-only DOM properties silently fails)

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

Attachment: patch v1

PConnect sets a native setter on readonly attributes that throws a TypeError. I wasn't seeing the TypeError, so mrbkap and I dug in.

When we have a JS getter but no JS setter, we throw an error in strict mode. However, this causes problems in two cases:
1 - If we had a bonafide native setter, we'd throw an error when we shouldn't.
2 - In non-strict mode, we bypass the XPConnect TypeError throw, causing foo.readOnlyProperty = bar to fail silently.

Fix attached. Flagging Waldo for review.

Comment 1

[Jeff Walden [:Waldo]]

Could you add a testcase to this, that fails before/passes after, so I have a better idea what exactly the problem is being fixed?  I feel like I'm guessing slightly to connect the dots based on comment 0 and the contents of the patch.  Plus, if you want a particular behavior here, you presumably want it to not break in the future, which a test will guarantee.

Comment 2

[Bobby Holley (:bholley)]

The test coverage for this is up for review over at bug 684327.

This is where we'd like it to throw when we assign properties:
https://github.com/bholley/mozilla-central/blob/xpc_test/js/src/xpconnect/tests/unit/test_attributes.js#L76

This is the relevant IDL:
https://github.com/bholley/mozilla-central/blob/xpc_test/js/src/xpconnect/tests/idl/xpctest_attributes.idl#L48

This is the native setter we want to fire:
http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcwrappednativejsops.cpp#497

This is the definition of the above:
http://mxr.mozilla.org/mozilla-central/source/js/src/jsobj.cpp#7023

Comment 3

[Jeff Walden [:Waldo]]

I believe the relevant language in WebIDL for how to interpret |readonly attribute foo| is this:

http://dev.w3.org/2006/webapi/WebIDL/#es-attributes

According to this, the property will be represented by a property descriptor of the form { [[Get]]: G, [[Set]]: S, [[Enumerable]]: true, [[Configurable]]: configurable }.  G will be a function object performing get-functionality.  And regarding S, "The attribute setter is undefined if the attribute is declared readonly and has neither a [PutForwards] nor a [Replaceable] extended attribute declared on it."  So I think we want to get rid of the XPConnect-specific hackery here and just use JS_PropertyStub in DefinePropertyIfFound, and kill js_GetterOnlyPropertyStub.  Or am I missing something here?

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 560470 [details] [diff] [review]
patch v1

Marking arr- partly because WebIDL wants different behavior, partly because it's TLAPD.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Jeff Walden (remove +bmo to email) from comment #3)
> Or am I missing something here?

You're correct. In fact, WebIDL is more specific about it below the section you quoted:

> Note that attempting to assign to a property corresponding to a read only attribute
> results in different behavior depending on whether the script doing so is in strict
> mode. When in strict mode, such an assignment will result in a TypeError being
> thrown. When not in strict mode, the assignment attempt will be ignored.

I've filed bug 688017 to remove js_GetterOnlyPropertyStub. Patch for that coming right up.

However, I believe that the patch in this bug is still correct. We may or may not want to use native property stubs in different places. But when we do, we want them to actually execute in consistent fashion. The current logic here for executing a native setter depends on whether a JS getter is defined, which seems wrong. Re-requesting review.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 560470 [details] [diff] [review]
patch v1

Review of attachment 560470 [details] [diff] [review]:
-----------------------------------------------------------------

I am somewhat persuaded by what you say.  At the same time, I'm not completely certain this isn't intentional.  I half-remember a case recently that seems like it was similar to this (reported by a JSAPI user in IRC or a newsgroup?  ash maybe?  memory very hazy), where the semantics that seemed initially to make sense weren't the ones we wanted.  Hopefully Brendan will remember what I'm thinking of.  If not, I'm sure he can say whether the current semantics were intended better than I can.

::: js/src/jsscopeinlines.h
@@ +302,5 @@
>          js::Value fval = setterValue();
>          return js::InvokeGetterOrSetter(cx, obj, fval, 1, vp, vp);
>      }
>  
> +    if (attrs & JSPROP_GETTER && setterOp() == NULL)

hasDefaultSetter() rather than null-checking here, please.

Comment 7

[Bobby Holley (:bholley)]

So, the consensus on IRC seems to be that this patch is correct. However, if it lands, we'll start throwing in non-strict mode due to the use of js_GetterOnlyPropertyStub.

We want to remove that over in bug 688017, but it's not totally clear how it should look, because there's a pending rewrite to make everything work as JS getters/setters. So I'm going to stop driving these issues for now.

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 560470 [details] [diff] [review]
patch v1

Brendan didn't have an opinion, and Waldo said his r+ carried.

Comment 9

[Tom S. (please needinfo tschuster)]

Comment on attachment 560470 [details] [diff] [review]
patch v1

Review of attachment 560470 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsscopeinlines.h
@@ +302,5 @@
>          js::Value fval = setterValue();
>          return js::InvokeGetterOrSetter(cx, obj, fval, 1, vp, vp);
>      }
>  
> +    if (attrs & JSPROP_GETTER && setterOp() == NULL)

I would usually write 
  if ((attrs & JSPROP_GETTER) && hasDefaultSetter())