Closed Bug 137000 Opened 18 years ago Closed 18 years ago

object member variables not set correctly from parameters passed to parent


(Core :: JavaScript Engine, defect, P1)






(Reporter: ben, Assigned: brendan)




(Keywords: js1.5)


(2 files, 2 obsolete files)

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i386; en-US; rv:0.9.9+) Gecko/20020411
BuildID:    2002041108

When passing parameters to a parent object via in
the child object constructor, member variables with the same name as the
parameter is forced to a value of undefined even if the parameter is not used to
set the member variable with its name.

Reproducible: Always
Steps to Reproduce:
1. Go to the URL,

Actual Results:  The first two fields on the page are set to undefined

Expected Results:  They should be set to values of "passed to matchAndSet" and
"match, but set inside".

This bug has also been verified for win32 build 2002041103, but did not exist
when using mozilla 0.9.7, so some change since then must be the cause.
Attached file Reduced HTML testcase
Confirming bug and cc'ing Brendan. Here is the reduced testcase:

function parentObject(p)
  this.p = 1;

function childObject()
childObject.prototype = parentObject;

var o = new parentObject();
document.write('<br>o = new parentObject():<br>o.p = ' + o.p + '<br>');

o = new childObject();
document.write('<br>o = new childObject():<br>o.p = ' + o.p + '<br>');

In NN4.7 and IE6, the output is:
o = new parentObject():
o.p = 1

o = new childObject():
o.p = 1

Mozilla trunk binaries 20020411xx WinNT, Linux:
o = new parentObject():
o.p = 1

o = new childObject():
o.p = undefined

As Ben says, the problem is the use of the identifier 'p' for both a
property name in the parent, and parameter to the parent constructor.
If we change the parameter name to anything other than 'p', the output
in Mozilla is the same as in NN4.7 and IE6.
Assignee: rogerl → khanson
Ever confirmed: true
OS: Linux → All
Keywords: js1.5
Here is an even simpler testcase of the same problem, without the .call(). 

function foo(arg) {};
foo.arg = 12;

the alert should display "12" but instead displays "undefined". The correct 
behavior occurs when using different names for the argument(s) to foo() and 
properties of that object.
Here's a simple fix, though I want to look more at the interactions with the
rest of our argument/property machinery.  I won't get a chance to do a test run
for a little while, but I'll try to get one tonight.
*** Bug 138708 has been marked as a duplicate of this bug. ***
Comment on attachment 85623 [details] [diff] [review]
Fall back to getting/setting on the function object if no live argv is found.

Can't do that, OBJ_GET_PROPERTY and all JSObjectOps take a jsid id, not a jsval
id.  You'll recurse to death, too.

Attachment #85623 - Flags: needs-work+
Testcase added to JS testsuite:



1. The patch in Comment #4 fixes the test given in Comment #3,
   but it does not fix the original test given in Comment #2.

2. The RC4 release of SpiderMonkey (01 Jan 2002) passes all the 
   tests above, plus Rustam's test from bug 138708.

3. The RC4a release fails in the same manner as the current trunk.
   So something changed between RC4 and RC4a (21 March 2002).

4. Possible candidate: the fix for bug 131510? 
   "Crash when |arguments| defined as a variable inside function"

5. At any rate, here is a quick change log for RC4a vs. RC4:
Wanted for 1.0.1 and (if I get my act together tonight) 1.1alpha.

Assignee: khanson → brendan
Keywords: mozilla1.0.1
Priority: -- → P1
Target Milestone: --- → mozilla1.0.1
This should wait till the 1.1beta trunk opens, bake a day or so, and then go
into the 1.0 branch for 1.0.1.

Attachment #85623 - Attachment is obsolete: true
Comment on attachment 86378 [details] [diff] [review]
"Unshare" arg or local-var on set, if JSPROP_SHARED

Attachment #86378 - Flags: superreview+
In reply to phil's comment #7, the regression here came in with the patch for
bug 62164.  That patch changed args and local vars in a function object to have
the JSPROP_SHARED attribute, which means that f.a = 42 stores no persistent 42
in a slot in the object denoted by f.  The patch to fix this bug, attachment
86378 [details] [diff] [review], preserves the economy of default-shared arg and local var properties, but
detects the f.a = 42 case where f is not active, and morphs f.a into an unshared
property with its own slot in which to store 42.

*** Bug 150032 has been marked as a duplicate of this bug. ***
Have just added a new section to the above testcase, by, from the duplicate bug 150032.

The new section tests what happens if a local var in a function 
uses the same name as a property of the function. This complements
the rest of the testcase, which considers the problem of a 
function parameter having the same name as a function property.

All sections of the testcase are currently passing in Rhino.
In SpiderMonkey I get failures even with the latest patch applied:

WITHOUT PATCH                   WITH PATCH
Sections 1,3,5,6 fail           Sections 3,5,6 fail
Phil, sections 3, 5, and 6 test the old-as-Mocha (1995) feature of SpiderMonkey
whereby f.a = x in a function f that has an arg or local var named a will set
that arg or local var, but not any property of the function.  I suppose it's
time we broke that bit of compatibility.  New patch in a few,

Attached patch better fixSplinter Review
This rips out the pre-ECMA "extension" (more a side-effect of an implementation
detail) where function f(a){return f.a}; f.a = 52; print(f(42)) would print 42
instead of 52.	It preserves the expectations of the rest of the engine that
args and vars are stored as properties.

Attachment #86378 - Attachment is obsolete: true
Most recent patch fails test case in comment #3.
khanson: it works for me:

[~/src/trunk-opt/mozilla/js/src]$ Linux_All_DBG.OBJ/js
js> function f(a){}
js> f.a=12
js> f.a

Are you sure you applied the patch and recompiled properly?

Comment on attachment 86950 [details] [diff] [review]
better fix

sr=shaver.  You're right, it _is_ time.
Attachment #86950 - Flags: superreview+
I got my files mixed up.  It works for me.
Most recent patch passes all sections of the above testcase.
I also ran the full JS testsuite against it in the optimized
JS shell on WinNT, and got no regressions -
Comment on attachment 86950 [details] [diff] [review]
better fix

r = khanson.  Looks fine.
Attachment #86950 - Flags: review+
Fix is in the trunk, going for drivers approval for the 1.0 branch.

This is crashing an optimized mozilla win32 trunk build pulled this evening. 
I've backed out and reapplied this patch a couple of times, and with the patch
I crash, without I run fine. The crash is in function 'Variables' of jsparse.c.
I'll file a separate bug with more detail in a moment (or is this already a 
known issue).
Filed bug 151066 for the crash.
Blocks: 149801
Depends on: 151066
Note the above patch has been backed out of the trunk.
See bug 151066:

------- Additional Comment_ #2 From John Morrison 2002-06-12 02:05 ------- 

Brendan backed out rev 3.105 of jsinterp.c which avoids the crash
in my win32 opt. build, pending further investigation.
*** Bug 150859 has been marked as a duplicate of this bug. ***
Comment on attachment 86950 [details] [diff] [review]
better fix

a=asa (on behalf of drivers) for checkin to 1.1
Attachment #86950 - Flags: approval+
Fixed on branch and trunk.  Thanks again, jrgm and rpotts.

Keywords: mozilla1.0.1fixed1.0.1
Fixed, I say!

Closed: 18 years ago
Resolution: --- → FIXED
Verified Fixed.

The above testcase passes in the debug/optimized JS shell on WinNT,
Linux, and Mac9.1. Tested on both the trunk and -rMOZILLA_1_0_BRANCH.
Keywords: verified1.0.1
Flags: testcase+
You need to log in before you can comment on or make changes to this bug.