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

VERIFIED FIXED in mozilla1.9.1

Status

()

Core
JavaScript Engine
P3
normal
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: bc, Assigned: Brian Crowder)

Tracking

({verified1.9.1})

Trunk
mozilla1.9.1
x86
All
verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 years ago
([]).__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
Flags: in-testsuite+
Flags: in-litmus-
(Reporter)

Comment 1

10 years ago
occurs on mozilla-central now.
Blocks: 449436
OS: Mac OS X → All

Comment 2

10 years ago
Still occuring?
Priority: -- → P1
Target Milestone: --- → mozilla1.9.1b1
(Assignee)

Comment 3

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

Updated

10 years ago
Assignee: general → crowder
(Assignee)

Comment 4

10 years ago
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?
(Assignee)

Comment 5

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

Comment 6

10 years ago
Agree.
Priority: P1 → P3
Target Milestone: mozilla1.9.1b1 → mozilla1.9.1
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
(Assignee)

Comment 8

10 years ago
We'll need JSOP_PROTO, JSOP_COUNT, etc., right?
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

10 years ago
Brian, any updates here?
(Assignee)

Comment 11

10 years ago
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?

Updated

10 years ago
Depends on: 469625
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
Attachment #353104 - Flags: review?(brendan) → review-
(Assignee)

Comment 14

10 years ago
Created attachment 359812 [details] [diff] [review]
deoptimize these cases

Like so...?
Attachment #353104 - Attachment is obsolete: true
Attachment #359812 - Flags: review?(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
Attachment #359812 - Flags: review?(brendan) → review+
(Assignee)

Comment 16

10 years ago
This patch rode along in bug 469625 (we should reopen it if that landing does not stick, I suppose)
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Reporter)

Comment 17

10 years ago
v 1.9.1-tracemonkey, 1.9.2
Status: RESOLVED → VERIFIED
(Reporter)

Comment 18

10 years ago
verified fixed 1.9.1 by bug 469625
Keywords: verified1.9.1
You need to log in before you can comment on or make changes to this bug.