Last Comment Bug 566136 - Assertion failure: isNative() after global.__proto__ = []
: Assertion failure: isNative() after global.__proto__ = []
Status: RESOLVED FIXED
[sg:critical?] fixed-in-tracemonkey [...
: regression, testcase, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
:
Mentors:
Depends on:
Blocks: 487039
  Show dependency treegraph
 
Reported: 2010-05-15 08:42 PDT by Igor Bukanov
Modified: 2013-03-20 08:36 PDT (History)
13 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1+
.7+
.7-fixed
.11+
.11-fixed


Attachments
v1 (1.43 KB, patch)
2010-05-16 10:25 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
v2 (2.53 KB, patch)
2010-05-16 12:42 PDT, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
fix for 192 and 191 (653 bytes, patch)
2010-06-24 15:47 PDT, Igor Bukanov
brendan: review+
christian: approval1.9.2.7+
christian: approval1.9.1.11+
Details | Diff | Splinter Review

Description Igor Bukanov 2010-05-15 08:42:19 PDT
The following scripts gives an assert in debug builds:

~/m/tm/js/src> cat ~/s/x.js
this.__proto__ = [];
print(length);
~/m/tm/js/src> ~/build/js/tdbg/js ~/s/x.js
Assertion failure: isNative(), at /home/igor/m/tm/js/src/jsscope.h:549
Aborted
Comment 1 Igor Bukanov 2010-05-16 08:43:26 PDT
The bug exists since the moment of introduction of the property cache so I nominate it for branches down to 190.
Comment 2 Igor Bukanov 2010-05-16 10:25:47 PDT
Created attachment 445612 [details] [diff] [review]
v1

The patch adds the missing isNative check
Comment 3 Brendan Eich [:brendan] 2010-05-16 10:29:46 PDT
Comment on attachment 445612 [details] [diff] [review]
v1

Should we make fill test pobj->isNative() instead? I.e., are there other paths that could lead here?

/be
Comment 4 Igor Bukanov 2010-05-16 12:40:44 PDT
(In reply to comment #3)
> (From update of attachment 445612 [details] [diff] [review])
> Should we make fill test pobj->isNative() instead? I.e., are there other paths
> that could lead here?

That was really good point. js_FindPropertyHelper also suffers from this bug as the following test case shows:

this.__proto__ = [];
function f() {
    eval('Math');
    length = 2;
}

f();

On the other hand AFAICS all other users of LookupProperty calls consistently check for native objects before casting prop to JSScopeProperty. Thus I prefer not to penalize the fill method and rather add the spot checks to the two bad users. But we should definitely do some fuzzing.

Gary, could you run a fuzzer after doing an assignments like this.__proto__ = [] or  this.__proto__ = <xml/> ?
Comment 5 Igor Bukanov 2010-05-16 12:42:26 PDT
Created attachment 445617 [details] [diff] [review]
v2

v2 adds another spot check for non-native prototypes of the global objects.
Comment 6 Brendan Eich [:brendan] 2010-05-16 13:00:52 PDT
js::PropertyCache::fill needs to be cold path or the cache isn't working. I suppose we can rely on the assertion plus fuzzing. I will wait till tomorrow to review in case fuzzing reveals other paths (in which case, or even so: let's test in fill and not worry until profile data shows it slows down benchmarks).

/be
Comment 7 Igor Bukanov 2010-05-16 13:13:44 PDT
(In reply to comment #6)
> let's
> test in fill and not worry until profile data shows it slows down benchmarks).

Doing the check in the fill is not enough. Consider that js_FindIdentifierBase is calling JS_UNLOCK_OBJ on the obj assuming that pobj is native. So we really have to monitor all the lookup users. For this reason I still prefer to do the check in the callers, not in the fill.
Comment 8 Brendan Eich [:brendan] 2010-05-16 14:27:44 PDT
You're right that js_FindIdentifierBase needs to follow the rules -- when did it stop? Did it used to loop over only js_IsCacheableNonGlobalScope(obj) and more recently also the global object was included in the loop?

It's huge to be able to count on native-only proto chain, but the general case of obj->lookupProperty requires obj->dropProperty after (until we get rid of that as proposed elsewhere). Breaking the general rule proliferates if-not-native tests.

/be
Comment 9 Brendan Eich [:brendan] 2010-05-16 14:31:01 PDT
I'm really asking whether it makes sense to unroll any final iteration of the loops in js_FindPropertyHelper and js_FindIdentifierBase if the global object is hit, since the cacheable non-global scope chain objects do not require this is-native checking, and indeed seem to share less code with the global object last-iteration case anyway.

/be
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2010-05-16 17:12:03 PDT
(In reply to comment #4)
> all other users of LookupProperty calls consistently check for native objects
> before casting prop to JSScopeProperty

This is where the root problem lies; we'll continue to have problems as long as we keep doing this without including some sort of automatic checks.  Could we provide a way of converting that will assert nativeness?

struct JSProperty {
  JSScopeProperty* asScopeProperty(JSObject* obj) const {
    union {
      const JSProperty* prop;
      const JSScopeProperty* sprop;
    } u;
    u.prop = this;

    JS_ASSERT(obj->scope()->hasProperty(u.sprop));

    return u.sprop;
  }
};

Or something in the same spirit with more rigor and less strawman-ness?
Comment 11 Brendan Eich [:brendan] 2010-05-16 18:42:56 PDT
We're getting rid of the JSProperty * opaque pointer type from JSObjectOps hooks, instead. See bug 566143.

/be
Comment 12 Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-05-17 06:06:03 PDT
I'd be pretty cool with __proto__ setting on a JSCLASS_IS_GLOBAL_OBJECT thing throwing an error, too.  We're going to get rid of settable proto (vs initializable proto) in due course anyway, and it seems like we can take a first stab by blocking some of the more ludicrous uses. :-)
Comment 13 Igor Bukanov 2010-05-17 06:59:38 PDT
(In reply to comment #12)
> I'd be pretty cool with __proto__ setting on a JSCLASS_IS_GLOBAL_OBJECT thing
> throwing an error, too.

This would not help this bug as a rogue script can set the proto of global.__proto__. The latter is Object.prototype which is a plain object.

>  We're going to get rid of settable proto 

That helps but only if there is guarantee that it would be impossible to construct a global object with non-native on the prototype chain.
Comment 14 Igor Bukanov 2010-05-17 07:07:28 PDT
(In reply to comment #8)
> You're right that js_FindIdentifierBase needs to follow the rules -- when did
> it stop? Did it used to loop over only js_IsCacheableNonGlobalScope(obj) and
> more recently also the global object was included in the loop?

This bug is a regression from my patch for the bug 487039. I was too zealous there optimizing the lookup.
Comment 15 Igor Bukanov 2010-05-17 07:08:44 PDT
(In reply to comment #9)
> I'm really asking whether it makes sense to unroll any final iteration of the
> loops in js_FindPropertyHelper and js_FindIdentifierBase

I have tried, but that makes the patch much bigger and I prefer a spot fixes here to simplify back-porting.
Comment 18 Igor Bukanov 2010-06-24 15:47:36 PDT
Created attachment 453882 [details] [diff] [review]
fix for 192 and 191

Compared with the trunk on 192-191 there is no need to fix js_FindIdentifierBase. The latter on those branches does not cache the lookup for globals as code there relies just on gvar optimization. In js_FindPropertyHelper I also have not added an assert that the global should be native. I do not remember if a non-native global was completely disabled there.

Brendan, could you review this before before the freeze on Friday?
Comment 19 christian 2010-06-28 09:45:35 PDT
Comment on attachment 453882 [details] [diff] [review]
fix for 192 and 191

a=LegNeato for 1.9.2.7 and 1.9.1.11. Please land this on mozilla-1.9.2 default and mozilla-1.9.1 default asap.

Thanks!
Comment 21 Al Billings [:abillings] 2010-07-16 12:26:33 PDT
Bob, can you verify this on 1.9.1 and 1.9.2?
Comment 22 Bob Clary [:bc:] 2010-07-16 13:46:08 PDT
tests in comment 0 and comment 4 do not assert in the shell for 1.9.1, 1.9.2 on mac os x 10.5, linux 32/64 bit or winxp.

v 1.9.1, 1.9.2
Comment 23 Ed Morley [:emorley] 2013-03-20 04:48:51 PDT
https://hg.mozilla.org/mozilla-central/rev/1ea0203ab5fa

Note You need to log in before you can comment on or make changes to this bug.