Last Comment Bug 454704 - Crash [@ JS_HashTableRawLookup] with XPCSafeJSObjectWrapper, getter, toSource
: Crash [@ JS_HashTableRawLookup] with XPCSafeJSObjectWrapper, getter, toSource
Status: VERIFIED FIXED
[sg:critical] fixed-in-tracemonkey
: crash, testcase, verified1.9.0.12, verified1.9.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: P2 critical (vote)
: ---
Assigned To: Jim Blandy :jimb
:
Mentors:
Depends on:
Blocks: 326633
  Show dependency treegraph
 
Reported: 2008-09-10 16:50 PDT by Jesse Ruderman
Modified: 2011-06-13 10:01 PDT (History)
11 users (show)
sayrer: blocking1.9.1+
dveditz: blocking1.9.0.12+
stransky: wanted1.8.1.x-
bob: in‑testsuite+
bob: in‑litmus-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (crashes Firefox when loaded) (239 bytes, text/html)
2008-09-10 16:50 PDT, Jesse Ruderman
no flags Details
Simpler crasher (262 bytes, text/html)
2009-01-14 13:52 PST, Jim Blandy :jimb
no flags Details
Bug 454704: Protect sharpObjectMap in the presence of wrapped getters. (1.29 KB, patch)
2009-01-14 14:01 PST, Jim Blandy :jimb
no flags Details | Diff | Splinter Review
Bug 454704: Protect sharpObjectMap in the presence of wrapped getters. (1.93 KB, patch)
2009-01-15 15:12 PST, Jim Blandy :jimb
igor: review+
Details | Diff | Splinter Review
js1_5/extensions/regress-454704.js (2.86 KB, text/plain)
2009-01-21 14:55 PST, Bob Clary [:bc:]
no flags Details
Bug 464704: Adapt fix for 1.9.0 tree. (1.96 KB, patch)
2009-06-22 15:16 PDT, Jim Blandy :jimb
igor: review+
samuel.sidler+old: approval1.9.0.12+
Details | Diff | Splinter Review

Description Jesse Ruderman 2008-09-10 16:50:18 PDT
Created attachment 337991 [details]
testcase (crashes Firefox when loaded)

Thread 0 Crashed:
0   JS_HashTableRawLookup + 10 (jshash.cpp:177)
1   MarkSharpObjects(JSContext*, JSObject*, JSIdArray**) + 120 (jsobj.cpp:360)
2   MarkSharpObjects(JSContext*, JSObject*, JSIdArray**) + 750 (jsobj.cpp:418)
3   MarkSharpObjects(JSContext*, JSObject*, JSIdArray**) + 750 (jsobj.cpp:418)
4   js_EnterSharpObject + 277 (jsobj.cpp:475)
5   js_array_join_sub + 141 (jsarray.cpp:1214)
6   array_toString(JSContext*, unsigned int, long*) + 165 (jsarray.cpp:1429)
7   js_Invoke + 1313 (jsinterp.cpp:1189)
...
Comment 1 Jim Blandy :jimb 2008-12-04 15:43:21 PST
I've reproduced this with the testcase given in f90c51ed3cd2 on Ubuntu.
Comment 2 Jim Blandy :jimb 2008-12-05 18:09:57 PST
What's interesting is that we seem to have invoked the getter for f, instead of just registering it in the table:

(gdb) where 30
#0  MarkSharpObjects (cx=0xb2a69800, obj=0xb14295a0, idap=0xbfff7a24)
    at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:386
#1  0xb7e44432 in js_EnterSharpObject (cx=0xb2a69800, obj=0xb14295a0, 
    idap=0x0, sp=0xbfff7ae4)
    at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:483
#2  0xb7db0f03 in array_join_sub (cx=0xb2a69800, obj=0xb14295a0, 
    op=TO_SOURCE, sep=0x0, rval=0xb15aa0f4)
    at /home/jimb/mc/b/454704/src/js/src/jsarray.cpp:1251
#3  0xb7db1c0e in array_toSource (cx=0xb2a69800, argc=0, vp=0xb15aa0f4)
    at /home/jimb/mc/b/454704/src/js/src/jsarray.cpp:1452
