898815 - Fix static analysis false positives due to ~AutoCompartment

Status: RESOLVED DUPLICATE of bug 924206

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

We used to use AutoEnterAtomsCompartment, which didn't do anything that could GC. That has been eliminated, and now plain AutoCompartment is used. But ~AutoCompartment wraps a pending exception, which can GC.

This reveals two different problems: first, we have a whole bunch of false positive rooting hazards, because most of the places previously wrapped with AutoEnterAtomsCompartment cannot set an exception (or cannot set an exception that would need to be wrapped, to be precise.) But there is at least one case where an exception is set that *does* need to be wrapped, and previously AutoEnterAtomsCompartment would have failed to wrap it, producing a compartment mismatch. SO we don't just want to go back to using a different RAII class.

My intended approach is to annotate ~AutoCompartment as "conditionally GCing", which means that it is not considered a GC function globally, but specific calls to the destructor *are* considered GC calls if anything in the RAII scope of the AutoCompartment could GC. Since anything that creates an exception can GC, this is pretty tight. The remaining cases should be covered by bug 898617.

I'm finding it a bit tricky to get right, but I believe I have this implemented now ABD (All But Debugging).

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Patch %s Pass functionName around instead of using a global variable

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Patch 1. Pass functionName around instead of using a global variable

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Patch 1 - Pass functionName around instead of using a global variable

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Patch 2 - Minor irrelevant cleanups to static rooting analysis

Comment 5

[Steve Fink [:sfink] [:s:]]

Attachment: Patch 3 - Generate multiple output files

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: Patch 4 - Refactor static rooting analysis to make the following patch cleaner

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Patch 5 - Mark ~AutoCompartment as GCing conditionally based on edges in the RAII scope

You're welcome to review the other patches too, but there's not much of interest.

Comment 8

[Brian Hackett [Laid off!]]

The patches in this bug seem to be adding a lot of complexity to the static analysis for what seems to me a pretty obscure case.  What is the case where the-class-formerly-known-as-AutoEnterAtomsCompartment might need to wrap a pending exception?

Comment 9

[Steve Fink [:sfink] [:s:]]

(In reply to Brian Hackett (:bhackett) from comment #8)
> The patches in this bug seem to be adding a lot of complexity to the static
> analysis for what seems to me a pretty obscure case.  What is the case where
> the-class-formerly-known-as-AutoEnterAtomsCompartment might need to wrap a
> pending exception?

To be honest, I don't think there is one anymore.

The one that got me going on this was AtomizeAndCopyChars, where the analysis showed a real bug because the allowGC=false version was potentially triggering a GC. Back when it was AutoEnterAtomsCompartment, I think that was a compartment violation, because it would not have gotten wrapped when exiting the atoms compartment. This is now fixed, because it can no longer trigger a GC.

The alternative to this patch is just whitelisting ~AutoCompartment (and AutoCompartment::AutoCompartment.) In fact, that's what this patch does: it annotates ~AutoCompartment as not GC'ing, then does control flow analysis to possibly add back in specific edges where it can GC after all.

With the current code, this adds exactly zero hazards, and causes only 4 functions to be marked as gcFunctions:

  JSObject::ReserveForTradeGuts
  JSObject::swap
  js::StartOffThreadParseScript
  JS::CompileOffThread

so for the current code, this isn't telling us anything new. Ugh. The more I think about it, the less useful this appears.

At this point, the main reason I'd want it in is because it's an example of a data flow analysis at a finer granularity than the call graph. It can be trivially disabled by removing the call to computeConditionalGCFunctions() (or earlier, remove the block containing the call to recordConditionalRAIIScopes()). In fact, I probably ought to move it into separate files. I think it has a bunch of reusable mechanisms for implementing similar analyses -- it preserves the ability to analyze files and functions in parallel, it provides functionality for dealing with RAII scopes, it allows specifying individual edges as GC-able (and has a way to attach reasons to those edges that will show up in the gcFunctions.txt report), and it (well, mostly the other patches) refactors some things I struggled to understand into something that I now feel I sort of have a grasp on.

Ok, and partly I wanted you to review it to point out the huge flaws in my understanding of what's going on, so I can be more useful in customizing the analysis in the future. I waded through the fire with this stuff, and I suspect some of the lessons I came out with aren't quite correct.

Still, you're right -- this doesn't really buy us much of anything beyond what whitelisting ~AutoCompartment would give us. I guess I'd still like a review for the reasons stated above, but I'm ok with not actually hooking any of this up. I can either put it in a file that never gets called, or just keep it for myself to crib from if we have a need for something similar in the future.

I really ought to get rid of the Map and Set usage, though. It was just the first thing to spring to mind.

Comment 10

[Steve Fink [:sfink] [:s:]]

Comment on attachment 792376 [details] [diff] [review]
Patch 1 - Pass functionName around instead of using a global variable

I landed these refactoring patches in bug 908825.

Comment 11

[Steve Fink [:sfink] [:s:]]

Comment on attachment 792383 [details] [diff] [review]
Patch 5 - Mark ~AutoCompartment as GCing conditionally based on edges in the RAII scope

I don't think there's anything left in this bug.