498993 - Duplicate 'var' statement alters scope

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bert Belder]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1) Gecko/20090615 Firefox/3.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1) Gecko/20090615 Firefox/3.5

When a variable is declared twice (using the 'var' statement) in a javascript function, a new scope for that variable is created. Other browsers treat the second declaration as 'just' an assignment. Looking at the test case (attachment) will make it all clear.

Reproducible: Always

Steps to Reproduce:
Try the test case (see attachment) in Firefox 3.5pre

Actual Results:  
the browser shows 'incorrect'

Expected Results:  
like other browsers I tested with (FF 3.0, IE 6,7,8, recent Opera, Chrome, Safari)
'correct' should be shown.

Although unnecessary many JS developers make duplicate variable declarations by mistake. Maybe this should be fixed before the final FF3.5 release.

Comment 1

[Bert Belder]

Attachment: test case

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Yep, confirmed.  Seems pretty serious, hopefully a small issue in the dominance analysis.  Copying the big guns here.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

I considered a much smaller and more conservative fix to simply mark the defn as assigned if the var was re-initialized. However, that would unfairly penalize function() { var x = 4; var x; } so I came up with this.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 383824 [details] [diff] [review]
Proposed fix

Minimize change, don't add boundp as a parameter, add bound (or fresh, better name?) as a member of BindData to be set as an "out" param.

/be

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 383824 [details] [diff] [review]
Proposed fix

This patch looks good, though as Brendan mentions a smaller patch would be even better.

Are PND_INITIALIZED and PND_ASSIGNED misnomers?  Isn't initialization in all cases the same as assignment?  At least, for correctness, we have to count assignment as initialization in some places, and vice versa in others.

Comment 6

[Brendan Eich [:brendan]]

INITIALIZED and ASSIGNED are not misnomers, and the former does *not* imply the latter:

jsemit.cpp:            if (pn->isConst() && !pn->isInitialized())
jsparse.cpp:                        if (!lexdep->isInitialized())
jsparse.cpp:            JS_ASSERT(dn->isInitialized());
[1]-  Done                    ( cd Darwin_DBG.OBJ/; make >&made )
[2]+  Done                    ( cd Darwin_OPT.OBJ/; make >&made )
Yoyodyne:src brendaneich$ grep isAssigned *.cpp
jsparse.cpp:                        if (lexdep->isAssigned())
jsparse.cpp:                            if (lexdep->isAssigned())
jsparse.cpp:                dn->isAssigned()) {
jsparse.cpp:                    if (pnu->isAssigned() && pnu->pn_blockid >= funtc->bodyid) {
jsparse.cpp:        if (!dn->isAssigned()) {

The dominance analysis requires INITIALIZED. The NoteLValue code imputes it for an assignment to an uninitialized var in the same block, in later source order than the declaration.

/be

Comment 7

[Brendan Eich [:brendan]]

Smaller patch is better, I trust mrbkap can do it.

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to comments

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 383984 [details] [diff] [review]
Updated to comments

How about giving BindData a ctor that inits fresh to true, then you need only the one false?

/be

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/c4e87b867e61

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 383984 [details] [diff] [review]
Updated to comments

Nice, minimal patch landed, r=me on that.

/be

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/c4e87b867e61

Comment 13

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f2ebf72059a5

forgot to -u "Blake Kaplan", sorry :(

Comment 14

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

What does this codepath affect? Do we need to re-run all FFTs or is it more localized?

Comment 15

[Brendan Eich [:brendan]]

Fast Fourier Transforms?

"Good luck with your Fourierism!" (_Metropolitan_, Whit Stillman, 1990)

The JS testsuite should cover this, but it lacked the attached testcase (a shell verion of that testcase, of course). D'oh!

The effect of failing to notice the second var is the bug (non-constant value propagated into flat closure). The testcase checks the obvious way this can go wrong; a variant testing var in with would be good.

The effect of a hypothetical followup bug where we deoptimize too much would simply miss the oppty to make a flat closure, falling back on the same old Call object machinery this bug's patch relies on.

I will attach shell tests in a minute.

/be

Comment 16

[Brendan Eich [:brendan]]

Attachment: js shell version of html testcase

Comment 17

[Brendan Eich [:brendan]]

Attachment: variant of js shell test using with, but it passed before the patch too

Since with statements can mutate a variable and we do not analyze them further to see which names might be assigned in their bodies, we already deoptimize from a flat closure for this testcase. But the data->fresh = false; statement is good belt and braces, and formally correct. So the patch would cover this testcase if for some reason we had a bug elsewhere (we do not) in deoptimizing due to with.

/be

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on trunk and 1.9.1 with given testcase and builds on OS X and Windows like

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090622 Minefield/3.6a1pre ID:20090622031209

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090622 Shiretoko/3.5pre ID:20090622031110

Mike, will this be included into a possible RC3?

Comment 19

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

Yes, it would be, as it landed on 191. If we respin for RC3, this will be included.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Not worth checking for a regression range for an already fixed bug.