Value[] foo = { ... }; is currently being entirely ignored, because it shows up in the analysis as an "Array", and the analysis only knows "Pointer" and "CSU" (Class/Struct/Union.)

Large bump in the number of hazards.

Comment on attachment 8365276 [details] [diff] [review] Handle Arrays in the analysis Review of attachment 8365276 [details] [diff] [review]: ----------------------------------------------------------------- r=me

I've verified that all identified hazards are false positives or in tests, so opening up.

This is all the low-hanging fruit. I have one other largish patch for nsJSNPRuntime, which I've split off for gecko peer review. There are three remaining hazards which are all false positives for which simple code reorganization will not work. We'll need to think of some other mechanism(s) for those.

Try run for the stack: https://tbpl.mozilla.org/?tree=Try&rev=f29bb9b0d9b6

Will flag this for review when the try run is green.

Comment on attachment 8366950 [details] [diff] [review] hazard_npapi-v0.diff Review of attachment 8366950 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/plugins/base/nsJSNPRuntime.cpp @@ -634,5 @@ > } else { > fv = OBJECT_TO_JSVAL(npjsobj->mJSObj); > } > > - JS::Value jsargs_buf[8]; Could we use AutoValueVector here? It looks like it will handle allocating the elements possibly-inline for us.

Comment on attachment 8366919 [details] [diff] [review] hazards_array-v0.diff Review of attachment 8366919 [details] [diff] [review]: ----------------------------------------------------------------- This all looks good.

I noticed the try run was failing. It turns out this was caused by the fix for bug 962256, so I kicked off a new one without that patch: https://tbpl.mozilla.org/?tree=Try&rev=916abaa43b1f

Try looks good: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ce4d2dd81858

Great idea, Jon! The default inline count is even 8, so we don't even need to pass through new template args to get identical behavior.

This time with more qreffing.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7400843fb1c0

And we're good now.

Whoops, this should never have been closed. The actual hazards have been fixed, but the analysis patch never landed because there were some nuisance false positives left.

When adding in stack arrays to the analysis, I hit a false positive that looked something like this: GCPointer localArray[10]; GCPointer *data = nullptr; localArray[3] = JS::NewGCThing(...); if (...) data = localArray; canGC(); if (data != localArray) free(data); The idea is that you get a fixed-size buffer on the stack to store a set of GC pointers, and if you need more space it gets malloced. The analysis sees this as a hazard, because 'localArray' is live after the canGC() call due to its use in the |if (data != localArray)| call. But its *contents* are never examined, so this is a false positive -- if a GC thing in localArray were moved, nothing would ever see it. So I'd like to allow this use by saying that Assume(...var...) doesn't extend var's live range, unless it's in the form of Assume(...*var...). I believe that this might also help with situations like: JSObject *raw = ...; RootedObject rooted(cx, raw); canGC(); if (rooted != raw) { ...do expensive fixup stuff... } though perhaps the fixup stuff will always end up using 'raw' in a way that extends the live range? I haven't looked at examples. (The attached patch does not ignore any Assign or Call that uses the variable without dereferencing it.)

Ok, now that I'm all educated on interning (pinning) and understand that this is not a hazard because of the interning (pinning), I agree that this should just be annotated away. The most precise annotation I could come up with is for XPCNativeMember (since I didn't want to annotate away jsid entirely, even though an argument could be made for it.) But it's a weird annotation. And I hope to stop pinning these jsids, in which case we'll need to root here after all. But that will also require fixing NPIdentifier, so I stuck in an annotation for it at the same time. (The analysis currently doesn't see it because the compiler only sees it as a void*.) That way, I'll remember to remove both annotations at the same time.

https://hg.mozilla.org/mozilla-central/rev/b471ab566d1c https://hg.mozilla.org/mozilla-central/rev/be2e7aeee256 https://hg.mozilla.org/mozilla-central/rev/3ac7d11365a2