Friday, 7 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.



Posted by Robert at 12:43 AM
Labels:

10 comments:

  1. This all looks good except for
    "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!
    ReplyDelete
  2. Robert O'CallahanDec 6, 2007 02:09 PM
    I don't have any specific examples in mind but I'm sure we'll find cases where we know we can call an "unsafe" method and it won't do anything unsafe.
    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.
    ReplyDelete
  3. >>> Running JS code during garbage collection is a very bad idea, so we crash.
    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.
    ReplyDelete
  4. I don't see why we'd want to mark an interface as being unsafe to call in certain ways due to a (possibly-transient) choice in one implementation of that interface. 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. If this nsIWhatever instance had been implemented in C++ (or in Python, which is safe to call from under the JS GC, but not from under the Python one, I imagine), would you still flag it? Will we need to add these annotations every time we shift implementation language (which I hope to be quite common once we're running atop MMGC and Tamarin)? Would we remove them if we switched things back?
    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.
    ReplyDelete
  5. James: concurrency is not the issue, and there's more here than whether a finalizer may allocate. Neither SpiderMonkey (non-incremental) nor MMgc (incremental) allows allocation from a finalizer. As roc's post mentions, there are other kinds of execution restrictions we would like to impose, for e.g. controlling effects to uphold security properties, or (my favorite) to do better JITting across native calls.
    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
    ReplyDelete
  6. Robert O'CallahanDec 7, 2007 05:41 AM
    shaver:
    > 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.
    ReplyDelete
  7. Brendan: the interface annotation would be saying that _any_ implementation of the interface _might_ be finalizer-like (or the inverse as I think roc is describing -- not safe to be called in a finalization context).
    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.
    ReplyDelete
  8. Robert O'CallahanDec 8, 2007 10:56 AM
    "any scriptable interface method (incl accessors) can obviously be implemented in script" --- I don't see why.
    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 :-).
    ReplyDelete
  9. I've already specced up a static checking pass for finalizer safety and other things in XPCOMGC: http://wiki.mozilla.org/XPCOMGC/Static_Checker
    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.
    ReplyDelete
  10. The annotation system we're trying to implement for XPCOMGC is at http://wiki.mozilla.org/XPCOMGC/Static_Checker
    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.
    ReplyDelete

Subscribe to: Post Comments (Atom)