443569 - Assertion failure: OBJ_IS_NATIVE(obj) after messing with __proto__

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: testcase (hangs/crashes Firefox when loaded)

Loading the testcase makes Firefox die.  Strangely, the JavaScript in the testcase does not break the standalone js shell.

Debug:
Assertion failure: OBJ_IS_NATIVE(obj), at /Users/jruderman/central/mozilla/js/src/jslock.cpp:1205

Opt: Usually hangs, but can crash.

This bug triggers the same assertion that bug 436741 triggered.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

regexp_getProperty doesn't expect the regexp to be on the proto chain of the object that's passed in, so it does a bunch of stuff that requires its object to be a regexp object and one of those things is to lock the object. This patch makes us look up the proto chain for the regexp object.

I'm not sure if I actually need to null-check (are we guaranteed to find the object on the proto chain?) but it felt better to be defensive. I also looked through the rest of the tree and didn't see any other getProperty implementations that would crash if they didn't find their object.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated patch

Sorry for the spam, I noticed that regexp_setProperty needed similar treatment, but failed to qrefresh before I attached the previous patch.

Comment 3

[Jesse Ruderman]

I finally got the stack trace for the crash I hit in a nightly:
http://crash-stats.mozilla.com/report/index/69cf4c2b-499b-11dd-86bb-001cc4e2bf68

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 328111 [details] [diff] [review]
Updated patch

>Don't do things to the object before we're sure it's the right type of object. bug 443569
>
>diff --git a/js/src/jsregexp.cpp b/js/src/jsregexp.cpp
>--- a/js/src/jsregexp.cpp
>+++ b/js/src/jsregexp.cpp
>@@ -3622,12 +3622,17 @@ regexp_getProperty(JSContext *cx, JSObje
> 
>     if (!JSVAL_IS_INT(id))
>         return JS_TRUE;
>+    while (JS_GET_CLASS(cx, obj) != &js_RegExpClass) {

Use OBJ_GET_CLASS inside the engine, avoid an API call.

>+        obj = OBJ_GET_PROTO(cx, obj);
>+        if (!obj)
>+            return JS_TRUE;

This reminds me of code I've written and/or seen elsewhere, going back to window objects in MozillaClassic. Is it similar to code you know of in the modern world?

r=me with OBJ_GET_CLASS instead of JS_GET_CLASS.

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #4)
> This reminds me of code I've written and/or seen elsewhere, going back to
> window objects in MozillaClassic. Is it similar to code you know of in the
> modern world?

We use it in just about every [GS]etProperty hook outside of the engine (usually hidden in a FindWrapper type function). Most hooks in the engine avoid the need by calling JS_GetInstancePrivate on the incoming object and bailing if it does not return private data.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to comments

This is what I'm going to check in when I next get a chance.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Pushed as changeset 3916fa3c8467.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 328309 [details] [diff] [review]
Updated to comments

This applies to the 1.9 branch.

Comment 9

[Samuel Sidler (old account; do not CC)]

Let's get a test for this before landing in 1.9.0.x.

Comment 10

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-443569.js,v  <--  regress-443569.js
initial revision: 1.1

mozilla-central: changeset:   16481:d634d3d4388e

Comment 11

[Samuel Sidler (old account; do not CC)]

Comment on attachment 328309 [details] [diff] [review]
Updated to comments

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.9 branch.

Comment 13

[Samuel Sidler (old account; do not CC)]

/me hands mrbkap the fixed1.9.0.2 keyword. ;)

Comment 14

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0/trunk linux/mac/win