870193 - gfx/layers/ipc code needs to pick one way to polymorphism and stick to it (currently virtual + #ifdef + template specialization)

Status: NEW

(Core :: Graphics, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

This is the "fix the right way" for bug 868556.

The consequences of this bug were dire: no gralloc anymore.

The root cause of this bug was that ISurfaceAllocator is using two unrelated means to achieve polymorphism: "virtual", and "#ifdef".

That turned out to be evil because of the following pattern:

Base.h:

  class Base {
    virtual GrallocBuffer* AllocGrallocBuffer() { return nullptr; }
  };

Derived.h:

  class Derived : Base {
  #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
    virtual GrallocBuffer* AllocGrallocBuffer() { return SomethingUseful(); }
  #endif
  };

Victim.h:

  #include "Derived.h"
  #define MOZ_HAVE_SURFACEDESCRIPTORGRALLOC

Victim.cpp:

  #include "Derived.h"

  Derived d;

  // we really need a GrallocBuffer here!
  GrallocBuffer *b = d.AllocGrallocBuffer();
  // BOOM! We got nullptr instead of an actual GrallocBuffer!

The problem is that Derived.h defines a class whose actual behavior depends on whether MOZ_HAVE_SURFACEDESCRIPTORGRALLOC is defined. As Victim.h happens to #include Derived.h before it defines the magic token, it gets a non-functional Derived class from Derived.h.

This happened in Bug 868556 with:
  ISurfaceAllocator in the role of Base
  ShadowLayerForwarder in the role of Derived
  ShadowLayerUtilsGralloc in the role of Victim

We should really pick one way to polymorphism and stick to it; the default choice is to pick 'virtual' and remove the #ifdefs.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

In addition to MOZ_HAVE_SURFACEDESCRIPTORGRALLOC there is also a MOZ_HAVE_PLATFORM_SPECIFIC_LAYER_BUFFERS that seems to control the overriding of several more virtual functions. Must remove it too.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Doesn't it get even worse?
Victim2.h:
  #define MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
  #include "Derived.h"

Victim2.cpp:
  #include "Derived.h"

What happens if Victim2.cpp allocates Derived which eventually ends up in a method in Victim.cpp, or vice versa?

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

That's really the question of what happens at link time when multiple definitions of an inline function are not identical... I don't know, trying.

(Side note: I made a typo, in Victim.cpp I meant to #include Victim.h, not #include Derived.h. I assume the same change in comment 2 --- sorry --- so Victim2.cpp #includes Victim2.h, not Derived.h.)

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Tried on a small example:

a.cpp:

  inline const char* foo() { return "A"; }
  void x() { foo(); }

b.cpp:

  inline const char* foo() { return "B"; }
  void x() { foo(); }

c.cpp:

  const char* foo();
  #include <iostream>
  using namespace std;
  int main() {
    cout << foo() << endl;
  }


bjacob:~$ g++ a.o b.o c.o -o x
bjacob:~$ ./x
A

bjacob:~$ g++ b.o a.o c.o -o x
bjacob:~$ ./x
B

So the answer, at least with GCC 4.6.2, is that the first definition gets picked. So yes, that's really evil.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

In comment 4, b.cpp should have "y()" instead of "x()".

Comment 6

[(no longer active)]

Calling this #ifdef anti-idiom polymorphism is quite generous! ;-)

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

This is actually spread all over gfx/layers/ipc and also uses template specializations in #ifdef blocks.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: WIP part 1: remove all the ifdefs

This removes all the #ifdefs; works on B2G, untested (probably doesn't build) elsewhere.

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Here is an overview of what needs to be done around here to get us out of this mess.

The method that blew up in bug 868556 is AllocGrallocBuffer. It is called on the child (==content) side to get a gralloc buffer. Its effect is to call PLayerTransactionParent::SendPGrallocBufferConstructor. The effect of that is to send an IPC message to the parent (==compositor) side, which receives a call to AllocPGrallocBuffer (note the P).

The details of what happens next are worked out in Vlad's wiki page, https://wiki.mozilla.org/User:VladVukicevic/Gralloc

What matters for the present discussion is that there are two methods here, AllocGrallocBuffer on the child side and AllocPGrallocBuffer on the parent side, which are subject to heavy #ifdef'ing, and the goal of the present bug is to remove this #ifdef'ing.

One would think that the #ifdef'ing of the gralloc paths serves at least the purpose of avoiding building gralloc-specific code on platforms that don't need it; one would be wrong. Some gralloc-specific code is indeed only built on B2G: that is mostly ShadowLayerUtilsGralloc.cpp. But the rest, including all the gralloc-specific IPC stuff (PGrallocBuffer), is built on all platforms.

So a high-level plan for action could look like this:

Part A. Removing all these #ifdefs to solve the original problem here and make the rest tractable.

1. Remove the #ifdef's on child-side code (AllocGrallocBuffer). Just build it everywhere.
2. Remove the #ifdef's on parent-side code (AllocPGrallocBuffer). Just build it everywhere.
3. Remove the #ifdef's on remaining stuff (the ParamTraits specializations).

Part B. Start thinking about the other questions:

1. How we can avoid compiling gralloc-specific code on all platforms?
2. Why do we have dozens of functions around whose only job is to forward a call to SendPGrallocBuffer (in the case of AllocGrallocBuffer) or to GrallocBufferActor::Create (in the case of AllocPGrallocBuffer) ?


Here is a very simplified inheritance tree of the involved classes (stripped of multiple inheritance; all the IPDL-generated base classes have been omitted).

For each class, we write "CHILD #IFDEF" if the class implements AllocGrallocBuffer with #ifdefs, and we write "PARENT #IFDEF" if the class implements AllocPGrallocBuffer with #ifdefs.

                           ISurfaceAllocator
                           /              \
                          /                \
    CompositableParentManager               \
          /                \                 \
         /                  \                 \
LayerTransactionParent   ImageBridgeParent    CompositableForwarder
    PARENT #IFDEF          PARENT #IFDEF      /            \
                                             /              \
                                        ImageBridgeChild   ShadowLayerForwarder
                                          CHILD #IFDEF       CHILD #IFDEF
                                         PARENT #IFDEF            |
                                                                  |
                                                           ClientLayerManager

On this diagram we see that Part A.1. is going to be work at least on ImageBridgeChild and ShadowLayerForwarder, while Part A.2. is going to be work at least on LayerTransactionParent, ImageBridgeParent, and ImageBridgeChild.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

I am getting back to looking into this now.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.