#4  0xb7e2a52b in js_Invoke (cx=0xb2a69800, argc=0, vp=0xb15aa0f4, flags=0)
    at /home/jimb/mc/b/454704/src/js/src/jsinterp.cpp:1192
#5  0xb7e2b18b in js_InternalInvoke (cx=0xb2a69800, obj=0xb14295a0, 
    fval=-1321007752, flags=0, argc=0, argv=0x0, rval=0xbfff934c)
    at /home/jimb/mc/b/454704/src/js/src/jsinterp.cpp:1388
#6  0xb7e2b3ea in js_InternalGetOrSet (cx=0xb2a69800, obj=0xb14295a0, 
    id=-1238794948, fval=-1321007752, mode=JSACC_READ, argc=0, argv=0x0, 
    rval=0xbfff934c) at /home/jimb/mc/b/454704/src/js/src/jsinterp.cpp:1449
#7  0xb7e3b3a2 in js_NativeGet (cx=0xb2a69800, obj=0xb14295a0, 
    pobj=0xb14295a0, sprop=0xb1484130, vp=0xbfff934c)
    at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:3669
#8  0xb7e3e174 in js_GetPropertyHelper (cx=0xb2a69800, obj=0xb14295a0, 
    id=-1238794948, vp=0xbfff934c, entryp=0x0)
    at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:3818
#9  0xb7e3e226 in js_GetProperty (cx=0xb2a69800, obj=0xb14295a0, 
    id=-1238794948, vp=0xbfff934c)
    at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:3830
#10 0xb7dff62c in js_Interpret (cx=0xb2a69800)
    at /home/jimb/mc/b/454704/src/js/src/jsinterp.cpp:4688
#11 0xb7e2a984 in js_Invoke (cx=0xb2a69800, argc=1, vp=0xb15aa0d4, flags=0)
    at /home/jimb/mc/b/454704/src/js/src/jsinterp.cpp:1331
#12 0xb7e2b18b in js_InternalInvoke (cx=0xb2a69800, obj=0xb14295a0, 
    fval=-1321005232, flags=0, argc=1, argv=0xbfff9780, rval=0xbfff978c)
    at /home/jimb/mc/b/454704/src/js/src/jsinterp.cpp:1388
#13 0xb7d936cf in JS_CallFunctionValue (cx=0xb2a69800, obj=0xb14295a0, 
    fval=-1321005232, argc=1, argv=0xbfff9780, rval=0xbfff978c)
    at /home/jimb/mc/b/454704/src/js/src/jsapi.cpp:5242
#14 0xb613d4a8 in XPC_SJOW_GetOrSetProperty (cx=0xb2a69800, obj=0xb1429600, 
    id=-1238794948, vp=0xbfff98f8, aIsSet=0)
    at /home/jimb/mc/b/454704/src/js/src/xpconnect/src/XPCSafeJSObjectWrapper.cpp:585
#15 0xb613d558 in XPC_SJOW_GetProperty (cx=0xb2a69800, obj=0xb1429600, 
    id=-1238794948, vp=0xbfff98f8)
    at /home/jimb/mc/b/454704/src/js/src/xpconnect/src/XPCSafeJSObjectWrapper.cpp:593
#16 0xb7e3b432 in js_NativeGet (cx=0xb2a69800, obj=0xb1429600, 
    pobj=0xb1429600, sprop=0xb1484290, vp=0xbfff98f8)
    at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:3669
#17 0xb7e3e174 in js_GetPropertyHelper (cx=0xb2a69800, obj=0xb1429600, 
    id=-1238794948, vp=0xbfff98f8, entryp=0x0)
    at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:3818
#18 0xb7e3e226 in js_GetProperty (cx=0xb2a69800, obj=0xb1429600, 
    id=-1238794948, vp=0xbfff98f8)
    at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:3830
#19 0xb7e441d7 in MarkSharpObjects (cx=0xb2a69800, obj=0xb1429600, idap=0x0)
    at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:417
#20 0xb7e4423e in MarkSharpObjects (cx=0xb2a69800, obj=0xb1429580, idap=0x0)
    at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:426
