203084 - Host properties getting lost on "simple" host classes

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Steven Cole]

Something has changed recently in the tip of the Spidermonkey source, and now
class-specific properties are getting somehow "lost".

When I make a JSClass that has only one property, with a property-specific
getter and setter pair, the property itself gets overridden the first time it's set.

In other words: a generic class:

static JSClass BusyIcon_Class =
{ "BusyIcon", 0,
  JS_PropertyStub,  JS_PropertyStub, JS_PropertyStub, JS_PropertyStub,
  JS_EnumerateStub, JS_ResolveStub,  JS_ConvertStub,  JS_FinalizeStub,
  JSCLASS_NO_OPTIONAL_MEMBERS
};

And one simple property:

static JSPropertySpec BusyIcon_Properties[] =
{
    { "x", ID_BusyIcon_x, JSPROP_ENUMERATE, BusyIcon_GetXPos, BusyIcon_SetXPos },
    { 0 }
};

When something gets this 'x' property, everything works fine.  But when someone
sets the 'x' property, things are turned off.  After instrumenting the add and
get callbacks, it looks like a new property is being created ('x') and stored in
the object, NOT using the tinyid. This new property is then just a standard JS
prop and doesn't cause the getter/setter callbacks to be used anymore.

I'll attach a diff to js.c at the end of this message that allows the test to be
compiled.

A simple script log looks like (my getters and setters are instrumented):

js> busyIcon
JS: Adding new BusyIcon object to 002F8B40 (as "busyIcon").
[object BusyIcon]
js> busyIcon.x
JS: BusyIcon_GetXPos --> 0
0
js> busyIcon.x = 3;
3
js> busyIcon.x
3
js>

Notice that the getter is called the first time, but not the second time.  The
setter is never called.

There is a workaround:  Setting JSPROP_SHARED on the properties makes everything
work just fine.  And it's arguably the right thing to do even if this bug is
fixed.  Still.  What's going on here shouldn't happen, I think.

--Steve Cole

Comment 1

[Steven Cole]

Attachment: Patch to JS shell to demonstrate problem

Patches js.c so that the problem can be tested in JS shell.

--scole

Comment 2

[Brendan Eich [:brendan]]

A regression from the fix for bug 90596.  I wasn't sure when patching that bug
how to preserve API compatibility, so I opted for a narrow interpretation: only
if the proto-prop has a short-id (tinyid) *and* the class default getter and
setter, do we propagate SPROP_HAS_SHORTID and sprop->shortid.  But that's too
narrow, because the API allows per-tinyid-bearing-properties to have their own
non-default getters and setters.

Patch in a minute, but I won't be around for more than an hour, then I'm off for
7 days.  Someone else will have to test, r= and check in.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 4

[Brendan Eich [:brendan]]

Attachment: fix the comment, too

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 121450 [details] [diff] [review]
fix the comment, too

Two reviews are better than one, but one's enough to get this patch in for
1.4b, with Asa's a=.  I'm off, see you in a week.

/be

Comment 6

[Steven Cole]

Unfortunately, this doesn't seem to fix the problem.  I get the same results as
before.

--scole

Comment 7

[Asa Dotzler [:asa]]

not gonna block for this.

Comment 8

[Brendan Eich [:brendan]]

Attachment: restore API compatibility

Ok, this fixes scole's testcase for me.  It clearly restores in the
SPROP_HAS_SHORTID case what the old code did in all cases of non-shared
property shadowing: copy the proto-property's getter and setter as well as its
shortid and SPROP_HAS_SHORTID flag.

/be

Comment 9

[Brendan Eich [:brendan]]

Patch works, restores needed API compatibility.  This needs to be fixed in 1.4
final if not in beta.

/be

Comment 10

[Mike Shaver (:shaver emeritus)]

Comment on attachment 122264 [details] [diff] [review]
restore API compatibility

OK, yeah. sr=shaver.

Comment 11

[Steven Cole]

Comment on attachment 122264 [details] [diff] [review]
restore API compatibility

Confirmed.  This patch fixes my testcase.  Thanks, Brendan!

--scole

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 122264 [details] [diff] [review]
restore API compatibility

Looking to get this in to avoid regressing a milestone release.

/be

Comment 13

[Asa Dotzler [:asa]]

Comment on attachment 122264 [details] [diff] [review]
restore API compatibility

a=asa (on behalf of drivers) for checkin to 1.4b.

Comment 14

[Brendan Eich [:brendan]]

Fixed, thanks to everyone.

/be

Comment 15

[Phil Schwartau]

Marking Verified FIXED based on Steve's Comment #11.

From my point of view, the fix passes the JS testsuite on WinNT,
Linux, and Mac OSX, in both the debug and optimized JS shell -

Comment 16

[Phil Schwartau]

Further verification of the fix. Here's what I'm seeing when I try
Steve's testcase (after applying his patch to js.c in Comment #1):

js> busyIcon
JS: Adding new BusyIcon object to 002FB340 (as "busyIcon").
[object BusyIcon]

js> busyIcon.x
JS: BusyIcon_GetXPos --> 0
0

js> busyIcon.x = 3;
JS: BusyIcon_SetXPos(3)
3

js> busyIcon.x
JS: BusyIcon_GetXPos --> 3
3