357169 - GC temp roots cleanup

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Currently JS_PUSH_SINGLE_TEMP_ROOT macro casts its argument to jsval to allow to root arbitrary GC thing. But this may hide a bugs when improper value is passed to the macro. Thus the idea is to add JS_PUSH_TEMP_ROOT_STRING and JS_PUSH_TEMP_ROOT_GCTHING and remove the cast from JS_PUSH_SINGLE_TEMP_ROOT.

Comment 1

[Igor Bukanov]

Attachment: Work in progress

The patch adds the new macros, switches AddRoot/RemoveRoot pair from jsscan.c to use temp root instead and moves implementation of root mapping and dumping to jsgc.c not to leak too much implementation details from jsgc.c

Comment 2

[Igor Bukanov]

Attachment: Implementation v1

Compared with the previous patch this version switches js_ReportUncaughtException to use JSTempValueRooter instead of AllocStack for local roots and removes  
JSGCLockHashEntry and JSGCRootHashEntry declarations from jsprvtd.h. 

Another chunk of changes comes from movement of InitGC/FinishGC after AddRoot/RemoveRoot and related methods to avoid forward declarations.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 242764 [details] [diff] [review]
Implementation v1

>+ * JS_PUSH_TEMP_ROOT_OBJECT and JS_PUSH_TEMP_ROOT_STRING are type-safe
>+ * alternatives to JS_PUSH_TEMP_ROOT_GCTHING for JSObject * and JSString *.

"... for JSObject and JSString roots."

>+ * They also provide a simple way to get a single pointer to rooted JSObject *
>+ * or JSString * via  JS_PUSH_TEMP_ROOT_OBJECT(cx, NULL, &tvr) or

Don't need the * after JSObject and JSString above to add a level of indirection.

>+ * JS_PUSH_TEMP_ROOT_STRING(cx, NULL, &tvr). Then &tvr.u.object or
>+ * tvr.u.string give the necessary pointer, which puns tvr.u.value safely
>+ * because JSObject * and JSString * are GC-things and as such their tag bits

"are GC-thing pointers, and as such ...."

>diff -U7 -p -r3.179 jsgc.c
>--- jsgc.c	7 Oct 2006 19:14:55 -0000	3.179
>+++ jsgc.c	19 Oct 2006 14:46:50 -0000
>@@ -586,14 +586,208 @@ js_ChangeExternalStringFinalizer(JSStrin
>         if (gc_finalizers[i] == (GCFinalizeOp) oldop) {
>             gc_finalizers[i] = (GCFinalizeOp) newop;
>             return (intN) i;
>         }
>     }
>     return -1;
> }
>+/* This is compatible with JSDHashEntryStub. */

Need a blank line before the comment above. Also, is this all moved only because of CheckLeakedRoots?  Might be nicer to leave the root hash stuff where it was and forward-declare that static helper.