#21 0xb7e4423e in MarkSharpObjects (cx=0xb2a69800, obj=0xb1429560, 
    idap=0xbfff9a74) at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:426
#22 0xb7e44432 in js_EnterSharpObject (cx=0xb2a69800, obj=0xb1429560, 
    idap=0x0, sp=0xbfff9b34)
    at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:483
#23 0xb7db0f03 in array_join_sub (cx=0xb2a69800, obj=0xb1429560, 
    op=TO_STRING, sep=0x0, rval=0xb15aa0cc)
    at /home/jimb/mc/b/454704/src/js/src/jsarray.cpp:1251
#24 0xb7db1b58 in array_toString (cx=0xb2a69800, argc=0, vp=0xb15aa0cc)
    at /home/jimb/mc/b/454704/src/js/src/jsarray.cpp:1466
#25 0xb7e2a52b in js_Invoke (cx=0xb2a69800, argc=0, vp=0xb15aa0cc, flags=0)
    at /home/jimb/mc/b/454704/src/js/src/jsinterp.cpp:1192
#26 0xb7e2b18b in js_InternalInvoke (cx=0xb2a69800, obj=0xb1429560, 
    fval=-1321007696, flags=0, argc=0, argv=0x0, rval=0xbfff9d54)
    at /home/jimb/mc/b/454704/src/js/src/jsinterp.cpp:1388
#27 0xb7e3934e in js_TryMethod (cx=0xb2a69800, obj=0xb1429560, 
    atom=0xb6af1264, argc=0, argv=0x0, rval=0xbfff9d54)
    at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:4963
#28 0xb7e3a441 in js_DefaultValue (cx=0xb2a69800, obj=0xb1429560, 
    hint=JSTYPE_VOID, vp=0xb15aa0b4)
    at /home/jimb/mc/b/454704/src/js/src/jsobj.cpp:4197
#29 0xb7df5f0c in js_Interpret (cx=0xb2a69800)
    at /home/jimb/mc/b/454704/src/js/src/jsinterp.cpp:3778
(More stack frames follow...)
(gdb)
Comment 3 Robert Sayre 2008-12-17 10:40:08 PST
Why is XPCSafeJSObjectWrapper is accessible to content?
Comment 4 Blake Kaplan (:mrbkap) 2008-12-17 14:20:05 PST
Historical reasons.
Comment 5 Blake Kaplan (:mrbkap) 2008-12-17 14:20:42 PST
(Namely, when it was originally implemented, it was available. I don't think there's a specific reason, though.)
Comment 6 Jim Blandy :jimb 2009-01-14 13:52:48 PST
Created attachment 357021 [details]
Simpler crasher

Here's a simplified page that also crashes Firefox, without getting any windows involved.
Comment 7 Jim Blandy :jimb 2009-01-14 14:01:09 PST
Created attachment 357022 [details] [diff] [review]
Bug 454704: Protect sharpObjectMap in the presence of wrapped getters.

This seems to do the trick.

Alternatively, should OBJ_GET_ATTRIBUTES carry getter and setter flags through wrappers, so we can avoid invoking them altogether?  (Are there ops that a wrapper can use to fetch getters and setters, instead of invoking them?)
Comment 8 Jim Blandy :jimb 2009-01-14 14:10:23 PST
With that patch applied, Firefox no longer crashes, but we do get bug 473630.  So there's more to be sorted out here.
Comment 9 Brendan Eich [:brendan] 2009-01-14 14:21:48 PST
Comment on attachment 357022 [details] [diff] [review]
Bug 454704: Protect sharpObjectMap in the presence of wrapped getters.

