Don't process CompositorFrames until their SharedBitmaps are ready

CompositorFrames and SharedBitmaps' memory handles come from two different
mojo pipes and currently there is no guarantee that for all SharedBitmaps 
referenced by a CompositorFrame are available when it arrives. In this CL
ClientSharedBitmapManager assigns a sequence number to each SharedBitmap
it allocates. These sequence numbers are sent to the browser process along
with the CompositorFrame that references those SharedBitmaps. When a
CompositorFrame arrives, RenderWidgetHostImpl asks from
SharedBitmapAllocationNotifierImpl the latest sequence number from that
renderer known to the browser and if the CompositorFrame references higher
sequence numbers it won't be processed until
SharedBitmapAllocationNotifierImpl notifies RenderWidgetHostImpl through the
SharedBitmapAllocationObserver interface that the necessary SharedBitmaps are
ready.
A few notes:
- This CL is meant to be a quick fix for crbug.com/734058. Long term we
would like this logic to be in viz and not content, but that's
substantially more work.
- This CL only fixes the issue for RenderWidgets, which got broken after
crrev.com/463845. Everything else including OffscreenCanvas has always
been broken and remains broken.
- SharedBitmapAllocationNotifier will no longer be associated with
RenderMessageFilter. It used to be associated to preserve ordering with
respect to CompositorFrames, but after crrev.com/463845 CompositorFrames
no longer share a pipe with RenderMessageFilter so there is no point.
As a result SharedBitmapAllocationNotifierImpl now has its own separate
pipe and it lives on the UI thread which is nicer because that's where
the observers (RenderWidgetHostImpls) live and we can avoid doing PostTasks.
The separate pipe also prepares us for an out of process display compositor
because RenderMessageFilter will always be a browser<->renderer pipe and 
being associated with it doesn't make much sense.
- With SharedBitmapAllocationNotifierImpl living on UI thread, I thought
we should be able to get rid of the mutex in ServerSharedBitmapManager,
but it seems like OnMemoryDump is still being called from another thread.
We should investigate whether something can be done about OnMemoryDump
so we can get rid of the lock because it feels very unnecessary now.

BUG=734058

Change-Id: I3ba32f1e1cb4fce1589445b2f17e9f9855aa0c40
Reviewed-on: https://chromium-review.googlesource.com/565645
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486875}

cc/ipc/cc_param_traits_macros.h

@@ -144,6 +144,7 @@ IPC_STRUCT_TRAITS_BEGIN(cc::TransferableResource)
   IPC_STRUCT_TRAITS_MEMBER(mailbox_holder)
   IPC_STRUCT_TRAITS_MEMBER(read_lock_fences_enabled)
   IPC_STRUCT_TRAITS_MEMBER(is_software)
+  IPC_STRUCT_TRAITS_MEMBER(shared_bitmap_sequence_number)
   IPC_STRUCT_TRAITS_MEMBER(is_overlay_candidate)
   IPC_STRUCT_TRAITS_MEMBER(color_space)
 #if defined(OS_ANDROID)

cc/ipc/shared_bitmap_allocation_notifier.mojom

@@ -8,10 +8,6 @@ import "gpu/ipc/common/mailbox.mojom";
 
 // This interface is used when allocating shared bitmaps to be shared with a
 // display compositor.
