72354 - js_SetProperty needs to resolve lazy setters

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[John Bandhauer]

brendan and I have discussed this issue. I'm filing a bug to track it. this 
turns out to be a blocker for landing xpconnect flattening on trunk (an 
activity I'm not yet ready to do, BTW).


.... jband wrote ....

I'm having problems with setters on protos and js_SetProperty.

The situation I have is with a lazily resolved setter on a
prototype. The code in js_SetProperty does not do anything that
forces a resolve as it walks the proto chain:
http://lxr.mozilla.org/seamonkey/source/js/src/jsobj.c#2295

So, I end up with a situation where if the property with a
getter/setter has been previously resolved on the proto then a
set of the property on the object will use the proto's setter and
all is well. But, if that property has not been previously
resolved on the proto then js_SetProperty will fail to discover
the setter on the proto and just add a plain old property to my
object.

I'm not seeing a resolve on the target object either for that
matter.

Is js_SetProperty wrong to not force a resolve on the objects in
the proto chain? Can you suggest what I might do to get around
this? Am I just missing something?

It appears that I might get away with some hackery in my
JSClass.setProperty callback to patch the getter/setter into
place (and call it) before returning. Is this my best option?

Thanks,

John.

.... brendan replied ....

ECMA clearly specifies that a [[Put]] creates a property in the object 
at hand, not in one of its prototypes (8.6.2.2, 8.6.2.3).  We violate 
that a bit by inheriting getter, setter, and attrs if a shadow-able 
prot-property of the same id exists; see the XXX comments in 
js_SetProperty.  I don't think ECMA users can tell the difference in our 
impl, except where magic native getter and setter hooks are used -- and 
then we may be fooling Mother ECMA so that the difference (an 
optimization in our impl) is not detectable.  I'm not sure -- I haven't 
spent much time trying, and have not constructed a case that violates 
ECMA detectably.

Given that, we could call resolve in js_SetProperty.  Want a patch?

/be

Comment 1

[John Bandhauer]

As I noted in private mail to brendan...

Hmm. I realise now that it isn't just a question of looking at
the prototype. I really need it to try a resolve on the target
object (just do a js_LookupProperty?). This is because I have a
case where the (as yet unresolved) setter is on the target
object, not the proto. So, by not forcing a resolve, we fail to
discover the lazily resolved setter and try to write a new
property in its place.

It seems like this whole function can get simpler by starting
with a js_LookupProperty call.

[brendan replied to an earlier message expressing a dislike of adding too much 
cost to js_SetProperty. I don't see how we can have lazy setters without doing 
most of what js_LookupProperty does here.]

Comment 2

[Brendan Eich [:brendan]]

jband: consider the watchpoint-restoring code -- it needs a symbol that has a
null entry.value (sprop is null).  Hard to discern by calling js_LookupProperty.
I'm hacking on a patch right now.

/be

Comment 3

[John Bandhauer]

[This remains a blocker that must be resolved before landing the xpcdom branch]

I changed the summary to reflect the fact that this is not just a proto issue - 
we need to (potentially) resolve starting with the target object and then on up 
the proto chain to find a lazy setter.

Comment 4

[Brendan Eich [:brendan]]

Pushing out, but jband and jst need this to land the xpcdom branch, so I'll fix
it ASAP.

/be

Comment 5

[Brendan Eich [:brendan]]

Patch coming up includes the patch in bug 70358, which I still want shaver to
review.  If you ignore the jsapi.c and jscntxt.h changes, and skip the
js_LookupProperty part of the jsobj.c patch, then you can review independently.
I didn't want to decouple the patches into two trees (I have several trees, too
full of patches already -- but I really wanted to think about and work on these
two bugs in the same tree).

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: proposed fix, per previous comment -- pls. help test and review

Comment 7

[Brendan Eich [:brendan]]

I decided to bite the bullet and call js_LookupProperty from js_SetProperty,
what a concept.  The runtime cost should not be terrible given the property
cache and other costs, but I haven't measured.  The main complexity lies in
dealing with the JS_THREADSAFE-only "hold" (sprop->nrefs++) done by
js_LookupProperty on the *propp out param it returns.

I'm keeping this case JS_THREADSAFE-only, but of course *any* call-out to a
setter needs to be preceded by a hold and followed by a drop, in case of delete.
The old js_SetProperty code did this, but knew it didn't have a hold already in
the JS_THREADSAFE case (because it did raw scope->lookup calls to get symbols,
it did not js_LookupProperty).

To help catch sprop->nrefs underflows, I added an assertion in
js_DropScopeProperty on the last-drop case that the property's symbol-id is no
longer in scope.  This required revising jshash.c (divergence from NSPR1, first
time in years?) to unlink entries from hash chains as it destroys a table, so
that they can't be found by freeEntry callback code.

Man, I need to nuke all this symbol and chain crap with JS_DOUBLE_HASHING soon!

jst: help test and I'll pay you back with tequila!

/be

Comment 8

[Johnny Stenback (:jst)]

Ooh, tequila, how could I saw no? Brendan, you have yourself a deal!

I've applied your patches and done some initial testing, so far so good. I'll
report back if I find problems after testing some more.

Comment 9

[Johnny Stenback (:jst)]

I tested this some more, and it appears to be working just fine, I've made sure
setters are properly resolved up the proto chain on both XPConnect'ed objects
and on pure JS objects, and I've seen no bad side effects caused by this patch.

Comment 10

[Brendan Eich [:brendan]]

