Analyze SpiderMonkey Push/pop for tvr & lock/unlock/etc

RESOLVED WONTFIX

Status

()

Core
Rewriting and Analysis
RESOLVED WONTFIX
9 years ago
4 years ago

People

(Reporter: (dormant account), Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
Will get a list of functions from Igor that are function-local
(Reporter)

Comment 1

9 years ago
try to inline InitTempNSArray, etc to see if gcc will show it as part of the cfg.
(Reporter)

Comment 2

9 years ago
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?
Status: NEW → ASSIGNED
(Reporter)

Comment 3

9 years ago
Created attachment 320606 [details]
Treeehydra script for finding mismatched constructor/destructor type things
actionmonkey's SpiderMonkey has deleted most TempValueRooters, as they're not needed anymore with the conservative stack-scanning GC.

Comment 5

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



(Reporter)

Comment 6

9 years ago
(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.
(Reporter)

Comment 7

9 years ago
I think i found a rooting bug
http://mxr.mozilla.org/mozilla/source/js/src/jsinterp.c#935?
(Reporter)

Comment 8

9 years ago
I guess I should mention that I'm checking push/pop on tvr with JS_PUSH_SINGLE_TEMP_ROOT in that method
Line 935 has shifted recently -- which rev are you using?

/be
(Reporter)

Comment 10

9 years ago
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
Oh I see! Yeah, that's a bug. Igor, can you file and fix?

/be

Updated

9 years ago
Depends on: 435546
(Reporter)

Updated

9 years ago
Depends on: 438645
(Reporter)

Comment 12

9 years ago
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

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.
Status: ASSIGNED → NEW
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.  :-) )
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.