450274 - TM: js1_5/extensions/regress-164697.js FAIL

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Bob Clary [:bc] (inactive)]

([]).__proto__ == Array.prototype reason: Expected value 'true', Actual value 'false'

([]).__proto__ == Array.prototype reason: Expected value 'true', Actual value 'false'

(new Array()).__proto__ == Array.prototype reason: Expected value 'true', Actual value 'false'

tracemonkey: 20f1f3c72ae7 tip

Comment 1

[Bob Clary [:bc] (inactive)]

occurs on mozilla-central now.

Comment 2

[Robert Sayre]

Still occuring?

Comment 3

[Brian Crowder]

Yes, this is still occurring.  I'm looking at it.

Comment 4

[Brian Crowder]

Okay.  Here's the problem.  The interpreter optimizes property look-ups in jsinterp.cpp at the do_getprop_with_obj label, and for dense-arrays, we use the object's prototype for prop-lookups using the property cache.  This means that when we lookup "__proto__" on [], we're actually looking for __proto__ on Array.prototype, and thus fail.

Fixing this requires more checking inside a very hot path, for an already non-standard feature.

Should we WONTFIX this?

Comment 5

[Brian Crowder]

sayrer: at the very least, I think we can safely drop the priority on this bug.

Comment 6

[Robert Sayre]

Agree.

Comment 7

[Brendan Eich [:brendan]]

Suggest if we care that we add a JSOP_PROTO bytecode for this. We deprecate setting [].__proto__ so let's WONTFIX that part of the regression. Easy bug for someone who wants to add a new opcode.

/be

Comment 8

[Brian Crowder]

We'll need JSOP_PROTO, JSOP_COUNT, etc., right?

Comment 9

[Brendan Eich [:brendan]]

No. See the __count__ bug which I WONTFIXed. We should not be shy about getting away from failed experiments such as writable __proto__ (apart from the specific use-case of making a "dict": var o = {__proto__:null, ...}) and __count__.

So, just JSOP_PROTO.

/be

Comment 10

[Robert Sayre]

Brian, any updates here?

Comment 11

[Brian Crowder]

No, this dropped way off my radar, and still doesn't really seem worth fixing, relative to other things.  Has the priority been moved back up?

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: stab

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 353104 [details] [diff] [review]
stab

obj is non-native (a dense array) so can't be passed to js_GetPropertyHelper. I have JSOP_PROTO as part of the fix for bug 469625, will attach soon.

/be

Comment 14

[Brian Crowder]

Attachment: deoptimize these cases

Like so...?

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 359812 [details] [diff] [review]
deoptimize these cases

>+    if (pn->pn_atom == cx->runtime->atomState.protoAtom ||
>+        pn->pn_atom == cx->runtime->atomState.countAtom ||
>+        pn->pn_atom == cx->runtime->atomState.parentAtom) {
...
>+                } else if (pn2->pn_atom == cx->runtime->atomState.protoAtom ||
>+                           pn2->pn_atom == cx->runtime->atomState.countAtom ||
>+                           pn2->pn_atom == cx->runtime->atomState.parentAtom) {

Nir: prefer proto/parent/count order used in jsobj.cpp. r=me with that picked, thanks!

/be

Comment 16

[Brian Crowder]

This patch rode along in bug 469625 (we should reopen it if that landing does not stick, I suppose)

Comment 17

[Bob Clary [:bc] (inactive)]

v 1.9.1-tracemonkey, 1.9.2

Comment 18

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.1 by bug 469625