Bug 1125848 - Consolidate PCompositor's creation-destruction logic. r=sotaro
☠☠ backed out by 01bd19b3f56b ☠ ☠
authorNicolas Silva <nsilva@mozilla.com>
Mon, 09 Mar 2015 18:43:39 +0100
changeset 261589 622f0877bc3f14f68e50fabbe7e7c3df0ff5f46c
parent 261588 132344b49ba6ddb63998e919af682fc1675c733d
child 261590 d11e756b726f67c23b68b325ab2c64856adda9b8
push id4718
push userraliiev@mozilla.com
push dateMon, 11 May 2015 18:39:53 +0000
treeherdermozilla-beta@c20c4ef55f08 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssotaro
bugs1125848
milestone39.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1125848 - Consolidate PCompositor's creation-destruction logic. r=sotaro
gfx/layers/ipc/CompositorChild.cpp
gfx/layers/ipc/CompositorChild.h
widget/nsBaseWidget.cpp
widget/nsBaseWidget.h
widget/windows/nsWindow.cpp
--- a/gfx/layers/ipc/CompositorChild.cpp
+++ b/gfx/layers/ipc/CompositorChild.cpp
@@ -35,38 +35,74 @@ namespace mozilla {
 namespace layers {
 
 /*static*/ CompositorChild* CompositorChild::sCompositor;
 
 Atomic<int32_t> CompositableForwarder::sSerialCounter(0);
 
 CompositorChild::CompositorChild(ClientLayerManager *aLayerManager)
   : mLayerManager(aLayerManager)
-  , mCanSend(true)
+  , mCanSend(false)
 {
 }
 
 CompositorChild::~CompositorChild()
 {
+  if (mCanSend) {
+    gfxCriticalError() << "CompositorChild was not deinitialized";
+  }
+}
+
+static void DeferredDestroyCompositor(nsRefPtr<CompositorParent> aCompositorParent,
+                                      nsRefPtr<CompositorChild> aCompositorChild)
+{
+    // Bug 848949 needs to be fixed before
+    // we can close the channel properly
+    //aCompositorChild->Close();
 }
 
 void
 CompositorChild::Destroy()
 {
-  mLayerManager->Destroy();
-  mLayerManager = nullptr;
+  if (!mCanSend) {
+    NS_WARNING("Trying to deinitialize a CompositorChild twice");
+    return;
+  }
+
+  SendWillStop();
+  // The call just made to SendWillStop can result in IPC from the
+  // CompositorParent to the CompositorChild (e.g. caused by the destruction
+  // of shared memory). We need to ensure this gets processed by the
+  // CompositorChild before it gets destroyed. It suffices to ensure that
+  // events already in the MessageLoop get processed before the
+  // CompositorChild is destroyed, so we add a task to the MessageLoop to
+  // handle compositor desctruction.
+
+  // From now on the only message we can send is Stop.
+  mCanSend = false;
+
+  if (mLayerManager) {
+    mLayerManager->Destroy();
+    mLayerManager = nullptr;
+  }
+
   // start from the end of the array because Destroy() can cause the
   // LayerTransactionChild to be removed from the array.
   for (int i = ManagedPLayerTransactionChild().Length() - 1; i >= 0; --i) {
     RefPtr<LayerTransactionChild> layers =
       static_cast<LayerTransactionChild*>(ManagedPLayerTransactionChild()[i]);
     layers->Destroy();
   }
-  MOZ_ASSERT(!mCanSend);
+
   SendStop();
+
+  // The DeferredDestroyCompositor task takes ownership of compositorParent and
+  // will release them when it runs.
+  MessageLoop::current()->PostTask(FROM_HERE,
+             NewRunnableFunction(DeferredDestroyCompositor, mCompositorParent, this));
 }
 
 bool
 CompositorChild::LookupCompositorFrameMetrics(const FrameMetrics::ViewID aId,
                                               FrameMetrics& aFrame)
 {
   SharedFrameMetricsData* data = mFrameMetricsTable.Get(aId);
   if (data) {
@@ -89,28 +125,42 @@ CompositorChild::Create(Transport* aTran
     NS_RUNTIMEABORT("Couldn't OpenProcessHandle() to parent process.");
     return nullptr;
   }
   if (!child->Open(aTransport, handle, XRE_GetIOMessageLoop(), ipc::ChildSide)) {
     NS_RUNTIMEABORT("Couldn't Open() Compositor channel.");
     return nullptr;
   }
 
+  child->mCanSend = true;
+
   // We release this ref in ActorDestroy().
   sCompositor = child.forget().take();
 
   int32_t width;
   int32_t height;
   sCompositor->SendGetTileSize(&width, &height);
   gfxPlatform::GetPlatform()->SetTileSize(width, height);
 
   // We release this ref in ActorDestroy().
   return sCompositor;
 }
 
+bool
+CompositorChild::OpenSameProcess(CompositorParent* aParent)
+{
+  MOZ_ASSERT(aParent);
+
+  mCompositorParent = aParent;
+  mCanSend = Open(mCompositorParent->GetIPCChannel(),
+                  CompositorParent::CompositorLoop(),
+                  ipc::ChildSide);
+  return mCanSend;
+}
+
 /*static*/ CompositorChild*
 CompositorChild::Get()
 {
   // This is only expected to be used in child processes.
   MOZ_ASSERT(XRE_GetProcessType() != GeckoProcessType_Default);
   return sCompositor;
 }
 
@@ -477,18 +527,16 @@ CompositorChild::CancelNotifyAfterRemote
     mWeakTabChild = nullptr;
   }
 }
 
 bool
 CompositorChild::SendWillStop()
 {
   MOZ_ASSERT(mCanSend);
-  // From now on the only two messages we can send are WillStop and Stop.
-  mCanSend = false;
   return PCompositorChild::SendWillStop();
 }
 
 bool
 CompositorChild::SendPause()
 {
   MOZ_ASSERT(mCanSend);
   if (!mCanSend) {
--- a/gfx/layers/ipc/CompositorChild.h
+++ b/gfx/layers/ipc/CompositorChild.h
@@ -55,16 +55,22 @@ public:
   /**
    * We're asked to create a new Compositor in response to an Opens()
    * or Bridge() request from our parent process.  The Transport is to
    * the compositor's context.
    */
   static PCompositorChild*
   Create(Transport* aTransport, ProcessId aOtherProcess);
 
+  /**
+   * Initialize the CompositorChild and open the connection in the non-multi-process
+   * case.
+   */
+  bool OpenSameProcess(CompositorParent* aParent);
+
   static CompositorChild* Get();
 
   static bool ChildProcessHasCompositor() { return sCompositor != nullptr; }
 
   void AddOverfillObserver(ClientLayerManager* aLayerManager);
 
   virtual bool
   RecvDidComposite(const uint64_t& aId, const uint64_t& aTransactionId) MOZ_OVERRIDE;
@@ -163,16 +169,19 @@ private:
     uint32_t mAPZCId;
   };
 
   static PLDHashOperator RemoveSharedMetricsForLayersId(const uint64_t& aKey,
                                                         nsAutoPtr<SharedFrameMetricsData>& aData,
                                                         void* aLayerTransactionChild);
 
   nsRefPtr<ClientLayerManager> mLayerManager;
+  // When not multi-process, hold a reference to the CompositorParent to keep it
+  // alive. This reference should be null in multi-process.
+  nsRefPtr<CompositorParent> mCompositorParent;
 
   // The ViewID of the FrameMetrics is used as the key for this hash table.
   // While this should be safe to use since the ViewID is unique
   nsClassHashtable<nsUint64HashKey, SharedFrameMetricsData> mFrameMetricsTable;
 
   // When we're in a child process, this is the process-global
   // compositor that we use to forward transactions directly to the
   // compositor context in another process.
--- a/widget/nsBaseWidget.cpp
+++ b/widget/nsBaseWidget.cpp
@@ -176,78 +176,56 @@ WidgetShutdownObserver::Observe(nsISuppo
 
 void
 nsBaseWidget::Shutdown()
 {
   DestroyCompositor();
   mShutdownObserver = nullptr;
 }
 
-static void DeferredDestroyCompositor(nsRefPtr<CompositorParent> aCompositorParent,
-                                      nsRefPtr<CompositorChild> aCompositorChild)
-{
-    // Bug 848949 needs to be fixed before
-    // we can close the channel properly
-    //aCompositorChild->Close();
-}
-
 void nsBaseWidget::DestroyCompositor()
 {
   if (mCompositorChild) {
-    nsRefPtr<CompositorChild> compositorChild = mCompositorChild.forget();
-    nsRefPtr<CompositorParent> compositorParent = mCompositorParent.forget();
-
-    compositorChild->SendWillStop();
-    // New LayerManager, CompositorParent and CompositorChild might be created
-    // as a result of internal GetLayerManager() call.
-    compositorChild->Destroy();
+    mCompositorChild->Destroy();
+    mCompositorChild = nullptr;
+    mCompositorParent = nullptr;
+  }
+}
 
-    // The call just made to SendWillStop can result in IPC from the
-    // CompositorParent to the CompositorChild (e.g. caused by the destruction
-    // of shared memory). We need to ensure this gets processed by the
-    // CompositorChild before it gets destroyed. It suffices to ensure that
-    // events already in the MessageLoop get processed before the
-    // CompositorChild is destroyed, so we add a task to the MessageLoop to
-    // handle compositor desctruction.
-
-    // The DefferedDestroyCompositor task takes ownership of compositorParent and
-    // will release them when it runs.
-    MessageLoop::current()->PostTask(FROM_HERE,
-               NewRunnableFunction(DeferredDestroyCompositor, compositorParent,
-                                   compositorChild));
+void nsBaseWidget::DestroyLayerManager()
+{
+  if (mLayerManager) {
+    mLayerManager->Destroy();
+    mLayerManager = nullptr;
   }
+  DestroyCompositor();
 }
 
 //-------------------------------------------------------------------------
 //
 // nsBaseWidget destructor
 //
 //-------------------------------------------------------------------------
 nsBaseWidget::~nsBaseWidget()
 {
   if (mLayerManager &&
       mLayerManager->GetBackendType() == LayersBackend::LAYERS_BASIC) {
     static_cast<BasicLayerManager*>(mLayerManager.get())->ClearRetainerWidget();
   }
 
-  if (mLayerManager) {
-    mLayerManager->Destroy();
-    mLayerManager = nullptr;
-  }
-
   if (mShutdownObserver) {
     // If the shutdown observer is currently processing observers,
     // then UnregisterShutdownObserver won't stop our Observer
     // function from being called. Make sure we don't try
     // to reference the dead widget.
     mShutdownObserver->mWidget = nullptr;
     nsContentUtils::UnregisterShutdownObserver(mShutdownObserver);
   }
 
-  DestroyCompositor();
+  DestroyLayerManager();
 
 #ifdef NOISY_WIDGET_LEAKS
   gNumWidgets--;
   printf("WIDGETS- = %d\n", gNumWidgets);
 #endif
 
   delete mOriginalBounds;
 
@@ -1076,35 +1054,37 @@ nsBaseWidget::GetCompositorVsyncDispatch
 void nsBaseWidget::CreateCompositor(int aWidth, int aHeight)
 {
   // This makes sure that gfxPlatforms gets initialized if it hasn't by now.
   gfxPlatform::GetPlatform();
 
   MOZ_ASSERT(gfxPlatform::UsesOffMainThreadCompositing(),
              "This function assumes OMTC");
 
-  MOZ_ASSERT(!mCompositorParent,
-    "Should have properly cleaned up the previous CompositorParent beforehand");
+  MOZ_ASSERT(!mCompositorParent && !mCompositorChild,
+    "Should have properly cleaned up the previous PCompositor pair beforehand");
+
+  if (mCompositorChild) {
+    mCompositorChild->Destroy();
+  }
 
   // Recreating this is tricky, as we may still have an old and we need
   // to make sure it's properly destroyed by calling DestroyCompositor!
 
   // If we've already received a shutdown notification, don't try
   // create a new compositor.
   if (!mShutdownObserver) {
     return;
   }
 
   CreateCompositorVsyncDispatcher();
   mCompositorParent = NewCompositorParent(aWidth, aHeight);
-  MessageChannel *parentChannel = mCompositorParent->GetIPCChannel();
   nsRefPtr<ClientLayerManager> lm = new ClientLayerManager(this);
-  MessageLoop *childMessageLoop = CompositorParent::CompositorLoop();
   mCompositorChild = new CompositorChild(lm);
-  mCompositorChild->Open(parentChannel, childMessageLoop, ipc::ChildSide);
+  mCompositorChild->OpenSameProcess(mCompositorParent);
 
   if (gfxPrefs::AsyncPanZoomEnabled() &&
       (WindowType() == eWindowType_toplevel || WindowType() == eWindowType_child)) {
     ConfigureAPZCTreeManager();
   }
 
   TextureFactoryIdentifier textureFactoryIdentifier;
   PLayerTransactionChild* shadowManager = nullptr;
@@ -1123,36 +1103,30 @@ void nsBaseWidget::CreateCompositor(int 
 #endif
 
   bool success = false;
   if (!backendHints.IsEmpty()) {
     shadowManager = mCompositorChild->SendPLayerTransactionConstructor(
       backendHints, 0, &textureFactoryIdentifier, &success);
   }
 
-  if (success) {
-    ShadowLayerForwarder* lf = lm->AsShadowForwarder();
-    if (!lf) {
-      lm = nullptr;
-      mCompositorChild = nullptr;
-      return;
-    }
-    lf->SetShadowManager(shadowManager);
-    lf->IdentifyTextureHost(textureFactoryIdentifier);
-    ImageBridgeChild::IdentifyCompositorTextureHost(textureFactoryIdentifier);
-    WindowUsesOMTC();
+  ShadowLayerForwarder* lf = lm->AsShadowForwarder();
 
-    mLayerManager = lm.forget();
+  if (!success || !lf) {
+    NS_WARNING("Failed to create an OMT compositor.");
+    DestroyCompositor();
     return;
   }
 
-  NS_WARNING("Failed to create an OMT compositor.");
-  DestroyCompositor();
-  // Compositor child had the only reference to LayerManager and will have
-  // deallocated it when being freed.
+  lf->SetShadowManager(shadowManager);
+  lf->IdentifyTextureHost(textureFactoryIdentifier);
+  ImageBridgeChild::IdentifyCompositorTextureHost(textureFactoryIdentifier);
+  WindowUsesOMTC();
+
+  mLayerManager = lm.forget();
 }
 
 bool nsBaseWidget::ShouldUseOffMainThreadCompositing()
 {
   return gfxPlatform::UsesOffMainThreadCompositing();
 }
 
 LayerManager* nsBaseWidget::GetLayerManager(PLayerTransactionChild* aShadowManager,
--- a/widget/nsBaseWidget.h
+++ b/widget/nsBaseWidget.h
@@ -436,16 +436,17 @@ protected:
    *
    * When this function returns, the compositor should not be 
    * able to access the opengl context anymore.
    * It is safe to call it several times if platform implementations
    * require the compositor to be destroyed before ~nsBaseWidget is
    * reached (This is the case with gtk2 for instance).
    */
   void DestroyCompositor();
+  void DestroyLayerManager();
 
   nsIWidgetListener* mWidgetListener;
   nsIWidgetListener* mAttachedWidgetListener;
   nsRefPtr<LayerManager> mLayerManager;
   nsRefPtr<LayerManager> mBasicLayerManager;
   nsRefPtr<CompositorChild> mCompositorChild;
   nsRefPtr<CompositorParent> mCompositorParent;
   nsRefPtr<mozilla::CompositorVsyncDispatcher> mCompositorVsyncDispatcher;
--- a/widget/windows/nsWindow.cpp
+++ b/widget/windows/nsWindow.cpp
@@ -666,21 +666,17 @@ NS_METHOD nsWindow::Destroy()
 
   // During the destruction of all of our children, make sure we don't get deleted.
   nsCOMPtr<nsIWidget> kungFuDeathGrip(this);
 
   /**
    * On windows the LayerManagerOGL destructor wants the widget to be around for
    * cleanup. It also would like to have the HWND intact, so we nullptr it here.
    */
-  if (mLayerManager) {
-    mLayerManager->Destroy();
-  }
-  mLayerManager = nullptr;
-  DestroyCompositor();
+  DestroyLayerManager();
 
   /* We should clear our cached resources now and not wait for the GC to
    * delete the nsWindow. */
   ClearCachedResources();
 
   // The DestroyWindow function destroys the specified window. The function sends WM_DESTROY
   // and WM_NCDESTROY messages to the window to deactivate it and remove the keyboard focus
   // from it. The function also destroys the window's menu, flushes the thread message queue,
@@ -6531,20 +6527,17 @@ bool nsWindow::OnHotKey(WPARAM wParam, L
 bool nsWindow::AutoErase(HDC dc)
 {
   return false;
 }
 
 void
 nsWindow::ClearCompositor(nsWindow* aWindow)
 {
-  if (aWindow->mLayerManager) {
-    aWindow->mLayerManager = nullptr;
-    aWindow->DestroyCompositor();
-  }
+  aWindow->DestroyLayerManager();
 }
 
 bool
 nsWindow::ShouldUseOffMainThreadCompositing()
 {
   // We don't currently support using an accelerated layer manager with
   // transparent windows so don't even try. I'm also not sure if we even
   // want to support this case. See bug 593471