Bob Clary [:bc] (inactive) Reporter
	Description • 19 years ago

when running in the browser ecma_3/Function/scope-001.js and ecma_3/Function/scope-002.js fail section G with Expected value 'false,true', Actual value 'false,false' while they pass in the shell.

function f() {}
window.hasOwnProperty('f') is true in 1.7.12, but false for 1.8 and 1.9 since bug 296639.

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 19 years ago

Is this something that we should be fixing on the 1.8 branch?

	Brendan Eich [:brendan]
	Comment 2 • 19 years ago

Yes, this ought to be fixed.  If we had noticed this sooner (thanks bc for noticing it at all!) we would have fixed it long ago.  One consolation: at top level, the workaround for the example in comment 0 is just hasOwnProperty('f'). That should work (please say if it does not).
 
/be

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 3 • 19 years ago

This could probably be fixed in a similar fashion to bug 310742 (which, IMO, makes sense for this case).

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 5 • 19 years ago

Comment on attachment 203539 [details] [diff] [review]
Wrong file.

Oops, this is the wrong file.

	Bob Clary [:bc] (inactive) Reporter
	Comment 7 • 19 years ago

(In reply to comment #2)
> One consolation: at top level, the workaround for the example in comment 0 is 
> just hasOwnProperty('f'). That should work (please say if it does not).

function f() {}
hasOwnProperty('f')
false

	Brendan Eich [:brendan]
	Comment 8 • 19 years ago

Comment on attachment 203540 [details] [diff] [review]
Something like this

Do this stuff:

>+    clasp = OBJ_GET_CLASS(cx, obj);
>+    xclasp = (clasp->flags & JSCLASS_IS_EXTENDED)
>+             ? (JSExtendedClass *)clasp
>+             : NULL;

After a brace and indent step following this else:

>+    } else if (xclasp && xclasp->outerObject &&
>+               xclasp->outerObject(cx, obj2) == obj) {
>+        *rval = JSVAL_TRUE;
>     } else if (OBJ_IS_NATIVE(obj2)) {
>         sprop = (JSScopeProperty *)prop;
>         *rval = BOOLEAN_TO_JSVAL(SPROP_IS_SHARED_PERMANENT(sprop));
>     } else {
>         *rval = JSVAL_FALSE;
>     }

Moving everything to here over.

r=me with that.

/be

	Brendan Eich [:brendan]
	Comment 9 • 19 years ago

(In reply to comment #7)
> (In reply to comment #2)
> > One consolation: at top level, the workaround for the example in comment 0 is 
> > just hasOwnProperty('f'). That should work (please say if it does not).
> 
> function f() {}
> hasOwnProperty('f')
> false

D'oh.  Our |this| binding code outerizes the inner object in whicih hasOwnProperty is found.  Of course.  This bug has no workaround.

/be

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 10 • 19 years ago

Fix checked into trunk.

	Bob Clary [:bc] (inactive) Reporter
	Comment 11 • 19 years ago

verified fixed on cvs debug trunk builds 2005-11-23 on windows/linux.

	Daniel Veditz [:dveditz]
	Comment 12 • 19 years ago

Comment on attachment 203540 [details] [diff] [review]
Something like this

a=dveditz for drivers

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 13 • 19 years ago

Checked in on branches.

	Bob Clary [:bc] (inactive) Reporter
	Comment 14 • 19 years ago

verified 2005-01-11 builds on windows/linux/mac on 1.8.0.1, 1.8, trunk

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 15 • 17 years ago

This caused bug 375344 because the OBJ_GET_CLASS was on the wrong object (oops!).

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 17 years ago

Need tests.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 17 • 17 years ago

This is covered in the JS test found in the URL field for this bug (ecma_3/Function/scope-001.js).

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 17 years ago

Do those run as part of the automated Tinderbox tests?

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 19 • 17 years ago

Sadly, no, they don't. IIRC, the js testsuite takes at least 30 minutes to run, even if one doesn't include the "long running" tests. bc has more info, I'm sure!

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 17 years ago

I don't think needing 30 mins to run is a problem, per se...  Especially not if it's got a dedicated machine.

	Bob Clary [:bc] (inactive) Reporter
	Comment 21 • 17 years ago

two-midairs! third time is the charm.

(In reply to comment #18)
> Do those run as part of the automated Tinderbox tests?
> 

No. It appears the required work to get buildbot to manage running the js tests and reporting to a tinderbox has a lower priority than other testing efforts and may not happen any time soon. Perhaps not before Firefox 3 ships if I understood correctly. Until then, I run them across a variety of machines and try to keep up with mrbkap, brendan, igor, crowder. 

There was a period that I was not running the tests regularly while I worked on a method for dealing with known failures and reporting regressions, but I am back to running the tests on daily basis again.

The shell tests, which would not have found this bug, actually run fairly quickly (depending on how you define "quickly"), but 30 minutes is definitely an upper bound. The 1.8.1 branch runs slower since it has more failures. The browser tests (which found this bug) take a couple of hours on a real machine to complete a run since they perform the following cycle for each test: start the browser, run the test, then shutdown. For the tests running on a VM, it can take considerably longer. To do a full test run for a branch, which includes opt, debug for shell, browser it can take on the order of a day on some of the VMs. In order to manage the results, I try to run the tests across all machines after brendan goes to sleep and before igor wakes up ;) and compare all of the results at once after the run completes.

When these are managed by buildbot, I think we can limit the build types to debug builds, run the js shell tests frequently during the day, and run the browser tests once a day. I can also trim the timeout I am currently using and exclude more of the slow running tests to improve turn around time.