Last Comment Bug 411025 - GC hazard in JS_CompileUCFunctionForPrincipals
: GC hazard in JS_CompileUCFunctionForPrincipals
Status: RESOLVED FIXED
[sg:critical?] [needs branch patch] 1...
: fixed1.8.1.13
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 425576
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-06 08:16 PST by Igor Bukanov
Modified: 2008-04-17 12:19 PDT (History)
7 users (show)
dveditz: blocking1.9?
dveditz: blocking1.8.1.13+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next+
bob: in‑testsuite-
bob: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (14.57 KB, patch)
2008-01-06 08:27 PST, Igor Bukanov
brendan: review+
brendan: approval1.9+
Details | Diff | Splinter Review
v2 (16.23 KB, patch)
2008-01-07 05:56 PST, Igor Bukanov
brendan: review+
brendan: approval1.9+
Details | Diff | Splinter Review
v1-v2 delta (15.62 KB, patch)
2008-01-07 06:03 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
v1-v2 delta for real (6.62 KB, patch)
2008-01-07 06:06 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
v3 (16.17 KB, patch)
2008-01-19 16:44 PST, Igor Bukanov
igor: review+
Details | Diff | Splinter Review
v3 for the 1.8.1 branch (15.68 KB, patch)
2008-02-13 13:58 PST, Igor Bukanov
brendan: review+
dveditz: approval1.8.1.13+
Details | Diff | Splinter Review
plain diff of trunk and 181 patches (7.52 KB, patch)
2008-02-13 14:12 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
for 1.8.0 (16.44 KB, patch)
2008-03-23 09:27 PDT, Alexander Sack
igor: review+
Details | Diff | Splinter Review
1.8 to 1.8.0 plaindiff (4.29 KB, patch)
2008-03-23 09:28 PDT, Alexander Sack
no flags Details | Diff | Splinter Review
use the right out label (16.04 KB, patch)
2008-03-28 00:27 PDT, timeless
no flags Details | Diff | Splinter Review
for 1.8.0 (16.82 KB, patch)
2008-03-28 00:30 PDT, timeless
no flags Details | Diff | Splinter Review
v3.02 for the 1.8.1 branch (16.06 KB, patch)
2008-03-28 00:36 PDT, timeless
no flags Details | Diff | Splinter Review
for 1.8.0 (out2, null fun instead of violating out constraints) (16.85 KB, patch)
2008-03-28 00:39 PDT, timeless
asac: approval1.8.0.next-
Details | Diff | Splinter Review

Description Igor Bukanov 2008-01-06 08:16:27 PST
JS_CompileUCFunctionForPrincipals contains the following code:

    JSFunction *fun;
...
    fun = js_NewFunction(cx, NULL, NULL, 0, JSFUN_INTERPRETED, obj, funAtom);
