+++ This bug was initially created as a clone of Bug #322889 +++ The bug 322889 regressed the following tests in a debug thread-safe build of jsshell: js1_5/extensions/regress-311792-02.js js1_5/extensions/regress-355497.js js1_5/extensions/regress-311792-01.js js1_5/extensions/regress-365692.js js1_5/extensions/regress-315509-02.js js1_6/extensions/regress-414098.js A typical regression looks like: -*- executing: ./../src/Linux_All_DBG.OBJ/js -f ./shell.js -f ./js1_5/shell.js -f ./js1_5/extensions/shell.js -f ./js1_5/extensions/regress-311792-02.js -f ./js-\ test-driver-end.js *-* Testcase js1_5/extensions/regress-311792-02.js failed: Expected exit code 0, got 0 Testcase terminated with signal 5 Complete testcase output was: Assertion failure: OBJ_IS_NATIVE(obj), at jslock.c:1222

The regression was filed as bug 322889.

The bug was closed accidentally.

js_CheckAccess was calling js_LookupProperty rather than OBJ_LOOKUP_PROPERTY, which caused it to explode when obj was a non-native dense array. It had some non-native tests later, but they could lead to divergence if the non-native shared js_CheckAccess, which is not uncommon! Finally, the case in which we didn't find prop would exit early if clasp->checkAccess was NULL, instead of falling through to call the global hook. (Not related to this failure, but certainly seems to be a bug, and I was in there anyway.) All of these are remedied now, and the failures are gone.

Comment on attachment 304122 [details] [diff] [review] fix non-native use of js_CheckAccess, plus missing call to global checkAccess > if (!prop) { > if (!writing) > *vp = JSVAL_VOID; > *attrsp = 0; > clasp = OBJ_GET_CLASS(cx, obj); >- return !clasp->checkAccess || >- clasp->checkAccess(cx, obj, ID_TO_VALUE(id), mode, vp); >+ if (!clasp->checkAccess) >+ break; I think you have to set |pobj = obj| before the |break| here to avoid crashing when we get pboj's class below. Also might comment on what's going on, it's really non-trivial.

A wise man suggested this simplification in IRC, so he gets to review my implementation of it. No new failures with this patch, and the 6 reported above are fixed.

Comment on attachment 304124 [details] [diff] [review] simplify the !prop case, per mrbkap > if (!OBJ_IS_NATIVE(pobj)) { > OBJ_DROP_PROPERTY(cx, pobj, prop); >+ /* Avoid diverging for non-natives that reuse js_CheckAccess. */ >+ if (obj->map->ops->checkAccess == js_CheckAccess) >+ break; > return OBJ_CHECK_ACCESS(cx, pobj, id, mode, vp, attrsp); > } a question: why it is obj, not pobj, that is used in obj->map?

Because I'm a dork, basically. Thanks, new patch coming.

Comment on attachment 304129 [details] [diff] [review] fixed obj/pobj, thanks igor > default: >- if (!js_LookupProperty(cx, obj, id, &pobj, &prop)) >+ if (!OBJ_LOOKUP_PROPERTY(cx, obj, id, &pobj, &prop)) > return JS_FALSE; > if (!prop) { > if (!writing) > *vp = JSVAL_VOID; > *attrsp = 0; >- clasp = OBJ_GET_CLASS(cx, obj); >- return !clasp->checkAccess || >- clasp->checkAccess(cx, obj, ID_TO_VALUE(id), mode, vp); >+ pobj = obj; >+ break; > } Blank line here would be nice (see below). > if (!OBJ_IS_NATIVE(pobj)) { > OBJ_DROP_PROPERTY(cx, pobj, prop); Prevailing style puts a blank line before comments that start or consume an entire line, so here. >+ /* Avoid diverging for non-natives that reuse js_CheckAccess. */ >+ if (pobj->map->ops->checkAccess == js_CheckAccess) >+ break; > return OBJ_CHECK_ACCESS(cx, pobj, id, mode, vp, attrsp); > } > > sprop = (JSScopeProperty *)prop; > *attrsp = sprop->attrs; > if (!writing) { > *vp = (SPROP_HAS_VALID_SLOT(sprop, OBJ_SCOPE(pobj))) > ? LOCKED_OBJ_GET_SLOT(pobj, sprop->slot) So reusing native JSObjectOps hooks from a non-native JSObjectOps is a bit scary. Everything else like this good? I didn't check, I'm sorry to say, when I reviewed. Mix-n-match vtbl entries, whee. /be

I'll audit the rest shortly; when I first saw this I was tempted to clone into array_CheckAccess, but didn't want the maintenance hazard for security-relevant code. I'll fix nits, attach for posterity and commit when the tree clears. I thought that this reuse was the original intent of the "nearly-native" array model, though; it certainly seems to be the common pattern for custom object ops, so it deserves inspection at the least. Thanks!

Checking in jsobj.c; /cvsroot/mozilla/js/src/jsobj.c,v <-- jsobj.c new revision: 3.438; previous revision: 3.437 done

Note the browser tests for builds started at midnight PST are still failing on a number of these tests. <http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTest/1203426000.1203434553.10975.gz&fulltext=1>.

bc: You sure these are -new- failures? They look like they've been failing for a while. Also, some of the tests are e4x or other tests that have been failing for me in DBG shell for several days, I think.

Woops, I lied. A couple do look new. Working on it.

new regressions since today at midnight.

minus unrelated yield errors.

Need to make progress quickly -- suggest we get the patch for this bug in and fix remaining issues in a followup bug, unless it's actually faster or otherwise better to do more here. /be

This patch has already landed; it seems the new access checks are causing trouble. I'm researching now. I will open a follow-up bug when I understand better.

Marking FIXED since this has already landed.