1119919 - Have some sort of static analysis for JSAPI uses

Status: NEW

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Basically, a requirement to annotate JSContext* arguments with some sort of "this has been reviewed" annotation, with the following exceptions:

1)  Code in js/src
2)  Code in dom/bindings

I thought about Ehsan's suggestion of not flagging if we just pass to JS_*, but the whole point is to make sure people aren't misusing the JS_*....

Thoughts?  Is this crazy?

Comment 1

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #0)
> Basically, a requirement to annotate JSContext* arguments with some sort of
> "this has been reviewed" annotation, with the following exceptions:
> 
> 1)  Code in js/src
> 2)  Code in dom/bindings

Makes sense.  And also js/public of course.  And js/xpconnect.  Maybe js/*.

We probably want to ignore InheritingFromnsWrapperCache::WrapObject.  Anything else?

> I thought about Ehsan's suggestion of not flagging if we just pass to JS_*,
> but the whole point is to make sure people aren't misusing the JS_*....

FWIW that was just a silly example, I wasn't actually suggesting that.  My point was if there are specific JSAPIs that you just can't screw up, we could allow their usage.  :-)

> Thoughts?  Is this crazy?

So I took a look and there is a *ton* of code that we'd have to annotate.  And presumably we want to review the usages as we're annotating.  The only issue that I can see with this is who will be responsible for doing all of those reviews...

Comment 2

[Bobby Holley (:bholley)]

A big source of this is going to be rooting, which we need for dictionaries etc. In reality rooting should just happen with the runtime, but some of rooted classes only accept a cx, not an rt (for no good reason).

I didn't have time to fix it, so I asked huseby to add nsContentUtils::RootingCx{,ForThread}. We could annotate that somehow, or just find a better solution.