Bug 1125848 - Backout because of crashes. r=me
authorNicolas Silva <nsilva@mozilla.com>
Fri, 13 Mar 2015 15:13:12 +0100
changeset 233557 906c7ac5ac40
parent 233556 8940a7b2bef3
child 233558 7e4e5e971d95
push id28417
push userryanvm@gmail.com
push dateFri, 13 Mar 2015 19:52:44 +0000
treeherdermozilla-central@977add19414a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersme
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 - Backout because of crashes. r=me
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
@@ -1,16 +1,15 @@
 /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* vim: set sw=2 ts=2 et tw=80 : */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "mozilla/layers/CompositorChild.h"
-#include "mozilla/layers/CompositorParent.h"
 #include <stddef.h>                     // for size_t
 #include "ClientLayerManager.h"         // for ClientLayerManager
 #include "base/message_loop.h"          // for MessageLoop
 #include "base/process_util.h"          // for OpenProcessHandle
 #include "base/task.h"                  // for NewRunnableMethod, etc
 #include "base/tracked.h"               // for FROM_HERE
 #include "mozilla/layers/LayerTransactionChild.h"
 #include "mozilla/layers/PLayerTransactionChild.h"
@@ -36,78 +35,38 @@ namespace mozilla {
 namespace layers {
 
 /*static*/ CompositorChild* CompositorChild::sCompositor;
 
 Atomic<int32_t> CompositableForwarder::sSerialCounter(0);
 
 CompositorChild::CompositorChild(ClientLayerManager *aLayerManager)
   : mLayerManager(aLayerManager)
-  , mCanSend(false)
+  , mCanSend(true)
 {
 }
 
 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()
 {
-  // This must not be called from the destructor!
-  MOZ_ASSERT(mRefCnt != 0);
-
-  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;
-  }
-
+  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.
-  nsRefPtr<CompositorChild> selfRef = this;
-  MessageLoop::current()->PostTask(FROM_HERE,
-             NewRunnableFunction(DeferredDestroyCompositor, mCompositorParent, selfRef));
 }
 
 bool
 CompositorChild::LookupCompositorFrameMetrics(const FrameMetrics::ViewID aId,
                                               FrameMetrics& aFrame)
 {
   SharedFrameMetricsData* data = mFrameMetricsTable.Get(aId);
   if (data) {
@@ -130,42 +89,28 @@ 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;
 }
 
@@ -532,16 +477,18 @@ 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,22 +55,16 @@ 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;
@@ -169,19 +163,16 @@ 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,56 +176,78 @@ 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) {
-    mCompositorChild->Destroy();
-    mCompositorChild = nullptr;
-    mCompositorParent = nullptr;
-  }
-}
+    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();
 
-void nsBaseWidget::DestroyLayerManager()
-{
-  if (mLayerManager) {
-    mLayerManager->Destroy();
-    mLayerManager = 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));
   }
-  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);
   }
 
-  DestroyLayerManager();
+  DestroyCompositor();
 
 #ifdef NOISY_WIDGET_LEAKS
   gNumWidgets--;
   printf("WIDGETS- = %d\n", gNumWidgets);
 #endif
 
   delete mOriginalBounds;
 
@@ -1054,37 +1076,35 @@ 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 && !mCompositorChild,
-    "Should have properly cleaned up the previous PCompositor pair beforehand");
-
-  if (mCompositorChild) {
-    mCompositorChild->Destroy();
-  }
+  MOZ_ASSERT(!mCompositorParent,
+    "Should have properly cleaned up the previous CompositorParent beforehand");
 
   // 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->OpenSameProcess(mCompositorParent);
+  mCompositorChild->Open(parentChannel, childMessageLoop, ipc::ChildSide);
 
   if (gfxPrefs::AsyncPanZoomEnabled() &&
       (WindowType() == eWindowType_toplevel || WindowType() == eWindowType_child)) {
     ConfigureAPZCTreeManager();
   }
 
   TextureFactoryIdentifier textureFactoryIdentifier;
   PLayerTransactionChild* shadowManager = nullptr;
@@ -1103,30 +1123,36 @@ void nsBaseWidget::CreateCompositor(int 
 #endif
 
   bool success = false;
   if (!backendHints.IsEmpty()) {
     shadowManager = mCompositorChild->SendPLayerTransactionConstructor(
       backendHints, 0, &textureFactoryIdentifier, &success);
   }
 
-  ShadowLayerForwarder* lf = lm->AsShadowForwarder();
+  if (success) {
+    ShadowLayerForwarder* lf = lm->AsShadowForwarder();
+    if (!lf) {
+      lm = nullptr;
+      mCompositorChild = nullptr;
+      return;
+    }
+    lf->SetShadowManager(shadowManager);
+    lf->IdentifyTextureHost(textureFactoryIdentifier);
+    ImageBridgeChild::IdentifyCompositorTextureHost(textureFactoryIdentifier);
+    WindowUsesOMTC();
 
-  if (!success || !lf) {
-    NS_WARNING("Failed to create an OMT compositor.");
-    DestroyCompositor();
+    mLayerManager = lm.forget();
     return;
   }
 
-  lf->SetShadowManager(shadowManager);
-  lf->IdentifyTextureHost(textureFactoryIdentifier);
-  ImageBridgeChild::IdentifyCompositorTextureHost(textureFactoryIdentifier);
-  WindowUsesOMTC();
-
-  mLayerManager = lm.forget();
+  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.
 }
 
 bool nsBaseWidget::ShouldUseOffMainThreadCompositing()
 {
   return gfxPlatform::UsesOffMainThreadCompositing();
 }
 
 LayerManager* nsBaseWidget::GetLayerManager(PLayerTransactionChild* aShadowManager,
--- a/widget/nsBaseWidget.h
+++ b/widget/nsBaseWidget.h
@@ -436,17 +436,16 @@ 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,17 +666,21 @@ 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.
    */
-  DestroyLayerManager();
+  if (mLayerManager) {
+    mLayerManager->Destroy();
+  }
+  mLayerManager = nullptr;
+  DestroyCompositor();
 
   /* 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,
@@ -6527,17 +6531,20 @@ bool nsWindow::OnHotKey(WPARAM wParam, L
 bool nsWindow::AutoErase(HDC dc)
 {
   return false;
 }
 
 void
 nsWindow::ClearCompositor(nsWindow* aWindow)
 {
-  aWindow->DestroyLayerManager();
+  if (aWindow->mLayerManager) {
+    aWindow->mLayerManager = nullptr;
+    aWindow->DestroyCompositor();
+  }
 }
 
 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