1907665 - Add a nogc.resetOnError() annotation

Status: NEW

(Core :: JavaScript: GC, task, P3)

Description

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

In Gecko, it is becoming common to have a pattern like:

Result<Ok, nsresult> foo(Span<uint8_t> aUnstablePtr, AutoCheckCannotGC&& aNoGC) {
  ...
  lastUseOfPointer(std::move(aUnstablePtr)); // Or some simpler usage.
  aNoGC.reset();
  scaryCallThatMightGC();
  return Ok();
}

Which is fine. The aNoGC guard is basically paired up with the aUnstablePtr input, and is active until the last use of the input, at which point the it is manually reset() and everything is happy.

The problem is in the ... portion. It may look like:

  if (badthing1()) { REPORT_INTERNAL_ERROR; return Err(NS_ERROR_YOU_SUCK); }
  if (badthing2()) { REPORT_INTERNAL_ERROR; return Err(NS_ERROR_YOU_SUCK); }
  .
  .
  .

i.e., there are a bunch of different failure paths. (Actual name might be IDB_REPORT_INTERNAL_ERR or a direct non-macro call to some error reporting function.) Those failure paths may log something to the console, which can run JS code and therefore GC.

If this gets to be common, it would be helpful to have something like aNoGC.resetOnError() at the top of the function, which means any return of an error value will implicitly reset the aNoGC token instead of requiring it to be reset on every error path. It's not quite that simple, because it needs to be reset before the REPORT_INTERNAL_ERROR call so as to not be live across it. So what it would actually need to do is to say that it's ok for aNoGC to be live across any GC call post-dominated by an error return.

Comment 1

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

Note that the error reporting path can GC in other ways than console logging. Anything that creates a JS exception, for example, can GC when allocating the exception.

Comment 2

[Jon Coppeard (:jonco)]

One approach would be to refactor to move the error creating parts outside the function that handles pointers that may be changed by GC. But I can see why you wouldn't want to do that if this is a common occurrence.

I take it this is for the analysis? It sounds like what you really want is for the analysis to track the pointer without relying on the guard object since I think it would be fine with code like this.

Comment 3

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

Oh, sorry, I guess I didn't provide much context. Yes, this is for the hazard analysis. The situation I see this occurring is in the ProcessData API. A user looks like:

  ProcessData(myUint8Array, [](mozilla::Span& aData, JS::AutoCheckCannotGC&& aNoGC) {
    ...
  }

In order to refactor this to move out error-creating parts, it needs to return enough information to distinguish the different errors you might encounter, and carry any data you want in the exception.

Examples:

• CanvasRenderingContext2D.cpp
• ClientWebGLContext.cpp and again and again
• ImageData.cpp is a perfect example of where this could be used, because it turns out to be a bad version of the refactoring you're proposing: it grabs the length in an nogc scope, then exfiltrates it without the nogc. It is fortunately just sidestepping false alarms in the error branches, since there's nothing there that could change the length (by detaching or resizing a ResizableArrayBuffer). With resetOnError(), the entire body could be moved into the lambda and then it would checked by the analysis without emitting any false alarms. I filed bug 1907981.

My actual example is in proposed IndexedDB code that we're trying to fix the hazards in. From scanning the tree, it seems like there would be few places where this would useful. (It'd be really useful when it is useful, at least?)

With respect to tracking the pointer without a guard object, the pointer is frequently just a uint8_t* that needs to get passed as a uint8_t* or char* or void* to various APIs, which means that even if you have a wrapper type that the analysis is aware of, you'll be unwrapping it constantly and it's very easy to accidentally separate the pointer from its guard. As for tracking just the uint8_t*, that's only possible in the local scope where the interior pointer is extracted in the first place. I guess I could taint all callees that take that pointer, or that take a tainted value (eg mozilla::Span constructed from a tainted pointer). But taint analysis is a big thing, one that people have tried and repeatedly failed to get workable in production.