It is too easy to forget to report failures with SystemAllocPolicy

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
9 years ago
7 years ago

People

(Reporter: luke, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

9 years ago
To know whether you need to report errors coming out of a js::Vector or js::Hash*, you need to know the allocation policy.  It's usually ContextAllocPolicy, so the answer is usually 'you don't need to'.  But in the rarer case of SystemAllocPolicy, its very easy to forget.  Igor and I have both recently made this error and I bet if I looked hard, I could find a few more.  Brendan was pointing this out on IRC earlier as well.

A potential solution, that I am asking everyone here to evaluate in terms of the tradeoff between safety and annoyance is as follows:

The idea is to make it so that it is locally evident that a call does not report errors.  That is, it might be useful to have the type system require that you to write:

  if (!v.append('x', ERROR_NOT_REPORTED)) {
    js_ReportOutOfMemory(cx)
    return false;
  }

iff you use the SystemAllocPolicy.

The basic strategy is to have two "reminder types":

struct HasDefaultConstructor {};
enum ReminderConstant { ERROR_NOT_REPORTED };
struct NoDefaultConstructor { NoDefaultConstructor(ReminderConstant) {} };

and then have each allocation policy choose the appropriate reminder type:

struct ContextAllocPolicy {
  typedef HasDefaultConstructor ReminderType;
  ...
};

struct SystemAllocPolicy {
  typdef NoDefaultConstructor ReminderType;
  ...
};

and, finally, have each Vector/HashMap operation take an AllocationPolicy::ReminderType argument that is default constructed:

  bool append(const T &t, ReminderType = ReminderType())

Thus, only callers of SystemAllocPolicy-based containers would be required to pass ERROR_NOT_REPORTED.

Thoughts?  Alternate solutions?

Comment 1

9 years ago
(In reply to comment #0)
> Igor and I have both recently made this error

For my part see the bug 563326 comment 41.
(In reply to comment #0)
> To know whether you need to report errors coming out of a js::Vector or
> js::Hash*, you need to know the allocation policy.  It's usually
> ContextAllocPolicy, so the answer is usually 'you don't need to'.  But in the
> rarer case of SystemAllocPolicy, its very easy to forget.  Igor and I have both
> recently made this error and I bet if I looked hard, I could find a few more. 
> Brendan was pointing this out on IRC earlier as well.
> 
> A potential solution, that I am asking everyone here to evaluate in terms of
> the tradeoff between safety and annoyance is as follows:
[...]
> Thoughts?  Alternate solutions?

That does look kind of annoying, but if it will save us a few bugs, then it seems worth the annoyance.

We could also do this with some kind of static analysis that requires that the return values be consumed. But that is more effort to set up and maintain.
(Reporter)

Comment 3

7 years ago
I just had an idea!  Its a bit insane, but its the only solution to this problem I see that isn't terrible.  In particular, this lets us *always* have the container report errors, even when the container outlives any given context.

The idea is to make a one-off "long-lived container" for each container type that has special friend-privileges with the container:

  template <class T, size_t N class AllocPolicy>
  struct LongLivedVector : private Vector<T,N,AllocPolicy> {
      typedef Vector<T,N,AllocPolicy> Base;

      // operations that can't fail are hoisted directly:
      using Base::operator[];
      using Base::length;
      ...

      // operations that can fail must be passed a 'cx' like so:
      Base &operator()(JSContext *cx) {
          AllocPolicy::cx = cx;  // of course this would be generalized
          return static_cast<Base &>(*this);
      }   
  };

Now, let's say you have one of these long-lived containers:

  LongLivedVector<int, 0, ContextAllocPolicy> v;

First, the constructor won't make you pass a 'cx' to the constructor.  Second, when you want to use it, the type system enforces that you bind 'cx':

  ... = v.length();        // fine
  if (!v(cx).append(...))  // need to bind 'cx' to call 'append' on Base
     ...

operator() is weird, I know, but I love the relative brevity and the lack of gunk-ing up Vector by trying to have this whole alternative maybe-parameter on each fallible member.

Alternatives to LongLived... Elder?  Enduring?  Lasting?  Old?

Comment 4

7 years ago
If the problem is OOM reporting, why not to fix this by changing our API to report OOM using JSRuntime hook? This would not impair the quality of error reporting as we can use the runtime to find the current JSThread and the stack frame where the error happens.
(Reporter)

Comment 5

7 years ago
Long-term I think something even better will happen: we can use TLS to map to the unified-JSContext/JSThreadData and report errors that way.  This seemed like a relatively simple near-term solution.
(Reporter)

Comment 6

7 years ago
I was enamored with the solution but IIRC, it didn't apply in many cases.  I'll just wait for the TLS solution.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.