-// This interface needs to be associated with the channel used to submit
-// CompositorFrames, to prevent running into message ordering issues (the
-// display compositor trying to access shared memory before the registration
-// message below made it to the display compositor).
 interface SharedBitmapAllocationNotifier {
   // Informs the display compositor that the child allocated a shared bitmap.
   DidAllocateSharedBitmap(handle<shared_buffer> buffer, gpu.mojom.Mailbox id);

cc/ipc/struct_traits_unittest.cc

@@ -1135,6 +1135,7 @@ TEST_F(StructTraitsTest, TransferableResource) {
   const uint32_t texture_target = 1337;
   const bool read_lock_fences_enabled = true;
   const bool is_software = false;
+  const uint32_t shared_bitmap_sequence_number = 123456;
   const bool is_overlay_candidate = true;
 
   gpu::MailboxHolder mailbox_holder;
@@ -1151,6 +1152,7 @@ TEST_F(StructTraitsTest, TransferableResource) {
   input.mailbox_holder = mailbox_holder;
   input.read_lock_fences_enabled = read_lock_fences_enabled;
   input.is_software = is_software;
+  input.shared_bitmap_sequence_number = shared_bitmap_sequence_number;
   input.is_overlay_candidate = is_overlay_candidate;
   mojom::TraitsTestServicePtr proxy = GetTraitsTestProxy();
   TransferableResource output;
@@ -1165,6 +1167,8 @@ TEST_F(StructTraitsTest, TransferableResource) {
             output.mailbox_holder.texture_target);
   EXPECT_EQ(read_lock_fences_enabled, output.read_lock_fences_enabled);
   EXPECT_EQ(is_software, output.is_software);
+  EXPECT_EQ(shared_bitmap_sequence_number,
+            output.shared_bitmap_sequence_number);
   EXPECT_EQ(is_overlay_candidate, output.is_overlay_candidate);
 }
 

cc/ipc/transferable_resource.mojom

@@ -23,12 +23,13 @@ enum ResourceFormat {
 struct TransferableResource {
   uint32 id;
   ResourceFormat format;
-  gfx.mojom.BufferFormat buffer_format;  
+  gfx.mojom.BufferFormat buffer_format;
   uint32 filter;
   gfx.mojom.Size size;
   gpu.mojom.MailboxHolder mailbox_holder;
   bool read_lock_fences_enabled;
   bool is_software;
+  uint32 shared_bitmap_sequence_number;
   bool is_overlay_candidate;
   bool is_backed_by_surface_texture;
   bool wants_promotion_hint;

cc/ipc/transferable_resource_struct_traits.cc

@@ -25,6 +25,7 @@ bool StructTraits<cc::mojom::TransferableResourceDataView,
   out->filter = data.filter();
   out->read_lock_fences_enabled = data.read_lock_fences_enabled();
   out->is_software = data.is_software();
+  out->shared_bitmap_sequence_number = data.shared_bitmap_sequence_number();
   out->is_overlay_candidate = data.is_overlay_candidate();
 #if defined(OS_ANDROID)
   out->is_backed_by_surface_texture = data.is_backed_by_surface_texture();

cc/ipc/transferable_resource_struct_traits.h

@@ -50,6 +50,11 @@ struct StructTraits<cc::mojom::TransferableResourceDataView,
     return resource.is_software;
   }
 
+  static uint32_t shared_bitmap_sequence_number(
+      const cc::TransferableResource& resource) {
+    return resource.shared_bitmap_sequence_number;
+  }
+
   static bool is_overlay_candidate(const cc::TransferableResource& resource) {
     return resource.is_overlay_candidate;
   }

cc/resources/resource_provider.cc

@@ -1729,6 +1729,12 @@ void ResourceProvider::TransferResource(Resource* source,
   if (source->type == RESOURCE_TYPE_BITMAP) {
     resource->mailbox_holder.mailbox = source->shared_bitmap_id;
     resource->is_software = true;
+    if (source->shared_bitmap) {
+      resource->shared_bitmap_sequence_number =
+          source->shared_bitmap->sequence_number();
+    } else {
+      resource->shared_bitmap_sequence_number = 0;
+    }
   } else {
     DCHECK(source->mailbox().IsValid());
     DCHECK(source->mailbox().IsTexture());

cc/resources/transferable_resource.cc

@@ -14,6 +14,7 @@ TransferableResource::TransferableResource()
       filter(0),
       read_lock_fences_enabled(false),
       is_software(false),
+      shared_bitmap_sequence_number(0),
 #if defined(OS_ANDROID)
       is_backed_by_surface_texture(false),
       wants_promotion_hint(false),

cc/resources/transferable_resource.h

@@ -38,6 +38,7 @@ struct CC_EXPORT TransferableResource {
   gpu::MailboxHolder mailbox_holder;
   bool read_lock_fences_enabled;
   bool is_software;
+  uint32_t shared_bitmap_sequence_number;
 #if defined(OS_ANDROID)
   bool is_backed_by_surface_texture;
   bool wants_promotion_hint;

cc/test/test_shared_bitmap_manager.cc

@@ -17,7 +17,9 @@ class OwnedSharedBitmap : public viz::SharedBitmap {
  public:
   OwnedSharedBitmap(std::unique_ptr<base::SharedMemory> shared_memory,
                     const viz::SharedBitmapId& id)
-      : viz::SharedBitmap(static_cast<uint8_t*>(shared_memory->memory()), id),
+      : viz::SharedBitmap(static_cast<uint8_t*>(shared_memory->memory()),
+                          id,
+                          0 /* sequence_number */),
         shared_memory_(std::move(shared_memory)) {}
 
   ~OwnedSharedBitmap() override {}
@@ -34,7 +36,7 @@ class OwnedSharedBitmap : public viz::SharedBitmap {
 class UnownedSharedBitmap : public viz::SharedBitmap {
  public:
   UnownedSharedBitmap(uint8_t* pixels, const viz::SharedBitmapId& id)
-      : viz::SharedBitmap(pixels, id) {}
+      : viz::SharedBitmap(pixels, id, 0 /* sequence_number */) {}
 
   // viz::SharedBitmap:
   base::SharedMemoryHandle GetSharedMemoryHandle() const override {

components/viz/client/client_shared_bitmap_manager.cc

@@ -24,24 +24,27 @@ namespace {
 class ClientSharedBitmap : public SharedBitmap {
  public:
   ClientSharedBitmap(
-      scoped_refptr<
-          cc::mojom::ThreadSafeSharedBitmapAllocationNotifierAssociatedPtr>
+      scoped_refptr<cc::mojom::ThreadSafeSharedBitmapAllocationNotifierPtr>
           shared_bitmap_allocation_notifier,
       base::SharedMemory* shared_memory,
-      const SharedBitmapId& id)
-      : SharedBitmap(static_cast<uint8_t*>(shared_memory->memory()), id),
+      const SharedBitmapId& id,
+      uint32_t sequence_number)
+      : SharedBitmap(static_cast<uint8_t*>(shared_memory->memory()),
+                     id,
+                     sequence_number),
         shared_bitmap_allocation_notifier_(
             std::move(shared_bitmap_allocation_notifier)) {}
 
   ClientSharedBitmap(
-      scoped_refptr<
-          cc::mojom::ThreadSafeSharedBitmapAllocationNotifierAssociatedPtr>
+      scoped_refptr<cc::mojom::ThreadSafeSharedBitmapAllocationNotifierPtr>
           shared_bitmap_allocation_notifier,
       std::unique_ptr<base::SharedMemory> shared_memory_holder,
-      const SharedBitmapId& id)
+      const SharedBitmapId& id,
+      uint32_t sequence_number)
       : ClientSharedBitmap(std::move(shared_bitmap_allocation_notifier),
                            shared_memory_holder.get(),
-                           id) {
+                           id,
+                           sequence_number) {
     shared_memory_holder_ = std::move(shared_memory_holder);
   }
 
@@ -57,8 +60,7 @@ class ClientSharedBitmap : public SharedBitmap {
   }
 
  private:
-  scoped_refptr<
-      cc::mojom::ThreadSafeSharedBitmapAllocationNotifierAssociatedPtr>
+  scoped_refptr<cc::mojom::ThreadSafeSharedBitmapAllocationNotifierPtr>
       shared_bitmap_allocation_notifier_;
   std::unique_ptr<base::SharedMemory> shared_memory_holder_;
 };
@@ -110,8 +112,7 @@ std::unique_ptr<base::SharedMemory> AllocateSharedMemory(size_t buf_size) {
 }  // namespace
 
 ClientSharedBitmapManager::ClientSharedBitmapManager(
-    scoped_refptr<
-        cc::mojom::ThreadSafeSharedBitmapAllocationNotifierAssociatedPtr>
+    scoped_refptr<cc::mojom::ThreadSafeSharedBitmapAllocationNotifierPtr>
         shared_bitmap_allocation_notifier)
     : shared_bitmap_allocation_notifier_(
           std::move(shared_bitmap_allocation_notifier)) {}
@@ -131,14 +132,15 @@ std::unique_ptr<SharedBitmap> ClientSharedBitmapManager::AllocateSharedBitmap(
   if (!memory || !memory->Map(memory_size))
     CollectMemoryUsageAndDie(size, memory_size);
 
-  NotifyAllocatedSharedBitmap(memory.get(), id);
+  uint32_t sequence_number = NotifyAllocatedSharedBitmap(memory.get(), id);
 
   // Close the associated FD to save resources, the previously mapped memory
   // remains available.
   memory->Close();
 
   return base::MakeUnique<ClientSharedBitmap>(
-      shared_bitmap_allocation_notifier_, std::move(memory), id);
+      shared_bitmap_allocation_notifier_, std::move(memory), id,
+      sequence_number);
 }
 
 std::unique_ptr<SharedBitmap> ClientSharedBitmapManager::GetSharedBitmapFromId(
@@ -151,28 +153,32 @@ std::unique_ptr<SharedBitmap> ClientSharedBitmapManager::GetSharedBitmapFromId(
 std::unique_ptr<SharedBitmap>
 ClientSharedBitmapManager::GetBitmapForSharedMemory(base::SharedMemory* mem) {
   SharedBitmapId id = SharedBitmap::GenerateId();
-  NotifyAllocatedSharedBitmap(mem, SharedBitmap::GenerateId());
+  uint32_t sequence_number = NotifyAllocatedSharedBitmap(mem, id);
   return base::MakeUnique<ClientSharedBitmap>(
-      shared_bitmap_allocation_notifier_, mem, id);
+      shared_bitmap_allocation_notifier_, mem, id, sequence_number);
 }
 
 // Notifies the browser process that a shared bitmap with the given ID was
 // allocated. Caller keeps ownership of |memory|.
-void ClientSharedBitmapManager::NotifyAllocatedSharedBitmap(
+uint32_t ClientSharedBitmapManager::NotifyAllocatedSharedBitmap(
     base::SharedMemory* memory,
     const SharedBitmapId& id) {
   base::SharedMemoryHandle handle_to_send =
       base::SharedMemory::DuplicateHandle(memory->handle());
   if (!base::SharedMemory::IsHandleValid(handle_to_send)) {
     LOG(ERROR) << "Failed to duplicate shared memory handle for bitmap.";
-    return;
+    return 0;
   }
 
   mojo::ScopedSharedBufferHandle buffer_handle = mojo::WrapSharedMemoryHandle(
       handle_to_send, memory->mapped_size(), true /* read_only */);
 
-  (*shared_bitmap_allocation_notifier_)
-      ->DidAllocateSharedBitmap(std::move(buffer_handle), id);
+  {
+    base::AutoLock lock(lock_);
+    (*shared_bitmap_allocation_notifier_)
+        ->DidAllocateSharedBitmap(std::move(buffer_handle), id);
+    return ++last_sequence_number_;
+  }
 }
 
 }  // namespace viz

components/viz/client/client_shared_bitmap_manager.h

@@ -12,6 +12,7 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/shared_memory.h"
+#include "base/synchronization/lock.h"
 #include "cc/ipc/shared_bitmap_allocation_notifier.mojom.h"
 #include "components/viz/common/resources/shared_bitmap_manager.h"
 #include "mojo/public/cpp/bindings/thread_safe_interface_ptr.h"
@@ -24,8 +25,7 @@ namespace viz {
 class ClientSharedBitmapManager : public SharedBitmapManager {
  public:
   explicit ClientSharedBitmapManager(
-      scoped_refptr<
-          cc::mojom::ThreadSafeSharedBitmapAllocationNotifierAssociatedPtr>
+      scoped_refptr<cc::mojom::ThreadSafeSharedBitmapAllocationNotifierPtr>
           shared_bitmap_allocation_notifier);
   ~ClientSharedBitmapManager() override;
 
@@ -40,13 +40,18 @@ class ClientSharedBitmapManager : public SharedBitmapManager {
       base::SharedMemory* mem);
 
  private:
-  void NotifyAllocatedSharedBitmap(base::SharedMemory* memory,
-                                   const SharedBitmapId& id);
+  uint32_t NotifyAllocatedSharedBitmap(base::SharedMemory* memory,
+                                       const SharedBitmapId& id);
 
-  scoped_refptr<
-      cc::mojom::ThreadSafeSharedBitmapAllocationNotifierAssociatedPtr>
+  scoped_refptr<cc::mojom::ThreadSafeSharedBitmapAllocationNotifierPtr>
       shared_bitmap_allocation_notifier_;
 
+  base::Lock lock_;
+
+  // Each SharedBitmap allocated by this class is assigned a unique sequence
+  // number that is incremental.
+  uint32_t last_sequence_number_ = 0;
+
   DISALLOW_COPY_AND_ASSIGN(ClientSharedBitmapManager);
 };
 

components/viz/common/quads/shared_bitmap.cc

@@ -16,8 +16,10 @@
 
 namespace viz {
 
-SharedBitmap::SharedBitmap(uint8_t* pixels, const SharedBitmapId& id)
-    : pixels_(pixels), id_(id) {}
+SharedBitmap::SharedBitmap(uint8_t* pixels,
+                           const SharedBitmapId& id,
+                           uint32_t sequence_number)
+    : pixels_(pixels), id_(id), sequence_number_(sequence_number) {}
 
 SharedBitmap::~SharedBitmap() {}
 

components/viz/common/quads/shared_bitmap.h

@@ -32,14 +32,20 @@ base::trace_event::MemoryAllocatorDumpGuid GetSharedBitmapGUIDForTracing(
 
 class SharedBitmap {
  public:
-  SharedBitmap(uint8_t* pixels, const SharedBitmapId& id);
+  SharedBitmap(uint8_t* pixels,
+               const SharedBitmapId& id,
+               uint32_t sequence_number);
 
   virtual ~SharedBitmap();
 
   uint8_t* pixels() { return pixels_; }
 
   const SharedBitmapId& id() { return id_; }
 
+  // The sequence number that ClientSharedBitmapManager assigned to this
+  // SharedBitmap.
+  uint32_t sequence_number() const { return sequence_number_; }
+
   // Returns the shared memory's handle when the back end is base::SharedMemory.
   // Otherwise, this returns an invalid handle.
   virtual base::SharedMemoryHandle GetSharedMemoryHandle() const = 0;
@@ -60,6 +66,7 @@ class SharedBitmap {
  private:
   uint8_t* pixels_;
   SharedBitmapId id_;
+  const uint32_t sequence_number_;
 
   DISALLOW_COPY_AND_ASSIGN(SharedBitmap);
 };

components/viz/service/display_embedder/server_shared_bitmap_manager.cc

@@ -40,7 +40,7 @@ class ServerSharedBitmap : public SharedBitmap {
                      scoped_refptr<BitmapData> bitmap_data,
                      const SharedBitmapId& id,
                      ServerSharedBitmapManager* manager)
-      : SharedBitmap(pixels, id),
+      : SharedBitmap(pixels, id, 0 /* sequence_number */),
         bitmap_data_(bitmap_data),
         manager_(manager) {}