I'd already run mozilla, xpcshell, and the js testsuite before attaching that
patch, but I had not tested watchpoints.  The patch forgot to hold sprop when
reconnecting it to its symbol, in the "set of a deleted, watched property" case
in the #if JS_HAS_OBJ_WATCHPOINT block near the top of js_SetProperty.  Patch to
fix that coming up.

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: Fix deleted, watched set-property case (one line change from last patch; the other line that changed was just a comment nit-pick).

Comment 12

[John Bandhauer]

I applied and ran mozilla and the first thing it did was botch an assert in 
js_DropScopeProperty (the one: JS_ASSERT(!scope->ops->lookup...).

I'll attach the stack.

Comment 13

[John Bandhauer]

Attachment: assertion location

Comment 14

[John Bandhauer]

I see that the addition of this assertion is your only change to jsscope.c. hmm.

Comment 15

[Brendan Eich [:brendan]]

argh, my testing was all optimized (presumably so was jst's).  That assertion
botches wrongly when an existing property is redefined.  New patch (just to
jsscope.c) coming up.

/be

Comment 16

[Johnny Stenback (:jst)]

Yes, I was running an optimized build too :-(

Comment 17

[Brendan Eich [:brendan]]

Correction: my mozilla and xpcshell testing was all optimized.  I run the js
testsuite on a debug js shell.  Now for that patch....

/be

Comment 18

[Brendan Eich [:brendan]]

Attachment: help the js_DropScopeProperty assertion (jsscope.c patch only, replaces patch for that file in earlier attachments)

Comment 19

[Brendan Eich [:brendan]]

Reviewing and testing buddies, please bless the revised aggregate patch soon.  I
really need to get on with JS_DOUBLE_HASHING (more evidence: JSSymbol.scope is
unused! a waste of four bytes per JSSymbol, which itself is a waste).

/be

Comment 20

[Phil Schwartau]

I don't know what I might be doing wrong, but I can't compile the 
JS shell on WinNT after applying the two patches above.


Method: 

1. pull js/src into a fresh directory
2. apply patch id=34383
3. delete jsscope.c
4. re-pull jsscope.c
5. apply patch id=34464
6. make -f Makefile.ref


I get a warning when jsobj.c is being compiled:

jsobj.c(2399) : warning C4013: 'HOLD_SCOPE_PROPERTY' undefined; 
                assuming extern returning int



Soon after this, the build fails:

   Creating library WINNT4.0_DBG.OBJ/js32.lib and object 
WINNT4.0_DBG.OBJ/js32.exp
jsobj.obj : error LNK2001: unresolved external symbol _HOLD_SCOPE_PROPERTY
WINNT4.0_DBG.OBJ/js32.dll : fatal error LNK1120: 1 unresolved externals
make[1]: *** [WINNT4.0_DBG.OBJ/js32.dll] Error 96
make: *** [all] Error 2

Comment 21

[John Bandhauer]

I dunno Phil. It builds for me. Does your jsscope.h not have a #define of 
HOLD_SCOPE_PROPERTY ? It's in the id=34383 patch.

Comment 22

[John Bandhauer]

brendan: I've been reading and rereading a side-by-side diff of js_SetProperty. 
I pretty much got it. I can't see anything wrong. I'm kinda lost on the 
watchpoint changes. I had fun tracing through the lock mgmt!

Most of the rest I reviewed elsewhere. As is so often the case: I have to trust 
you got it right because I can't see anything that might be wrong.

r/sr=jband

I'm hoping shaver will give it a thorough once over too.

Comment 23

[Johnny Stenback (:jst)]

I've been running a debug build with these patches and I don't see any problems,
no assertions or anything caused by these changes.

Comment 24

[Phil Schwartau]

jband: you were right, for some reason my jsscope.h was not affected
by the patch, even though my console output said that is was:

[/d/JS_EXP/mozilla/js/src] patch < 72354_34383.patch
patching file `jsapi.c'
patching file `jscntxt.h'
patching file `jshash.c'
patching file `jsobj.c'
patching file `jsscope.c'
patching file `jsscope.h'


ALTHOUGH IT SAYS, "patching file `jsscope.h'", IT DID NOTHING TO IT:

[/d/JS_EXP/mozilla/js/src] grep HOLD_SCOPE_PROPERTY jsscope.h

[/d/JS_EXP/mozilla/js/src] cvs stat jsscope.h
===================================================================
File: jsscope.h         Status: Up-to-date
   Working revision:    3.15
   Repository revision: 3.15    /cvsroot/mozilla/js/src/jsscope.h,v
   Sticky Tag:          (none)
   Sticky Date:         (none)
   Sticky Options:      (none)

[/d/JS_EXP/mozilla/js/src] cvs diff -rHEAD jsscope.h


COMPARE: IT DID DO THE RIGHT THING TO jsapi.c:

[/d/JS_EXP/mozilla/js/src] cvs stat jsapi.c
===================================================================
File: jsapi.c           Status: Locally Modified

   Working revision:    3.100
   Repository revision: 3.100   /cvsroot/mozilla/js/src/jsapi.c,v
   Sticky Tag:          (none)
   Sticky Date:         (none)
   Sticky Options:      (none)



I don't mean to sidetrack this bug, but has anyone ever heard of this?
I've seen patch fail before, but a SILENT failure like this one?
I had no idea that jsscope.h had not been patched...

Was this a Linux patch file needing its line terminators modified
in order to work correctly on Windows?

Comment 25

[Brendan Eich [:brendan]]

Fix is in, shaver can bless me after the fact.

/be

Comment 26

[Phil Schwartau]

Marking Verified -