Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({fixed1.8.1.1})

Trunk
fixed1.8.1.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.1 -
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
Created attachment 242671 [details] [diff] [review]
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
(Assignee)

Comment 2

11 years ago
Created attachment 242764 [details] [diff] [review]
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.
Attachment #242671 - Attachment is obsolete: true
Attachment #242764 - Flags: review?(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

Updated

11 years ago
Blocks: 355044
Flags: blocking1.8.1.1?
(Assignee)

Comment 4

11 years ago
Created attachment 243716 [details] [diff] [review]
Implementation v2

Addressing the nits and minimizing code movements.
Attachment #242764 - Attachment is obsolete: true
Attachment #243716 - Flags: review?(brendan)
Attachment #242764 - Flags: review?(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
Attachment #243716 - Flags: review?(brendan) → review+
(Assignee)

Comment 6

11 years ago
Created attachment 243729 [details] [diff] [review]
Implementation v2b

Patch to commit with comments reformatted.
Attachment #243716 - Attachment is obsolete: true
Attachment #243729 - Flags: review+
(Assignee)

Comment 7

11 years ago
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Flags: in-testsuite-

Updated

11 years ago
Blocks: 358528
No longer blocks: 358528
Depends on: 358528
(Assignee)

Comment 8

11 years ago
Created attachment 243914 [details] [diff] [review]
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.
Attachment #243729 - Attachment is obsolete: true
Attachment #243914 - Flags: approval1.8.1.1?
How'd I miss that?  Skimming, sorry.

/be

Comment 10

11 years ago
Sun compiler complains:

"../../../mozilla/js/src/jsapi.c", line 1805: void function cannot return value cc: acomp failed for ../../../mozilla/js/src/jsapi.c 
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 11

11 years ago
Created attachment 244294 [details] [diff] [review]
simple patch
Attachment #244294 - Flags: review?(igor.bukanov)
(Assignee)

Comment 12

11 years ago
(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.

Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Attachment #244294 - Attachment is obsolete: true
Attachment #244294 - Flags: review?(igor.bukanov)
(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

11 years ago
clear. thanks for explanation.
Brendan: you nominated this for blocking, but it's severity is set to "enhancement". Do we really need this in 2.0.0.1?
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

Updated

11 years ago
Flags: blocking1.8.1.1? → blocking1.8.1.1-

Comment 17

11 years ago
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!
Attachment #243914 - Flags: approval1.8.1.1? → approval1.8.1.1+
(Assignee)

Comment 18

11 years ago
Bug 358528 is a regression so it must depend on this bug, not block the bug.
Blocks: 358528
No longer depends on: 358528
(Assignee)

Comment 19

11 years ago
Bug 358867 is debug-only compiler-only regression that I missed to include into the blocking list. 
Blocks: 358867
(Assignee)

Comment 20

11 years ago
Created attachment 246739 [details] [diff] [review]
Implementation v12b + fix from bug 358528 + fix from bug 358867

The missed dependency forces asking one more approval.
Attachment #243914 - Attachment is obsolete: true
Attachment #246739 - Flags: approval1.8.1.1?

Comment 21

11 years ago
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.
Attachment #246739 - Flags: approval1.8.1.1? → approval1.8.1.1+
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.
No longer blocks: 358528, 358867
Depends on: 358528, 358867
(Assignee)

Comment 23

11 years ago
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
Keywords: fixed1.8.1.1

Comment 24

11 years ago
This appears to have caused regression bug 358661.
Depends on: 358661
You need to log in before you can comment on or make changes to this bug.