>+++ jsscan.c	19 Oct 2006 14:46:54 -0000
>@@ -527,14 +527,15 @@ MatchChar(JSTokenStream *ts, int32 expec
> }
> 
> static JSBool
> ReportCompileErrorNumber(JSContext *cx, void *handle, uintN flags,
>                          uintN errorNumber, JSErrorReport *report,
>                          JSBool charArgs, va_list ap)
> {
>+    JSTempValueRooter lineTvr;

Nit in light of other local names (mostly linestr; onError is contrary, granted): linetvr would be more consistent with local names such as linestr.

Looks great otherwise, thanks.

/be

Comment 4

[Igor Bukanov]

Attachment: Implementation v2

Addressing the nits and minimizing code movements.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 243716 [details] [diff] [review]
Implementation v2


>+ * JS_PUSH_TEMP_ROOT_OBJECT and JS_PUSH_TEMP_ROOT_STRING are type-safe
>+ * alternatives to JS_PUSH_TEMP_ROOT_GCTHING for JSObject and JSString. They
>+ * also provide a simple way to get a single pointer to rooted JSObject or
>+ * JSString via  JS_PUSH_TEMP_ROOT_OBJECT(cx, NULL, &tvr) or
>+ * JS_PUSH_TEMP_ROOT_STRING(cx, NULL, &tvr). Then &tvr.u.object or
>+ * tvr.u.string give the necessary pointer, which puns tvr.u.value safely
>+ * because JSObject * and JSString * are GC-things and, as such their tag bits
>  * are all zeroes.

Final nit: wrap to shorter tw than 78?  Anyway, lose the extra space before
JS_PUSH_TEMP_ROOT_OBJECT(cx, NULL, &tvr), and s/give/gives/.

r=me, thanks.

/be

Comment 6

[Igor Bukanov]

Attachment: Implementation v2b

Patch to commit with comments reformatted.

Comment 7

[Igor Bukanov]

I committed the patch from comment 6 to the trunk:

Checking in jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.285; previous revision: 3.284
done
Checking in jsarray.c;
/cvsroot/mozilla/js/src/jsarray.c,v  <--  jsarray.c
new revision: 3.99; previous revision: 3.98
done
Checking in jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.124; previous revision: 3.123
done
Checking in jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.80; previous revision: 3.79
done
Checking in jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision: 3.170; previous revision: 3.169
done
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.180; previous revision: 3.179
done
Checking in jsgc.h;
/cvsroot/mozilla/js/src/jsgc.h,v  <--  jsgc.h
new revision: 3.59; previous revision: 3.58
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.300; previous revision: 3.299
done
Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.299; previous revision: 3.298
done
Checking in jsprvtd.h;
/cvsroot/mozilla/js/src/jsprvtd.h,v  <--  jsprvtd.h
new revision: 3.21; previous revision: 3.20
done
Checking in jsregexp.c;
/cvsroot/mozilla/js/src/jsregexp.c,v  <--  jsregexp.c
new revision: 3.127; previous revision: 3.126
done
Checking in jsscan.c;
/cvsroot/mozilla/js/src/jsscan.c,v  <--  jsscan.c
new revision: 3.116; previous revision: 3.115
done
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.135; previous revision: 3.134
done

Comment 8

[Igor Bukanov]

Attachment: Implementation v12b + fix from bug 358528

This is a patch for 1.8.1 branch that includes the fix for the regression reported in bug 358528.

Comment 9

[Brendan Eich [:brendan]]

How'd I miss that?  Skimming, sorry.

/be

Comment 10

[Evan Yan]

Sun compiler complains:

"../../../mozilla/js/src/jsapi.c", line 1805: void function cannot return value cc: acomp failed for ../../../mozilla/js/src/jsapi.c 

Comment 11

[Evan Yan]

Attachment: simple patch

Comment 12

[Igor Bukanov]

(In reply to comment #11)
> Created an attachment (id=244294) [edit]
> simple patch

This should go to bug 358867 as regression where Brendan already created a patch.

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> (In reply to comment #11)
> > Created an attachment (id=244294) [edit]
> > simple patch
> 
> This should go to bug 358867 as regression where Brendan already created a
> patch.

Alfred did.

Evan: just to be clear, REOPENing a bug is best done when the bug was not in fact fixed by the patch that landed.  A further problem introduced by the patch, or any other recurrence that shows an unpatched bug, should be filed anew.

/be

Comment 14

[Evan Yan]

clear. thanks for explanation.

Comment 15

[Daniel Veditz [:dveditz]]

Brendan: you nominated this for blocking, but it's severity is set to "enhancement". Do we really need this in 2.0.0.1?

Comment 16

[Brendan Eich [:brendan]]

Igor can say whether this is causing pain merging trunk fixes that are wanted.  We want this patch for js1.7src, but we could mini-branch if we have to (rather match 1.8.1.1 exactly).

If you ignore all the DEBUG-related root-dumping changes, it's a small patch, which improves clarity and efficiency.  I think you should take it.

/be

Comment 17

[Jay Patel [:jay]]

Comment on attachment 243914 [details] [diff] [review]
Implementation v12b + fix from bug 358528

Approved for 1.8.1 branch, a=jay for drivers.  Please land asap.  Thanks!

Comment 18

[Igor Bukanov]

Bug 358528 is a regression so it must depend on this bug, not block the bug.

Comment 19

[Igor Bukanov]

Bug 358867 is debug-only compiler-only regression that I missed to include into the blocking list. 

Comment 20

[Igor Bukanov]

Attachment: Implementation v12b + fix from bug 358528 + fix from bug 358867

The missed dependency forces asking one more approval.

Comment 21

[Jay Patel [:jay]]

Comment on attachment 246739 [details] [diff] [review]
Implementation v12b + fix from bug 358528 + fix from bug 358867

Approved for 1.8.1 branch, a=jay for drivers.

Comment 22

[Daniel Veditz [:dveditz]]

Believe it or not, we make regressions -block- the original bug rather than the other way around: to correctly fix this bug you really need to fix the others too.

Helps in trying to land bugs on old branches.

Comment 23

[Igor Bukanov]

I committed the patch from comment 20 to MOZILLA_1_8_BRANCH: 

Checking in jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.214.2.31; previous revision: 3.214.2.30
done
Checking in jsarray.c;
/cvsroot/mozilla/js/src/jsarray.c,v  <--  jsarray.c
new revision: 3.58.2.23; previous revision: 3.58.2.22
done
Checking in jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.80.4.19; previous revision: 3.80.4.18
done
Checking in jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.50.4.12; previous revision: 3.50.4.11
done
Checking in jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision: 3.117.2.29; previous revision: 3.117.2.28
done
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.104.2.29; previous revision: 3.104.2.28
done
Checking in jsgc.h;
/cvsroot/mozilla/js/src/jsgc.h,v  <--  jsgc.h
new revision: 3.33.4.10; previous revision: 3.33.4.9
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.76; previous revision: 3.181.2.75
done
Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.208.2.45; previous revision: 3.208.2.44
done
Checking in jsprvtd.h;
/cvsroot/mozilla/js/src/jsprvtd.h,v  <--  jsprvtd.h
new revision: 3.17.20.3; previous revision: 3.17.20.2
done
Checking in jsregexp.c;
/cvsroot/mozilla/js/src/jsregexp.c,v  <--  jsregexp.c
new revision: 3.99.2.19; previous revision: 3.99.2.18
done
Checking in jsscan.c;
/cvsroot/mozilla/js/src/jsscan.c,v  <--  jsscan.c
new revision: 3.81.2.27; previous revision: 3.81.2.26
done
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.50.2.57; previous revision: 3.50.2.56
done

Comment 24

[Jay Patel [:jay]]

This appears to have caused regression bug 358661.