656424 - setProperty function is never called

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Robin Ehrlich]

User-Agent:       Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0; GTB6; .NET CLR 1.1.4322; .NET CLR 2.0.50727; .NET CLR 3.0.04506.30; .NET CLR 3.0.04506.648; .NET CLR 3.5.21022; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729; OfficeLiveConnector.1.3; OfficeLivePatch.0.0; .NET4.0C)
Build Identifier: 1.8.5

I am converting an application for JS 1.7 to JS 1.8.5 and have encountered a 
serious problem. The setProperty function is never called. For a particular 
class, I have the following properties defined:

static JSPropertySpec cardProperties[] = {
   {"timeout", PROP_ID_TIMEOUT,
    JSPROP_ENUMERATE | JSPROP_PERMANENT,
    cardPropertyGetTimeout, cardPropertySetTimeout},
   {NULL, 0, 0, NULL, NULL}
};

In Javascript I use:

c = new Card();
i = c.timeout;
c.timeout = 10;

The get function is called, but not the set function.

I verified that the set function is called in JS 1.7.

The JSClass setter is called, but not the setter in the properties array.

Reproducible: Always

Comment 1

[Boris Zbarsky [:bzbarsky]]

What's the signature of your cardPropertySetTimeout function?  It's a bit odd that it's not being called at all; I'd not be surprised if it's being called incorrectly...

Comment 2

[Robin Ehrlich]

Attachment: A very simple test file demonstrating the problem

If you run this very simple test you will see that the constructor and the getter is called once, but the setter is never called.

Comment 3

[Robin Ehrlich]

The signature is:

static JSBool cardPropertySetTimeout(JSContext *cx, JSObject *obj, jsid id, JSBool strict, jsval *vp)

I have submitted a very simple test program that demonstrates the problem.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm.  Yeah, that should all work....  Are you just running pure interp with no jit?

Comment 5

[Robin Ehrlich]

I agree it should work, but it doesn't. Could someone please try running my simple test to verify? I am running on a WIN32 build with all the default options plus --enable-threadsafe --enable-trace-jscalls. I agree it is very strange that this feature is failing. I have run quite a few tests and the setter callback from the JSPropertySpec table is never called.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Does the problem go away if you take out the "JSOPTION_JIT | JSOPTION_METHODJIT" jscontext options?

Comment 7

[Robin Ehrlich]

> Does the problem go away if you take out the "JSOPTION_JIT | 
> JSOPTION_METHODJIT" jscontext options?

No. Exactly the same result.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Does the setter get called if you do not run the getter before that?  So in this code:

c = new Card();
c.timeout = 10;

Comment 9

[Robin Ehrlich]

If I run:

c = new Card();
c.timeout = 10;

the setter is still not called.

Comment 10

[Robin Ehrlich]

I am definitely not an expert on the Javascript engine, but the bug seems to be in js_SetPropertyHelper in jsobj.cpp. This function initially gets the setter from the class (jsobj.cpp:5623). At line 5683 the setter is reset from the shape where is property setter is stored, but only (!shape->hasSlot()). I think it should always get the setter from the shape.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Hmm.  So if you add JSPROP_SHARED for that property, do things work?

Comment 12

[Robin Ehrlich]

> Hmm.  So if you add JSPROP_SHARED for that property, do things work?
Yes, but I don't know what the side effects of adding JSPROP_SHARED are. I realize that this may be a workaround, but it seems to me to be a kludge solution.

Comment 13

[Jason Orendorff [:jorendorff]]

The engine is behaving correctly. Add JSPROP_SHARED if you want the property to behave like an ES5 accessor property. (That API oddity probably predates ES5 by ten years, so there's little we can do about it now.)

The consequences of adding JSPROP_SHARED are:

  - assigning to the property on an object that inherits the property from
    a prototype calls the setter (exactly the behavior you want)

  - no field is allocated for the property

  - "the shared permanent hack", a bit of obscure legacy oddness which you
    can ignore, since this property is not also JSPROP_PERMANENT.

Comment 14

[Jason Orendorff [:jorendorff]]

P.S. Thank you for providing a complete test case. I was able to figure out what was going on in a few minutes because of that. Otherwise this would never have been resolved.

Comment 15

[Robin Ehrlich]

Thank you for the help in resolving this problem. In order to make the conversion from JS 1.7 to JS 1.8.5, I would like to suggest that this change be documented in the 1.8.5 release notes and that the JSPropertySpec documentation specify when the setter is called.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Jason, the weird thing here is the behavior change and the fact that presumably scripted setters on the proto _do_ get run.... or do those get defined as JSPROP_SHARED?

Comment 17

[Jeff Walden [:Waldo]]

Scripted setters get defined with JSPROP_SHARED.

Comment 18

[Jason Orendorff [:jorendorff]]

That there was a behavior change is surprising to me. I thought it had always worked this way. Jeff, did we change something in the FF4 timeframe, maybe when straightening up ES5 accessor behavior?

Comment 19

[Jeff Walden [:Waldo]]

I'm not aware of any such change, but seemingly something did change, and that's as plausible a guess as anything.