483179 - TM: JIT embeds stale closure in trace for JSOP_DEFLOCALFUN

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Stefan A. Möller]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b4pre) Gecko/20090313 SeaMonkey/2.0b1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b4pre) Gecko/20090313 SeaMonkey/2.0b1pre

If you assign the value of "this" to a JavaScript object's private variable, and then read that variable's value, it is - under certain circumstances - referring to *another* object of the same class.

Reproducible: Always

Steps to Reproduce:
See attached testcase. The testcase contains a loop over 20 individual instances of the same class.
Actual Results:  
The values of "this.id" and "thisObject.id" should be identical for all the individual objects.

Expected Results:  
In some of the objects those values are different.

This bug happens in SeaMonkey 2.0 only. Neither Seamonkey 1.x nor Firefox 3.0 show this behaviour.

Comment 1

[Stefan A. Möller]

Attachment: testcase

Comment 2

[Philip Chee]

Can you also check with Firefox 3.1b3 which corresponds to SeaMonkey 2.0a3

Comment 3

[Brendan Eich [:brendan]]

Attachment: js shell version of testcase

Beward: the "testcase" attachment puts up 20 alerts.

/be

Comment 4

[Brendan Eich [:brendan]]

This is a blocker for sure - thanks for filing it!

/be

Comment 5

[Jason Orendorff [:jorendorff]]

"thisObject" gets the wrong answer. "this" is correct.

I think the JIT is not emitting the right guards for this kind of property cache hit.

Comment 6

[Jason Orendorff [:jorendorff]]

Correction, the JIT correctly bails out of the second loop.  The problem is in the first loop, the one that creates the closures.  We can't create closures on trace yet, not until upvar2 lands.  We shouldn't be recording JSOP_DEFLOCALFUN.

Comment 7

[Brendan Eich [:brendan]]

I misspoke on IRC: the upvar2 patch does fix this bug.

/be

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #5)
> "thisObject" gets the wrong answer. "this" is correct.
> 
> I think the JIT is not emitting the right guards for this kind of property
> cache hit.

Those poor guards, when will they earn your respect? When, I ask! :-P

/be

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: v1 (although upvar2 obsoletes this)

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 367306 [details] [diff] [review]
v1 (although upvar2 obsoletes this)

Before b3, sure. With upvar2 close, would rather avoid the conflict and charge hard across the goal line.

/be

Comment 11

[Jason Orendorff [:jorendorff]]

Un-taking.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 367306 [details] [diff] [review]
v1 (although upvar2 obsoletes this)

I think we should take this, it seems like it'd be possible to fool upvar2 with a |with| statement or |eval|, leading back to this bug.

Comment 13

[Robert Sayre]

http://hg.mozilla.org/tracemonkey/rev/25220accae01

Comment 14

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/25220accae01

Comment 15

[Robert Sayre]

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

Comment 16

[Aakash Desai [:aakashd]]

verified FIXED on builds: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090422 Shiretoko/3.5b4pre ID:20090422042031

Comment 17

[Christian Holler (:decoder)]

Filter on qa-project-auto-change:

Bug in removed tracer code, setting in-testsuite- flag.