...
    if (!js_CompileFunctionBody(cx, fun, principals, chars, length,
                                filename, lineno)) {
...
    if (obj &&
        funAtom &&
        !OBJ_DEFINE_PROPERTY(cx, obj, ATOM_TO_JSID(funAtom),
                             OBJECT_TO_JSVAL(fun->object),
                             NULL, NULL, JSPROP_ENUMERATE, NULL)) {
        fun = NULL;
    }

...
    LAST_FRAME_CHECKS(cx, fun);
    return fun;

Here js_NewFunction creates a function rooted only via the newborn array. If the supplied source contains other function objects, then when the compiler allocates them they displace the fun from the newborn array and unroot it. Then any last-ditch GC during the compilation later would collect the live pointer.

Before the bug 404935 and on 1.8.1 branch the problem was not that bad as a pseudo-frame that the compiler creates would root fun during the compilation. Still after js_CompileFunctionBody returns, OBJ_DEFINE_PROPERTY would be called with unrooted fun or, when obj is null, the unrooted value would be returned to the caller.
Comment 1 Igor Bukanov 2008-01-06 08:27:16 PST
Created attachment 295633 [details] [diff] [review]
v1

The fix adds JS_PUSH_TEMP_ROOT_FUNCTION to root the function. To avoid yet another copy of almost similar macro lines to define the new rooting helper I parametrized JS_PUSH_TEMP_ROOT_COMMON to accept the count and field name arguments so all JS_PUSH_TEMP_ROOT_ can be made one-liners. With the patch all TVR macro definitions fit on one screen:

#define JS_PUSH_TEMP_ROOT_COMMON(cx,x,tvr,cnt,fieldName)                      \
    JS_BEGIN_MACRO                                                            \
        JS_ASSERT((cx)->tempValueRooters != (tvr));                           \
        (tvr)->count = cnt;                                                   \
        (tvr)->u.fieldName = (x);                                             \
        (tvr)->down = (cx)->tempValueRooters;                                 \
        (cx)->tempValueRooters = (tvr);                                       \
    JS_END_MACRO

#define JS_POP_TEMP_ROOT(cx,tvr)                                              \
    JS_BEGIN_MACRO                                                            \
        JS_ASSERT((cx)->tempValueRooters == (tvr));                           \
        (cx)->tempValueRooters = (tvr)->down;                                 \
    JS_END_MACRO

#define JS_PUSH_TEMP_ROOT(cx,cnt,arr,tvr)                                     \
    JS_BEGIN_MACRO                                                            \
        JS_ASSERT((ptrdiff_t)(cnt) >= 0);                                     \
        JS_PUSH_TEMP_ROOT_COMMON(cx, arr, tvr, ((ptrdiff_t) (cnt)), array);   \
    JS_END_MACRO

#define JS_PUSH_SINGLE_TEMP_ROOT(cx,val,tvr)                                  \
    JS_PUSH_TEMP_ROOT_COMMON(cx, val, tvr, JSTVU_SINGLE, value)

#define JS_PUSH_TEMP_ROOT_OBJECT(cx,obj,tvr)                                  \
    JS_PUSH_TEMP_ROOT_COMMON(cx, obj, tvr, JSTVU_SINGLE, object)

#define JS_PUSH_TEMP_ROOT_STRING(cx,str,tvr)                                  \
    JS_PUSH_TEMP_ROOT_COMMON(cx, str, tvr, JSTVU_SINGLE, string)

#define JS_PUSH_TEMP_ROOT_FUNCTION(cx,fun,tvr)                                \
    JS_PUSH_TEMP_ROOT_COMMON(cx, fun, tvr, JSTVU_SINGLE, function)

#define JS_PUSH_TEMP_ROOT_QNAME(cx,qn,tvr)                                    \
    JS_PUSH_TEMP_ROOT_COMMON(cx, qn, tvr, JSTVU_SINGLE, qname)

#define JS_PUSH_TEMP_ROOT_XML(cx,xml_,tvr)                                    \
    JS_PUSH_TEMP_ROOT_COMMON(cx, xml_, tvr, JSTVU_SINGLE, xml)

#define JS_PUSH_TEMP_ROOT_TRACE(cx,trace_,tvr)                                \
    JS_PUSH_TEMP_ROOT_COMMON(cx, trace_, tvr, JSTVU_TRACE, trace)

#define JS_PUSH_TEMP_ROOT_SPROP(cx,sprop_,tvr)                                \
    JS_PUSH_TEMP_ROOT_COMMON(cx, sprop_, tvr, JSTVU_SPROP, sprop)

#define JS_PUSH_TEMP_ROOT_WEAK_COPY(cx,weakRoots_,tvr)                        \
    JS_PUSH_TEMP_ROOT_COMMON(cx, weakRoots_, tvr, JSTVU_WEAK_ROOTS, weakRoots)

#define JS_PUSH_TEMP_ROOT_PARSE_CONTEXT(cx,pc,tvr)                            \
    JS_PUSH_TEMP_ROOT_COMMON(cx, pc, tvr, JSTVU_PARSE_CONTEXT, parseContext)

#define JS_PUSH_TEMP_ROOT_SCRIPT(cx,script_,tvr)                              \
    JS_PUSH_TEMP_ROOT_COMMON(cx, script_, tvr, JSTVU_SCRIPT, script)
Comment 2 Brendan Eich [:brendan] 2008-01-07 01:02:09 PST
Comment on attachment 295633 [details] [diff] [review]
v1

Thanks, nits only:

>+ * jsval, use JS_PUSH_TEMP_ROOT_<KIND>. The macros reinterprets an arbitrary

"... macros reinterpret ..."

>+ * The macros also provide a simple way to get a single pointer to rooted GC

"single rooted pointer"

>+#define JS_PUSH_TEMP_ROOT_COMMON(cx,x,tvr,cnt,fieldName)                      \

"mem" or "kind" (per <KIND>) instead of fieldName seems more consistently brief.

>@@ -1070,18 +1027,18 @@ extern JSErrorFormatString js_ErrorForma
> /* Relative operations weights. */
> #define JSOW_JUMP                   1
> #define JSOW_ALLOCATION             100
> #define JSOW_LOOKUP_PROPERTY        5
> #define JSOW_GET_PROPERTY           10
> #define JSOW_SET_PROPERTY           20
> #define JSOW_NEW_PROPERTY           200
> #define JSOW_DELETE_PROPERTY        30
>-#define JSOW_ENTER_SHARP            4096
>-#define JSOW_SCRIPT_JUMP            4096
>+#define JSOW_ENTER_SHARP            JS_OPERATION_WEIGHT_BASE
>+#define JSOW_SCRIPT_JUMP            JS_OPERATION_WEIGHT_BASE

From another patch, which I thought landed already?

r+a=me with nits picked.

/be
Comment 3 Igor Bukanov 2008-01-07 05:42:11 PST
(In reply to comment #2)
> From another patch, which I thought landed already?

I had to back it out once again due to Talos failures, see bug 409109 comment 21.
Comment 4 Igor Bukanov 2008-01-07 05:56:06 PST
Created attachment 295760 [details] [diff] [review]
v2

The new version fixes JS_PUSH_TEMP_ROOT_COMMON macro that was without () around the cnt parameter and rewrites comments to better reflect the reality with 5 out 12 PUSH macros rooting non-GC things.
Comment 5 Igor Bukanov 2008-01-07 06:03:33 PST
Created attachment 295761 [details] [diff] [review]
v1-v2 delta

This is a delta between v1 merged with the trunk and v2.
Comment 6 Igor Bukanov 2008-01-07 06:06:08 PST
Created attachment 295763 [details] [diff] [review]
v1-v2 delta for real
Comment 7 Daniel Veditz [:dveditz] 2008-01-09 22:07:14 PST
From comment 0 it sounds like we need a 1.8 fix, too.
Comment 8 Igor Bukanov 2008-01-19 16:44:04 PST
Created attachment 298040 [details] [diff] [review]
v3

Here is a trunk-synchronized version of the patch that I will check in.
Comment 10 Samuel Sidler (old account; do not CC) 2008-02-12 20:26:26 PST
Igor, do you have time to make a branch patch for this? Or does v3 apply cleanly?
Comment 11 Igor Bukanov 2008-02-13 07:24:56 PST
(In reply to comment #10 by Samuel Sidler)
> Igor, do you have time to make a branch patch for this? Or does v3 apply
> cleanly?
> 

I plan to have a branch patch later today.
Comment 12 Igor Bukanov 2008-02-13 13:58:32 PST
Created attachment 303119 [details] [diff] [review]
v3 for the 1.8.1 branch

The essence of the fix in jsapi.c required a substantial hand-merge. The rest of changes is renaming and removal of non-existing on 1.8.1 branch features to adjust the re-factoring of TVR macros from the trunk. Strictly speaking this is not necessary, but it is rather safe and until 1.8.1 is supported I prefer to sync TVR-related code.
Comment 13 Igor Bukanov 2008-02-13 14:12:35 PST
Created attachment 303122 [details] [diff] [review]
plain diff of trunk and 181 patches
Comment 14 Brendan Eich [:brendan] 2008-02-13 14:59:16 PST
Comment on attachment 303119 [details] [diff] [review]
v3 for the 1.8.1 branch

r=me, thanks.

/be
Comment 15 Daniel Veditz [:dveditz] 2008-02-15 11:28:20 PST
Comment on attachment 303119 [details] [diff] [review]
v3 for the 1.8.1 branch

approved for 1.8.1.13, a=dveditz for release-drivers
Comment 17 Al Billings [:abillings] 2008-03-13 18:05:57 PDT
Is there any way to verify this change in the release candidates for Firefox 2.0.0.13?
Comment 18 Igor Bukanov 2008-03-13 21:11:06 PDT
(In reply to comment #17)
> Is there any way to verify this change in the release candidates for Firefox
> 2.0.0.13?
> 

I am not aware about a simple test case that can do that. It should be possible to write one, but I do not think that it is a productive use of time.
Comment 19 Alexander Sack 2008-03-23 09:27:25 PDT
Created attachment 311259 [details] [diff] [review]
for 1.8.0

i tried to merge the 1.8 patch (attachment 303119 [details] [diff] [review]) down to 1.8.0. In addition to what the 1.8 version backported, I needed to backport the JS_STATIC_ASSERT macro.
Comment 20 Alexander Sack 2008-03-23 09:28:12 PDT
Created attachment 311260 [details] [diff] [review]
1.8 to 1.8.0 plaindiff

for your convenience ...
Comment 21 Igor Bukanov 2008-03-23 16:27:13 PDT
(In reply to comment #19)
> Created an attachment (id=311259) [details]
> for 1.8.0
> 
> i tried to merge the 1.8 patch (attachment 303119 [details] [diff] [review]) down to 1.8.0. In addition
> to what the 1.8 version backported, I needed to backport the JS_STATIC_ASSERT
> macro.

In the past when backporting the code with JS_STATIC_ASSERT to 1.8.0 branch I simply was removing the macro. Some compilers had troubles with it so to avoid the risk of regressions with other embeddings the macro was not introduced on the branch. But I do not know if this is an issue now.
Comment 22 Alexander Sack 2008-03-24 11:00:48 PDT
Comment on attachment 311259 [details] [diff] [review]
for 1.8.0

taking this for distro patches, caillon please sign off.
Comment 23 Alexander Sack 2008-03-24 11:02:24 PDT
caillon, if you are experiencing the compiler issues mentioned in comment #21 on any of your supported platforms let me know. Maybe just strip the JS_STATIC_ASSERT macros before landing then.
Comment 24 Blake Kaplan (:mrbkap) 2008-03-28 00:20:05 PDT
Comment on attachment 303119 [details] [diff] [review]
v3 for the 1.8.1 branch

Here's a potential problem that could cause crashes during GC:

>@@ -4057,37 +4057,41 @@ JS_CompileUCFunctionForPrincipals(JSCont
>     fun = js_NewFunction(cx, NULL, NULL, nargs, 0, obj, funAtom);
>     if (!fun)
>         goto out;
>+
>+    /* From this point the control must flow through the label out. */
>+    JS_PUSH_TEMP_ROOT_FUNCTION(cx, fun, &tvr);
...
>+  out:
>+    cx->weakRoots.newborn[GCX_PRIVATE] = (JSGCThing *) fun;
>+    JS_POP_TEMP_ROOT(cx, &tvr);

But in this case, we haven't pushed the temp root or the tvr or the temp root stack, which can cause all sorts of problems.
Comment 25 Blake Kaplan (:mrbkap) 2008-03-28 00:27:03 PDT
Comment on attachment 303119 [details] [diff] [review]
v3 for the 1.8.1 branch

>@@ -4107,17 +4111,22 @@ JS_CompileUCFunctionForPrincipals(JSCont
>     }
>     if (obj && funAtom) {
>         if (!OBJ_DEFINE_PROPERTY(cx, obj, ATOM_TO_JSID(funAtom),
>                                  OBJECT_TO_JSVAL(fun->object),
>                                  NULL, NULL, JSPROP_ENUMERATE, NULL)) {
>             return NULL;
>         }
>     }
>-out:
>+
>+  out:
>+    cx->weakRoots.newborn[GCX_PRIVATE] = (JSGCThing *) fun;
>+    JS_POP_TEMP_ROOT(cx, &tvr);

Sorry for the spam, didn't see this before... the |return NULL| there really wants to be |fun = NULL|.
Comment 26 timeless 2008-03-28 00:27:48 PDT
Created attachment 312220 [details] [diff] [review]
use the right out label
Comment 27 timeless 2008-03-28 00:30:52 PDT
Created attachment 312221 [details] [diff] [review]
for 1.8.0
Comment 28 timeless 2008-03-28 00:36:04 PDT
Created attachment 312222 [details] [diff] [review]
v3.02 for the 1.8.1 branch
Comment 29 timeless 2008-03-28 00:39:27 PDT
Created attachment 312223 [details] [diff] [review]
for 1.8.0 (out2, null fun instead of violating out constraints)
Comment 30 timeless 2008-04-01 03:35:26 PDT
bug 425576 ended up taking the next regression set.
Comment 31 timeless 2008-04-09 07:28:03 PDT
Comment on attachment 312223 [details] [diff] [review]
for 1.8.0 (out2, null fun instead of violating out constraints)

per caillon we'll use this bug...
Comment 32 Christopher Aillon (sabbatical, not receiving bugmail) 2008-04-09 10:18:32 PDT
Comment on attachment 312223 [details] [diff] [review]
for 1.8.0 (out2, null fun instead of violating out constraints)

a=caillon for 1.8.0.15
Comment 33 Alexander Sack 2008-04-17 12:19:54 PDT
Comment on attachment 312223 [details] [diff] [review]
for 1.8.0 (out2, null fun instead of violating out constraints)

this patch appears not to include the latest from bug 425576 (v2) ... removing approval for now as this has not yet landed on 1.8.0 branch.

timeless, can we just take the v2 patch on top of the original one we had here? or did you add something else?

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