463997 - TM: js1_5/Regress/regress-252892.js FAIL browser only

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Bob Clary [:bc] (inactive)]

regressed by http://hg.mozilla.org/tracemonkey/rev/d4fe79372140 bug 458851

STATUS: 9 FAILED!
STATUS: 12 FAILED!
STATUS: 13 FAILED!

Comment 1

[Bob Clary [:bc] (inactive)]

note this occurs on mozilla-central as well after the merges.

Comment 2

[Andreas Gal :gal]

For-in stuff. Brendan, help :)

Comment 3

[Brendan Eich [:brendan]]

This WFM on both tm and m-c. What am I missing? Platform differences? I am up to date and unpatched, testing on Mac.

/be

Comment 4

[Carsten Book [:Tomcat]]

note with my actual tracemonkey build i get on the testruns on mac 10.5:

TEST_ID=js1_5/Regress/regress-252892.js, TEST_BRANCH=1.9.1, TEST_REPO=tracemonkey, TEST_BUILDTYPE=debug, TEST_TYPE=browser, TEST_OS=darwin, TEST_KERNEL=9.5.0, TEST_PROCESSORTYPE=intel32, TEST_MEMORY=4, TEST_CPUSPEED=medium, TEST_TIMEZONE=+0100, TEST_RESULT=FAILED, TEST_EXITSTATUS=NORMAL, TEST_DESCRIPTION=for (var i in o) in heavyweight function f should define i in f's activation reason: Expected value 'true', Actual value 'false'; messages: ;, TEST_MACHINE=tomcat.local, TEST_DATE=2008-11-18-15-36-17+0100

Comment 5

[Bob Clary [:bc] (inactive)]

I can reproduce with fresh debug builds from mc and tm on mac 10.5. Looking at
my logs for today, it appears to not occur with every test run but does occur
on both linux and mac with non-zero frequency. Let me try a valgrind run to see
what it says.

Comment 6

[Brendan Eich [:brendan]]

Bob, Tomcat: you guys are testing in-browser, right? I'm testing shell only (debug and optimized).

/be

Comment 7

[Bob Clary [:bc] (inactive)]

browser only. see summary.

Comment 8

[Brendan Eich [:brendan]]

Duh! Sorry. Worried.

Graydon was chasing oracle-related browser-only badness.

/be

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Can we get a decision on whether or not this blocks b2 ASAP, SVP?

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Andreas says blocking, but not beta blocking. So be it!

Comment 11

[David Mandelin [:dmandelin]]

Attachment: Patch

The cause of the bug is subtle and requires the interaction of several features. The bug is triggered by 'with (window) { var x = blah }', where 'window' is the window object (global this in browser) and 'x = blah' is any statement that performs an assignment (including 'for x'). The bad behavior is that the assignment is made on the window object instead of to the activation object that 'var x' goes to.

The reason this happens is that during the lookup in the FORNAME/SETNAME/etc of property 'x', a new-style resolver hook is called on the window object with JSRESOLVE_ASSIGNING set. This ultimately delegates to nsWindowSH::NewResolve in nsDOMClassInfo.cpp. Near the end (search for expando, or see my patch), there is a block that optimizes property definitions on the window object by defining a property (I believe the point is that the property is defined at a point much closer to the start of lookup). This happens even inside a with statement, producing the bug behavior.

I think the fundamental issue is that the expando optimization assumes that it is at the end of the line for lookup, so that the property will eventually be defined on the global object. But of course this is not true inside a with block. 

To solve the problem I created an additional resolve flag indicating that we are assigning in a with block. The expando optimization checks for this flag and does not run if it is set. This appears to fix the problem, with minimal perf impact on any code paths that occur in the absence of 'with'. The only cost is the extra flag check on the expando optimization.

It seems that it might also be possible to make the expando optimization smarter, by having it check if it really is at the end of the line of lookup. It would also make it possible to run the optimization inside a with block in cases when it should be run. But it wasn't obvious how to do that, because it really needs to be looking further up the scope chain, and I don't know of a way for this point in the code to know its position in the scope chain (or even the fact that it was invoked by an object on the scope chain).

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 353776 [details] [diff] [review]
Patch

The flag is a good idea, but it should be called JSRESOLVE_WITH or JSRESOLVE_IN_WITH (or VIA_WITH, or WITHING? Hrm... suggestions welcome) -- it's orthogonal to JSRESOLVE_ASSIGNING.

/be

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

I like JSRESOLVE_WITH. I think that the patch needs to factor out the other JSRESOLVE_* code into a function and stick it in cx->resolveFlags, since it looks like with this patch |with (window) x = 4| when we resolve |x| on |window|, flags will *only* include JSRESOLVE_WITH.

Comment 14

[David Mandelin [:dmandelin]]

Attachment: Revised patch

This attempts to take care of the issues noted in the comments.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 354855 [details] [diff] [review]
Revised patch

Nits only:

>diff -r 1e89ebda81e0 dom/src/base/nsDOMClassInfo.cpp
>   if ((flags & (JSRESOLVE_ASSIGNING)) &&
>+      !(flags & (JSRESOLVE_WITH)) &&     /* Fixes bug 463997 */

IMO the comment here isn't worth it (it's what 'hg blame' is for) and I'm not sure what's up with the extra parens around the JSRESOLVE flags are for. Nuke them, I'd say!

>diff -r 1e89ebda81e0 js/src/jsobj.cpp
>+uintN InferFlags(JSContext *cx, uintN defaultFlags);

Define the helper function here instead of the extra prototype. Also, it should be static and the signature should look (as with other function signatures):

static uintN
InferFlags(JSContext *cx, uintN defaultFlags)
{
...
}

Comment 16

[David Mandelin [:dmandelin]]

Pushed to TM with nits fixed as 95d7117ae3ce. I also had to move Detecting to stay before InferFlags. FTR, I have no idea what was up with those extra parens either. Definitely not an intentional style choice. :-)

Comment 17

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/95d7117ae3ce

Comment 18

[Bob Clary [:bc] (inactive)]

verified fixed tm,mc. still need the fix on mozilla-1.9.1.

i'm worried we will lose this among the branches. how do we track what still needs to land on mozilla-1.9.1 ?

Comment 19

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2f1228a2d077

Comment 20

[Bob Clary [:bc] (inactive)]

v 1.9.1