417981 - PROPERTY_CACHE_TEST called with non-native objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Mike Shaver (:shaver emeritus)]

JSOP_CALLPROP calls PROPERTY_CACHE_TEST without verifying that the object in question is native, and with the arrays patch in bug 322889 the object often is not.

JSOP_NAME, JSOP_BINDNAME, and JSOP_CALLNAME do the same; if we're sure that fp->scopeChain can only be a native object then we should assert it, otherwise they need to defend as well.

(JSOP_SET{NAME,PROP} defend by checking ops->setProperty vs js_SetProperty before the open-coded "JS_PROPERTY_CACHE_TEST".  JSOP_GETPROP and JSOP_GETPROPX check getProperty instead.)

The semantics of PROPERTY_CACHE_TEST don't seem to permit an easy internal check, so I guess we'll need to add such a test to each of the callers.

Comment 1

[Brendan Eich [:brendan]]

Yes, callers must test. My bad -- but hmm, wouldn't we want array.method() to win from property cache (esp. the polymorphic L1 fast hit -- "pchit" -- case)? Should Array.prototype be a slow array (i.e., native)?

/be

Comment 2

[Mike Shaver (:shaver emeritus)]

Happily, Array.prototype is always a slow array, because the non-index names of the methods force it to be.

Comment 3

[Brendan Eich [:brendan]]

Attachment: native guards for P_C_T, plus more efficient else clauses

diff -w version of same next.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: diff -w version of last attachment

Comment 5

[Mike Shaver (:shaver emeritus)]

Looks OK to me, testing it out now.  I don't see that this will cause us to get propcache love for array methods, but that sounds like prime follow-on fodder to me

Comment 6

[Mike Shaver (:shaver emeritus)]

Comment on attachment 303960 [details] [diff] [review]
native guards for P_C_T, plus more efficient else clauses

Tests out well too, r=shaver.

Comment 7

[Mike Shaver (:shaver emeritus)]

Filed bug 418239 on not hitting the propcache for native prototypes of non-native objects.

Comment 8

[Brendan Eich [:brendan]]

Fixed:

js/src/jsinterp.c 3.431
js/src/jsinterp.h 3.75

Thanks for the followup bug, I'll take that now.

/be