>Bug 454704: Protect sharpObjectMap in the presence of wrapped getters.
>
>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp
>--- a/js/src/jsobj.cpp
>+++ b/js/src/jsobj.cpp
>@@ -381,12 +381,20 @@ MarkSharpObjects(JSContext *cx, JSObject
>          * itself badly.  Without this fix, if we reenter the basis case where
>          * map->depth == 0, when unwinding the inner call we will destroy the
>          * newly-created hash table and crash.
>+         *
>+         * Note that wrapped objects may have getters, which we'll
>+         * invoke instead of simply marking, since we won't recognize
>+         * the object as a native object.  Those getters could start a
>+         * sharp-marking traversal themselves.  So, leave depth
>+         * incremented across all property gets, as well.
>          */
>         ++map->depth;
>         ida = JS_Enumerate(cx, obj);
>-        --map->depth;
>         if (!ida)
>-            return NULL;
>+            {
>+                --map->depth;
>+                return NULL;
>+            }

Please turn off the Stallman-free-software-song elisp (or whatever) that is foisting this over-indented and alien style on braced consequents :-/.

Igor, could you review? IIRC you have relatively recent experience (GC-oriented) here. Or maybe mrbkap could r+ if you are swamped. Thanks,

/be
Comment 10 Brendan Eich [:brendan] 2009-01-14 14:28:28 PST
Actually, mrbkap has the blame, but it was a long time ago:

revision 3.207
date: 2005/08/12 00:46:56;  author: mrbkap%gmail.com;  state: Exp;  lines: +8 -0
bug 304376: Fix hash table refcounting problem while recursively entering a marked object. r+a=brendan

and I'm r+a bottle-inspector (http://www.thesimpsons.com/episode_guide/0416.htm, "good, good, rat, good, ..." [Hitler's head in a jar goes by as inspector gabs with Homer]). Sorry for doing a Duff-ish job there.

