Thursday 6 December 2007
Script Safety Annotations
I've been encountering a trunk bug where we make a call to an XPCOM method that happens to be implemented in Javascript. Unfortunately we make this call while we're tearing down a DOM element, during garbage collection. Running JS code during garbage collection is a very bad idea, so we crash.
This is a specific case of a general problem: some code is not safe to call at certain times. I would like to be able to add checkable annotations to functions and methods that indicate the following:
- The function may spin the event loop or execute any script (these are equivalent).
- The function may execute trusted script but will not spin the event loop or execute untrusted script.
- The function will not execute script or spin the event loop (default, so dangerous methods have to be annotated).
We would need a way to specify these attributes on XPCOM IDL interface methods. (It would also be useful to have syntax for setting a default annotation for all methods of an interface.) Most of the scriptable Gecko API methods implemented in C++ could use these annotations. This might help Taras' refactoring work. It could help code analysis tools quite a lot by constraining what can run. I think it would help us catch a lot of bugs where we make unsafe calls.
We would want an escape hatch to "cast away" annotation checks, where some dynamic test guarantees that it's OK to call nominally less-safe code.
Comments
"We would want an escape hatch to "cast away" annotation checks, where some dynamic test guarantees that it's OK to call nominally less-safe code." Could you clarify that?
To me that reads that not-scriptable will actually mean not-scriptable-except-sometimes.
I added annotation support to dehydra recently so if xpidlgen produced annotations in the generated files, that'd be even better!
OK, here's an example: callbacks. Our choices are:
-- duplicate every function that takes a callback parameter when some callers pass a safe callback and other callers pass an unsafe callback
-- add safety polymorphism to the annotations and checker
-- cast away unsafeness somewhere
If there's not too much of this, the last option is the most sensible.
Does SpiderMonkey use an incremental garbage collector? If that's the case, perhaps that is the real problem here and you should consider switching to a concurrent garbage collector.
The safety issue is transitive as well, so I need to know if any of the interfaces on which my implementation operates can be implemented by anything that might fly an alert or be access a JS property. Would that not degenerate quickly into pretty much anything being unsafe?
I think that when running under GC, we should be posting non-trivial operations to a work-queue, which can be processed once GC is complete. IIRC we do something similar with certain kinds of mutation under reflow. Static analysis could possibly help us find cases where we operate under GC on interfaces for which we know of an implementation rendering that operation unsafe, as a programming aid.
shaver: there's a problem today that finalization and prior mandatory protocols are conflated in destructors called from last-Release. We want to separate things, but along the way and forever after, we want finalizers' execution to be constrained. You're right that finalizers are concrete things, not abstract interfaces or methods, in general -- but sometimes the interface makes it clear that all impls are finalizer-like.
/be
> It's not a characteristic of the interface
> here, as I see it, that you can never poke an
> nsIWhatever when you have GC on the stack above
> you.
On the contrary, I think "this method must not be called during GC" is a very legitimate precondition for a method and deserves to be part of its public interface.
If we choose to make it a precondition for most of our methods, then so be it.
roc: then why not just use [scriptable] to mean what you're asking for, since any scriptable interface method (incl accessors) can obviously be implemented in script? Make it a static and/or runtime error to call XPConnect's thunking when there's GC on the stack.
Why is [scriptable] the right thing? I can easily imagine that we have GC-safe APIs which could be usefully exposed to script and are therefore [scriptable]. Just because an API *can* be called during GC doesn't mean it *has* to be called during GC :-).
But I'm not convinced that we want to start dividing up IDL methods into multiple categories. We can solve last-release-causes-immediate-finalization issues much more simply than creating massive annotation graphs.
Note that in the case of XPCOMGC, you can't call any method on other GC-allocated objects, so the discussion about whether some IDL methods should be "GC-safe" is moot.
In the 1.9 timeframe, it sounds more effective to delay object destruction from last-release to a safe point, which is fairly easy to do.