Platform/GFX/design patterns

This page contains notes about design pattern discussions around certain recurrent problems. The proposed patterns are not unanimously approved solutions, these are just notes about the discution (not a conclusion).

Recurrent problems

1) Crash when dereferencing null pointers: we tend to have a lot of them, we want to have less of them 2) When investigating a crash of type of problem 1), it is often not obvious whether the pointer is supposed to never be null (in which case the fix should go in the code that made the assignment) or if the crash site should have handled the null pointer case. 3) Objects that have more states that they should have: The example is TextureClient which is created by a factory and then has its texture data allocated, meaning both the results of creating and allocating the TextureClient much be checked, which may be easy to forget, and adds to the list of things to think about when investigating crashes. 4) Other classic bugs, such as buffer overflows due to passing around c-style arrays without passing its length along with it.

Nical: Issues (1) and (2) can be generalized to any sort of invalid state (not just null pointers), but they happen so often at this level, that it is worth having a specific solution to them. If we come up with a solution for the more general case that efficiently covers the null pointer cases it is even better!

Note that dereferencing a null pointer (1) is very often not the bug in itself, but the manifestation of other issues. Making it hard to run into null pointer dereferences may as well not reduce the amount of bugs, but it would be a good improvement if those bugs manifest themselves in ways that more explicitly show the issues (for instance "crash when assigning a null pointer to Foo" is in many cases easier to investigate than "crash dereferencing Foo" since the stack trace is closer to where the real problem happens).

Why do we have these problems so often

• Human mistakes: A lot of code deals with a lot of states. It is easy to miss checking for an invalid state, and forgetting for just a small fraction of the time can result in a lot of different bugs.
• Code evolution: When modifying code that doesn't check for certain states, it is tempting to not check in the new code either, because since it is not apparent in the existing code that a certain state is invalid, making the decision that it is in the added code could break existing code (this is analogous to problem (2): since the contract is not explicit in the code, fixing or modifying it in a way that respects the contract requires extra work.
• Well designed/written code sometimes progressively evolve into stateful mess. It is not always one commit that is at fault but the interactions of a lot of them.
• Constructors can't return a specific value for failure, so it is tempting to separate construction and initialization if initialization can fail.

Acceptance criteria

It is hard to quantify expected results from this discussion, really. The idea is to make some classes of bugs less likely to happen and to make fixing them easier.

Some proposed solutions

NotNull<T> template type

It would be a thin wrapper around any pointer type (int*, RefPtr<DrawTarget>, etc) that can doesn't have a default constructor, and assert during assignment that thet pointer is not null.

   template<typename T>
   struct NotNull
   {
       T payload;
   
       NotNull(T val) : payload(val) { NS_ABORT_IF_FALSE(!!payload); }
   
       NotNull(const NotNull& rhs) : payload(rhs.payload) {}
   
       // ... and the glue to make it behave as a regular pointer type
   };
   
   // can be used for function/method interfaces...
   NotNull<WebThing*> MassageWebThing(NotNull<WebThing*> aThing) {
     // no need to null-check aThing, it is not null by construction
     // ... do things with it...
     if (foo) {
         return aThing;
     } else {
         WebgThing* unsafeThing = CreateWebThing();
         if (!unsafeThing) {
             // deal with it because we are going to return it as a NotNull
             // so if we leave it as it is it will assert;
         }
         return unsafeThing;
     }
   }
   
   // can be used with structs fields which are never null 
   struct FooContext {
     NotNull<RefPtr<FooData> > mData; // must be set in the constructor
   
     FooContext()
     : mData(CreateDefaultFooData())
     {}
   };

Pros

• Documents very well the programmer intent (helps with problem (2)), crash happens during assignment rather than later when dereferenced (better stack traces).
• Low overhead (the pointer is checked only once, rather than everywhere it is used)

Cons

• Applies to only a subset of the problems. While this can help in some places, it won't solve all of our null pointer issues.

C++ references

C++ already has the concept of non-null pointers in the form of C++ references

Pros

• It's already there

Cons

• We tend to deal with reference counted pointers more often than raw pointers in gecko, and c++ references can't express reference counting.

AssertInvariants pattern

Classes that need to have complex states ("this can be null if that is equal to foobar, but not otherwise") could define a method (say, bool ChechInvariants() const) and would assert that it returns true in each method that modify the state of the object. If this becomes a convention, it can provide developers with a good place to look for useful information when debugging. The boilerplate of asserting the invariants can even be put in a RAII helper behind a maccro, see example below:

   #ifdef DEBUG
   #define T_ASSERT_INVARIANTS(klass) AutoAssertInvariants<klass> autoCheck(this);
   template<typename T>        \
   struct AutoAssertInvariants \
   {                           \
     T mThing;                 \
     AutoAssertInvariants(const T* aThing)   \
     : mThing(aThing)                        \
     {                                       \
       MOZ_ASSERT(mThing->CheckInvariants());\
     }                                       \
     ~AutoAssertInvariants()                 \
     {                                       \
       MOZ_ASSERT(mThing->CheckInvariants());\
     }                                       \
   };
   #else
   #define T_ASSERT_INVARIANTS(klass)  
   #endif
   
   // in GfxComplexThing.cpp
   #define ASSERT_INVARIANTS T_ASSERT_INVARIANTS(GfxComplexThing)
   
   GfxComplexThing::CheckInvariants()
   {
     // express *in code* the invariants of this object here
   }
   
   GfxComplexThing::DoThis()
   {
     ASSERT_INVARIANTS
     // do this...
   }
   
   GfxComplexThing::DoThat()
   {
     ASSERT_INVARIANTS
     // do that... 
   }
   
   #undef ASSERT_INVARIANTS

Examples of classes that could benefit from this:

• gfxContext
• ContentClient/RotatedBuffer (if anywone can actually understand what the contracts are today)
• DrawTarget implementations

Pros

• Checking at the beginning and end of each method makes it likely that the assertion is fired in a useful stack.
• Even without assering, a CheckInvariants() method is a good convention because it describes in the code the contracts, so it solves problem (2)
• Not limited to null pointer issues.

Cons

• Some might find this overkill and decide not to use it.

Creating objects in a valid state

If initialization can fail, one can wrap the creation and initialization of objects in a static method that return either the object in a valid state or null. The constructor could be made protected to ensure that we always use the factory method.

   TemporaryRef<Foo> Foo::Create(IntSize aSize, BackendType aBackend)
   {
     RefPtr<Foo> result;
     switch (aBackend) {
       case BackendType::SKIA:
         result = new FooSkia();
         break;
       // ...
     }
     if (!result->init(aSize)) {
       return nullptr;
     }
     return result;
   }

TextureClient and TextureSource are good examples of classes that should have used this pattern.