/be
Comment 11 Igor Bukanov 2009-01-14 15:15:22 PST
(In reply to comment #9)
> (From update of attachment 357022 [details] [diff] [review])
> >Bug 454704: Protect sharpObjectMap in the presence of wrapped getters.

It is better to move map->depth++/-- and related comments from MarkSharpObjects and put them around the line where js_EnterSharpObject calls MarkSharpObjects. This not only make clear what the comments are talking about but also avoid duplicated  map->depth-- in MarkSharpObjects due to an early return. For an extra safety MarkSharpObjects should also assert that map->depth >= 1.
Comment 12 Jim Blandy :jimb 2009-01-14 15:23:20 PST
(In reply to comment #9)
> Please turn off the Stallman-free-software-song elisp (or whatever) that is
> foisting this over-indented and alien style on braced consequents :-/.

Fixed.  Emacs doesn't decide which line the braces go on; my fingers will get
reconfigured soon enough.
Comment 13 Jim Blandy :jimb 2009-01-14 15:27:11 PST
(In reply to comment #11)
> It is better to move map->depth++/-- and related comments from MarkSharpObjects
> and put them around the line where js_EnterSharpObject calls MarkSharpObjects.
> This not only make clear what the comments are talking about but also avoid
> duplicated  map->depth-- in MarkSharpObjects due to an early return. For an
> extra safety MarkSharpObjects should also assert that map->depth >= 1.

So, this is interesting: in the case of a nested traversal like the one described in the comment added by the patch, because depth would already be > 0, js_EnterSharpObject won't call MarkSharpObjects at all.  It assumes the first MarkSharpObjects' traversal visited everything there was to visit, which isn't true.
Comment 14 Brendan Eich [:brendan] 2009-01-14 15:44:44 PST
(In reply to comment #12)
> (In reply to comment #9)
> > Please turn off the Stallman-free-software-song elisp (or whatever) that is
> > foisting this over-indented and alien style on braced consequents :-/.
> 
> Fixed.  Emacs doesn't decide which line the braces go on; my fingers will get
> reconfigured soon enough.

Thanks -- jwz always blamed his elisp for (two-space c-basic-offset) RMSstyle in the Netscape XFE.

/be
Comment 15 Igor Bukanov 2009-01-14 15:52:09 PST
(In reply to comment #13)
> in the case of a nested traversal... because depth would already be >
> 0, js_EnterSharpObject won't call MarkSharpObjects at all.  It assumes the
> first MarkSharpObjects' traversal visited everything there was to visit, which
> isn't true.

This leads to printing of null for values value of some properties of non-native (is SM sense) objects. This should be fine for the fix as without the patch the code just crashes as a result of double-free.
Comment 16 Jim Blandy :jimb 2009-01-14 21:55:34 PST
(In reply to comment #15)
> This leads to printing of null for values value of some properties of
> non-native (is SM sense) objects. This should be fine for the fix as without
> the patch the code just crashes as a result of double-free.

It doesn't lead to a failure to detect cycles in the nested traversal?  (I'm not sure I can parse the first sentence.)
Comment 17 Igor Bukanov 2009-01-15 03:30:34 PST
(In reply to comment #16)
> It doesn't lead to a failure to detect cycles in the nested traversal?

You are right, it will lead to infinite recursion when !OBJ_IS_NATIVE() happens twice in the object graph, not printing NULL or something. But that is fine as a workaround. At some point the recursion detection will be triggered and an error is reported. This is very similar to what one get when using custom toSource implementation like in:

js> var point = { toSource: function() { return "(" + uneval(this.x) + ", " + uneval(this.y)+")"; } };
js> point.x = 10 
10
js> point.y = point;
[object Object]
js> uneval(point)
typein:8: InternalError: too much recursion


Such error message is infinitely better than the crash via double-free that happens with the current code.
Comment 18 Jim Blandy :jimb 2009-01-15 15:12:45 PST
Created attachment 357244 [details] [diff] [review]
Bug 454704: Protect sharpObjectMap in the presence of wrapped getters.

Revised per Igor's suggestions.
Comment 19 Igor Bukanov 2009-01-16 04:35:55 PST
I mark the bug as critical as it leads to double-free.
Comment 20 Igor Bukanov 2009-01-16 04:36:33 PST
To Jim: can you land this to TraceMonkey?
Comment 21 Jim Blandy :jimb 2009-01-16 09:41:20 PST
(In reply to comment #20)
> To Jim: can you land this to TraceMonkey?

Certainly.
Comment 22 Jim Blandy :jimb 2009-01-16 10:17:44 PST
Landed in TM: http://hg.mozilla.org/tracemonkey/rev/73f9f5e01fef
Comment 24 Bob Clary [:bc:] 2009-01-21 14:55:49 PST
Created attachment 358055 [details]
js1_5/extensions/regress-454704.js
Comment 26 Bob Clary [:bc:] 2009-02-08 09:18:21 PST
v 1.9.1, 1.9.2
Comment 27 Jim Blandy :jimb 2009-06-22 15:16:19 PDT
Created attachment 384503 [details] [diff] [review]
Bug 464704: Adapt fix for 1.9.0 tree.



Adapt fix for 1.9.0 tree.  Verified that Firefox crashes without patch, doesn't crash with.
Comment 28 Jim Blandy :jimb 2009-06-23 11:19:02 PDT
Thanks, Igor!  A favor: I haven't got my SSH machinery updated for committing to CVS; could you land that patch for me?
Comment 29 Igor Bukanov 2009-06-23 13:01:18 PDT
(In reply to comment #28)
> Thanks, Igor!  A favor: I haven't got my SSH machinery updated for committing
> to CVS; could you land that patch for me?

It is to late for me to do it today. But I will land tomorrow early morning PDT if this gets an approval.
Comment 30 Samuel Sidler (old account; do not CC) 2009-06-23 13:15:56 PDT
Comment on attachment 384503 [details] [diff] [review]
Bug 464704: Adapt fix for 1.9.0 tree.


Approved for 1.9.0.12. a=ss
Comment 31 Igor Bukanov 2009-06-23 13:25:19 PDT
landed to 1.9.0 :

Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.485; previous revision: 3.484
done
Comment 32 Al Billings [:abillings] 2009-06-30 17:41:34 PDT
Verified for 1.9.0.12 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.12pre) Gecko/2009063005 GranParadiso/3.0.12pre (.NET CLR 3.5.30729). Testcases both crash on 1.9.0.11 but not with 1.9.0.12 build.
Comment 33 Martin Stránský 2009-07-08 02:55:17 PDT
Does not affect 1.8.
Comment 34 Bob Clary [:bc:] 2009-08-18 01:42:42 PDT
http://hg.mozilla.org/tracemonkey/rev/e0017d17ac44

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

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