Bug 1499186 Part 2 - Only allow one in flight paint at a time, r=nical.
authorBrian Hackett <bhackett1024@gmail.com>
Wed, 17 Oct 2018 10:32:13 -0600
changeset 490286 b341213ecf95d1448abc0a38ef058138b41324ee
parent 490285 0fba5665bc65c55a4d1f27905a2a60396dbe2bb8
child 490287 7400b733b50f3a9d4fa8ca18363a9dbb52956398
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewersnical
bugs1499186
milestone64.0a1
Bug 1499186 Part 2 - Only allow one in flight paint at a time, r=nical.
toolkit/recordreplay/ipc/Channel.h
toolkit/recordreplay/ipc/ChildIPC.cpp
toolkit/recordreplay/ipc/ChildInternal.h
toolkit/recordreplay/ipc/ChildNavigation.cpp
toolkit/recordreplay/ipc/DisabledIPC.cpp
toolkit/recordreplay/ipc/JSControl.cpp
--- a/toolkit/recordreplay/ipc/Channel.h
+++ b/toolkit/recordreplay/ipc/Channel.h
@@ -373,21 +373,25 @@ struct FatalErrorMessage : public Messag
 
 // The format for graphics data which will be sent to the middleman process.
 // This needs to match the format expected for canvas image data, to avoid
 // transforming the data before rendering it in the middleman process.
 static const gfx::SurfaceFormat gSurfaceFormat = gfx::SurfaceFormat::R8G8B8X8;
 
 struct PaintMessage : public Message
 {
+  // Checkpoint whose state is being painted.
+  uint32_t mCheckpointId;
+
   uint32_t mWidth;
   uint32_t mHeight;
 
-  PaintMessage(uint32_t aWidth, uint32_t aHeight)
+  PaintMessage(uint32_t aCheckpointId, uint32_t aWidth, uint32_t aHeight)
     : Message(MessageType::Paint, sizeof(*this))
+    , mCheckpointId(aCheckpointId)
     , mWidth(aWidth)
     , mHeight(aHeight)
   {}
 };
 
 struct HitCheckpointMessage : public Message
 {
   uint32_t mCheckpointId;
--- a/toolkit/recordreplay/ipc/ChildIPC.cpp
+++ b/toolkit/recordreplay/ipc/ChildIPC.cpp
@@ -418,42 +418,56 @@ SetVsyncObserver(VsyncObserver* aObserve
 static void
 NotifyVsyncObserver()
 {
   if (gVsyncObserver) {
     gVsyncObserver->NotifyVsync(TimeStamp::Now());
   }
 }
 
-void
+// Whether an update has been sent to the compositor for a normal paint, and we
+// haven't reached PaintFromMainThread yet. This is used to preserve the
+// invariant that there can be at most one paint performed between two
+// checkpoints, other than repaints triggered by the debugger.
+static bool gHasActivePaint;
+
+bool
 OnVsync()
 {
   // In the repainting stress mode, we create a new checkpoint on every vsync
   // message received from the UI process. When we notify the parent about the
   // new checkpoint it will trigger a repaint to make sure that all layout and
   // painting activity can occur when diverged from the recording.
   if (parent::InRepaintStressMode()) {
     CreateCheckpoint();
   }
+
+  // After a paint starts, ignore incoming vsyncs until the paint completes.
+  return !gHasActivePaint;
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // Painting
 ///////////////////////////////////////////////////////////////////////////////
 
-// Graphics memory is only written on the compositor thread and read on the
-// main thread and by the middleman. The gNumPendingPaints counter is used to
-// synchronize access, so that data is not read until the paint has completed.
-static Maybe<PaintMessage> gPaintMessage;
-static size_t gNumPendingPaints;
-
-// Target buffer for the draw target created by the child process widget.
+// Target buffer for the draw target created by the child process widget, which
+// the compositor thread writes to.
 static void* gDrawTargetBuffer;
 static size_t gDrawTargetBufferSize;
 
+// Dimensions of the last paint which the compositor performed.
+static size_t gPaintWidth, gPaintHeight;
+
+// How many updates have been sent to the compositor thread and haven't been
+// processed yet. This can briefly become negative if the main thread sends an
+// update and the compositor processes it before the main thread reaches
+// NotifyPaintStart. Outside of this window, the compositor can only write to
+// gDrawTargetBuffer or update gPaintWidth/gPaintHeight if this is non-zero.
+static Atomic<int32_t, SequentiallyConsistent, Behavior::DontPreserve> gNumPendingPaints;
+
 // ID of the compositor thread.
 static Atomic<size_t, SequentiallyConsistent, Behavior::DontPreserve> gCompositorThreadId;
 
 already_AddRefed<gfx::DrawTarget>
 DrawTargetForRemoteDrawing(LayoutDeviceIntSize aSize)
 {
   MOZ_RELEASE_ASSERT(!NS_IsMainThread());
 
@@ -464,17 +478,18 @@ DrawTargetForRemoteDrawing(LayoutDeviceI
   } else {
     gCompositorThreadId = threadId;
   }
 
   if (aSize.IsEmpty()) {
     return nullptr;
   }
 
-  gPaintMessage = Some(PaintMessage(aSize.width, aSize.height));
+  gPaintWidth = aSize.width;
+  gPaintHeight = aSize.height;
 
   gfx::IntSize size(aSize.width, aSize.height);
   size_t bufferSize = layers::ImageDataSerializer::ComputeRGBBufferSize(size, gSurfaceFormat);
   MOZ_RELEASE_ASSERT(bufferSize <= parent::GraphicsMemorySize);
 
   if (bufferSize != gDrawTargetBufferSize) {
     free(gDrawTargetBuffer);
     gDrawTargetBuffer = malloc(bufferSize);
@@ -488,59 +503,58 @@ DrawTargetForRemoteDrawing(LayoutDeviceI
                                           /* aUninitialized = */ true);
   if (!drawTarget) {
     MOZ_CRASH();
   }
 
   return drawTarget.forget();
 }
 
-static void
-FinishInProgressPaint(MonitorAutoLock&)
-{
-  while (gNumPendingPaints) {
-    gMonitor->Wait();
-  }
-}
-
 void
 NotifyPaintStart()
 {
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
 
-  MonitorAutoLock lock(*gMonitor);
+  // A new paint cannot be triggered until the last one finishes and has been
+  // sent to the middleman.
+  MOZ_RELEASE_ASSERT(HasDivergedFromRecording() || !gHasActivePaint);
+
   gNumPendingPaints++;
+  gHasActivePaint = true;
+
+  CreateCheckpoint();
 }
 
 static void
 PaintFromMainThread()
 {
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
 
-  // If another paint started before we got here, finish it first.
-  {
-    MonitorAutoLock lock(*gMonitor);
-    FinishInProgressPaint(lock);
-  }
+  // There cannot not be any other in flight paints.
+  MOZ_RELEASE_ASSERT(!gNumPendingPaints);
 
-  if (IsActiveChild() && gPaintMessage.isSome()) {
+  // Clear the active flag now that we have completed the paint.
+  MOZ_RELEASE_ASSERT(gHasActivePaint);
+  gHasActivePaint = false;
+
+  if (IsActiveChild() && gDrawTargetBuffer) {
     memcpy(gGraphicsShmem, gDrawTargetBuffer, gDrawTargetBufferSize);
-    gChannel->SendMessage(gPaintMessage.ref());
+    gChannel->SendMessage(PaintMessage(navigation::LastNormalCheckpoint(),
+                                       gPaintWidth, gPaintHeight));
   }
 }
 
 void
 NotifyPaintComplete()
 {
   MOZ_RELEASE_ASSERT(Thread::Current()->Id() == gCompositorThreadId);
 
   // Notify the main thread in case it is waiting for this paint to complete.
   {
     MonitorAutoLock lock(*gMonitor);
-    MOZ_RELEASE_ASSERT(gNumPendingPaints);
     if (--gNumPendingPaints == 0) {
       gMonitor->Notify();
     }
   }
 
   // Notify the middleman about the completed paint from the main thread.
   NS_DispatchToMainThread(NewRunnableFunction("PaintFromMainThread", PaintFromMainThread));
 }
@@ -553,50 +567,56 @@ Repaint(size_t* aWidth, size_t* aHeight)
 
   // Don't try to repaint if the first normal paint hasn't occurred yet.
   if (!gCompositorThreadId) {
     *aWidth = 0;
     *aHeight = 0;
     return;
   }
 
-  // Create an artifical vsync to see if graphics have changed since the last
-  // paint and a new paint is needed.
-  NotifyVsyncObserver();
-
-  {
-    MonitorAutoLock lock(*gMonitor);
+  // Ignore the request to repaint if the compositor thread has already
+  // diverged from the recording. In this case we have already done a repaint
+  // and the last graphics we sent will still be correct.
+  Thread* compositorThread = Thread::GetById(gCompositorThreadId);
+  if (!compositorThread->WillDivergeFromRecordingSoon()) {
+    // Create an artifical vsync to see if graphics have changed since the last
+    // paint and a new paint is needed.
+    NotifyVsyncObserver();
 
     if (gNumPendingPaints) {
       // Allow the compositor to diverge from the recording so it can perform
-      // the paint we just triggered, or any in flight paint that that existed
-      // at the point we are paused at.
+      // any paint we just triggered, or finish any in flight paint that that
+      // existed at the point we are paused at.
       Thread::GetById(gCompositorThreadId)->SetShouldDivergeFromRecording();
 
-      FinishInProgressPaint(lock);
+      // Wait for the compositor to finish all in flight paints, including any
+      // one we just triggered.
+      MonitorAutoLock lock(*gMonitor);
+      while (gNumPendingPaints) {
+        gMonitor->Wait();
+      }
     }
   }
 
-  if (gPaintMessage.isSome()) {
+  if (gDrawTargetBuffer) {
     memcpy(gGraphicsShmem, gDrawTargetBuffer, gDrawTargetBufferSize);
-    *aWidth = gPaintMessage.ref().mWidth;
-    *aHeight = gPaintMessage.ref().mHeight;
+    *aWidth = gPaintWidth;
+    *aHeight = gPaintHeight;
   } else {
     *aWidth = 0;
     *aHeight = 0;
   }
 }
 
 static bool
 CompositorCanPerformMiddlemanCalls()
 {
   // After repainting finishes the compositor is not allowed to send call
   // requests to the middleman anymore.
-  MonitorAutoLock lock(*gMonitor);
-  return gNumPendingPaints != 0;
+  return !!gNumPendingPaints;
 }
 
 bool
 SuppressMessageAfterDiverge(IPC::Message* aMsg)
 {
   MOZ_RELEASE_ASSERT(HasDivergedFromRecording());
 
   // Only messages necessary for compositing can be sent after the sending
@@ -616,17 +636,17 @@ SuppressMessageAfterDiverge(IPC::Message
 
   return true;
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // Checkpoint Messages
 ///////////////////////////////////////////////////////////////////////////////
 
-// When recording, the time when the last HitCheckpoint message was sent.
+// The time when the last HitCheckpoint message was sent.
 static double gLastCheckpointTime;
 
 // When recording and we are idle, the time when we became idle.
 static double gIdleTimeStart;
 
 void
 BeginIdleTime()
 {
--- a/toolkit/recordreplay/ipc/ChildInternal.h
+++ b/toolkit/recordreplay/ipc/ChildInternal.h
@@ -71,16 +71,19 @@ void Repaint(size_t* aWidth, size_t* aHe
 // Called when running forward, immediately before hitting a normal or
 // temporary checkpoint.
 void BeforeCheckpoint();
 
 // Called immediately after hitting a normal or temporary checkpoint, either
 // when running forward or immediately after rewinding.
 void AfterCheckpoint(const CheckpointId& aCheckpoint);
 
+// Get the ID of the last normal checkpoint.
+size_t LastNormalCheckpoint();
+
 } // namespace navigation
 
 namespace child {
 
 // IPC activity that can be triggered by navigation.
 void RespondToRequest(const js::CharBuffer& aBuffer);
 void HitCheckpoint(size_t aId, bool aRecordingEndpoint);
 void HitBreakpoint(bool aRecordingEndpoint, const uint32_t* aBreakpoints, size_t aNumBreakpoints);
--- a/toolkit/recordreplay/ipc/ChildNavigation.cpp
+++ b/toolkit/recordreplay/ipc/ChildNavigation.cpp
@@ -1106,16 +1106,22 @@ void
 AfterCheckpoint(const CheckpointId& aCheckpoint)
 {
   AutoDisallowThreadEvents disallow;
 
   MOZ_RELEASE_ASSERT(IsRecordingOrReplaying());
   gNavigation->AfterCheckpoint(aCheckpoint);
 }
 
+size_t
+LastNormalCheckpoint()
+{
+  return gNavigation->LastCheckpoint().mNormal;
+}
+
 void
 DebuggerRequest(js::CharBuffer* aRequestBuffer)
 {
   gNavigation->HandleDebuggerRequest(aRequestBuffer);
 }
 
 void
 SetBreakpoint(size_t aId, const BreakpointPosition& aPosition)
--- a/toolkit/recordreplay/ipc/DisabledIPC.cpp
+++ b/toolkit/recordreplay/ipc/DisabledIPC.cpp
@@ -46,17 +46,17 @@ void CreateCheckpoint()
 }
 
 void
 SetVsyncObserver(VsyncObserver* aObserver)
 {
   MOZ_CRASH();
 }
 
-void
+bool
 OnVsync()
 {
   MOZ_CRASH();
 }
 
 void
 NotifyPaintStart()
 {
--- a/toolkit/recordreplay/ipc/JSControl.cpp
+++ b/toolkit/recordreplay/ipc/JSControl.cpp
@@ -409,17 +409,17 @@ Middleman_HadRepaint(JSContext* aCx, uns
   if (!args.get(0).isNumber() || !args.get(1).isNumber()) {
     JS_ReportErrorASCII(aCx, "Bad width/height");
     return false;
   }
 
   size_t width = args.get(0).toNumber();
   size_t height = args.get(1).toNumber();
 
-  PaintMessage message(width, height);
+  PaintMessage message(CheckpointId::Invalid, width, height);
   parent::UpdateGraphicsInUIProcess(&message);
 
   args.rval().setUndefined();
   return true;
 }
 
 static bool
 Middleman_HadRepaintFailure(JSContext* aCx, unsigned aArgc, Value* aVp)