Last Comment Bug 432915 - Analyze SpiderMonkey Push/pop for tvr & lock/unlock/etc
: Analyze SpiderMonkey Push/pop for tvr & lock/unlock/etc
Status: RESOLVED WONTFIX
:
Product: Core
Classification: Components
Component: Rewriting and Analysis (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
Depends on: 435546 438645
Blocks: analyses
  Show dependency treegraph
 
Reported: 2008-05-08 16:47 PDT by (dormant account)
Modified: 2013-02-25 14:57 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Treeehydra script for finding mismatched constructor/destructor type things (2.50 KB, text/plain)
2008-05-12 15:28 PDT, (dormant account)
no flags Details

Description (dormant account) 2008-05-08 16:47:14 PDT
Will get a list of functions from Igor that are function-local
Comment 1 (dormant account) 2008-05-08 16:55:00 PDT
try to inline InitTempNSArray, etc to see if gcc will show it as part of the cfg.
Comment 2 (dormant account) 2008-05-12 15:23:37 PDT
Igor, just ran this on actionmonkey's C++ spidermonkey(which is a little out of date) and didn't find anything wrong. 
The functions I used were
/^JS_PUSH/ -> "JS_POP_TEMP_ROOT" (these defines were turned into static functions for analysis purposes
/^InitTempNSArray$/ -> "FinishTempNSArray"


What are the other functions that act on stack variables that you'd like to check?
Comment 3 (dormant account) 2008-05-12 15:28:31 PDT
Created attachment 320606 [details]
Treeehydra script for finding mismatched constructor/destructor type things
Comment 4 Jason Orendorff [:jorendorff] 2008-05-13 08:22:15 PDT
actionmonkey's SpiderMonkey has deleted most TempValueRooters, as they're not needed anymore with the conservative stack-scanning GC.
Comment 5 Igor Bukanov 2008-05-13 08:59:18 PDT
(In reply to comment #2)
...
> /^InitTempNSArray$/ -> "FinishTempNSArray"
> 
> 
> What are the other functions that act on stack variables that you'd like to
> check?

Another famous-for-bugs pair is OBJ_LOOKUP_PROPERTY(cx,obj,id,objp,propp) and OBJ_DROP_PROPERTY(cx,obj,prop) macros.

But the rules here requires heavy control flow analysis since OBJ_DROP_PROPERTY must be called only and only if OBJ_LOOKUP_PROPERTY returns true and sets prop to non-null value. In addition in few places OBJ_LOOKUP_PROPERTY is hidden besides helper functions. If this is not feasible, we could at least annotate code with OBJ_MUST_DROP_PROPERTY(obj, prop) right after the checks for non-null prop. That can be combined with the requirement that OBJ_MUST_DROP_PROPERTY must have preceding OBJ_MUST_DROP_PROPERTY. 

Plus there is that JS_LOCK/JS_UNLOCK. But it definitely requires propagation from the function return. I guess this implies that the functions that returns with object locked must be annotated if we want to stay with a function-local analysis.



Comment 6 (dormant account) 2008-05-13 14:24:27 PDT
(In reply to comment #5)
> (In reply to comment #2)
> ...
> > /^InitTempNSArray$/ -> "FinishTempNSArray"
> > 
> > 
> > What are the other functions that act on stack variables that you'd like to
> > check?
> 
> Another famous-for-bugs pair is OBJ_LOOKUP_PROPERTY(cx,obj,id,objp,propp) and
> OBJ_DROP_PROPERTY(cx,obj,prop) macros.
> 
> But the rules here requires heavy control flow analysis since OBJ_DROP_PROPERTY
> must be called only and only if OBJ_LOOKUP_PROPERTY returns true and sets prop
> to non-null value. In addition in few places OBJ_LOOKUP_PROPERTY is hidden
> besides helper functions. If this is not feasible, we could at least annotate
> code with OBJ_MUST_DROP_PROPERTY(obj, prop) right after the checks for non-null

So lets treat the helper functions same as OBJ_LOOKUP_PROPERTY?
Also we can detect null/non-null in most cases. However we do have to annotate the functions to indicate that they don't leak the pointer when it's assigned through an outparam.

> prop. That can be combined with the requirement that OBJ_MUST_DROP_PROPERTY
> must have preceding OBJ_MUST_DROP_PROPERTY. 

> 
> Plus there is that JS_LOCK/JS_UNLOCK. But it definitely requires propagation
> from the function return. I guess this implies that the functions that returns
> with object locked must be annotated if we want to stay with a function-local
> analysis.

Actually looks like JS_LOCK/JS_UNLOCK are function local. perhaps other lock/unlock functions aren't.
Comment 7 (dormant account) 2008-05-23 16:55:52 PDT
I think i found a rooting bug
http://mxr.mozilla.org/mozilla/source/js/src/jsinterp.c#935?
Comment 8 (dormant account) 2008-05-23 16:57:34 PDT
I guess I should mention that I'm checking push/pop on tvr with JS_PUSH_SINGLE_TEMP_ROOT in that method
Comment 9 Brendan Eich [:brendan] 2008-05-23 17:16:31 PDT
Line 935 has shifted recently -- which rev are you using?

/be
Comment 10 (dormant account) 2008-05-23 17:35:02 PDT
i'm using some old rev, but i'm assuming mxr is current and there shouldn't be a return JS_FALSE there in js_OnUnknownMethod(JSContext *cx, jsval *vp) as that doesn't flow through out
Comment 11 Brendan Eich [:brendan] 2008-05-23 17:44:39 PDT
Oh I see! Yeah, that's a bug. Igor, can you file and fix?

/be
Comment 12 (dormant account) 2008-06-30 16:29:53 PDT
For my notes:
jst mentions

nsI[Thread]JSContextStack::Push/Pop would be another one that'd be good to pay attention to.

example:
http://mxr.mozilla.org/mozilla-central/source/dom/src/base/nsJSEnvironment.cpp#1327

Comment 13 Michael Kohler [:mkohler] 2010-05-13 10:08:09 PDT
This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.
Comment 14 Jeff Walden [:Waldo] (remove +bmo to email) 2013-02-25 14:57:27 PST
Property holding/dropping is gone these days.  We use RAII for tvr push/popping.  All is right with the world.

(...okay, maybe I shouldn't have gone that far.  :-) )

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