Closed Bug 1499186 Opened Last year Closed Last year

Show graphics updates in order when hitting breakpoints during replay


(Core :: Web Replay, defect)

Not set



Tracking Status
firefox64 --- fixed


(Reporter: bhackett1024, Assigned: bhackett1024)




(4 files)

Attached patch patchSplinter Review
Newly painted bitmaps sent from the child process as Paint messages are used by the middleman to immediately update the graphics shown by the UI process.  Before bug 1488808 this was fine, as the child blocked the main thread to wait for the paint to complete, ensuring that the paint shows the state of the DOM at the exact point where the painting data is sent to the middleman.  After bug 1488808, the paint shows the state where the checkpoint was created, but the paint is sent to the middleman at a later point in time: after the compositor finishes the update and a runnable executes on the main thread to send the data to the middleman. Between the time when the checkpoint was created and the painting data was posted, the main thread can run JS, update the DOM, etc.

This is a problem when a breakpoint is hit somewhere between the time the checkpoint is created and the time the associated paint completes.  When rewinding, the Paint message will be received before the breakpoint is hit, with the result being that earlier graphics (from the Paint message, reflecting the checkpoint) are shown before the later graphics that are expected when the process pauses at the breakpoint and repaints.  A similar problem happens when running forward: we show the later graphics first when hitting the breakpoint and repainting, then clobber that with the earlier graphics when the Paint message is received afterwards.

The solution to this is pretty simple: the graphics from a Paint message are only shown when both the Paint and the associated checkpoint hit have both been received, with no interim repainting due to pausing at a breakpoint.  This patch implements this fix, and a supporting fix that ensures we can't get into weird situations like having multiple in flight paints at a time that would complicate the solution here.  The latter is done by ignoring vsyncs (which might trigger new paints) that are received between the time a paint is triggered and the time it completes.
Allow recording/replaying processes to ignore vsyncs that are received before the last paint completes, and simplify the calls needed when a paint is triggered.
Attachment #9017299 - Flags: review?(nical.bugzilla)
Make sure there is only one paint happening at a time when recording/replaying, so that each paint is associated with the most recent checkpoint.
Attachment #9017300 - Flags: review?(nical.bugzilla)
Wait until both a Paint message and its associated checkpoint have been reached before updating the graphics in the UI process.
Attachment #9017301 - Flags: review?(nical.bugzilla)
Attachment #9017299 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 9017300 [details] [diff] [review]
Part 2 - Only allow one in flight paint at a time.

Review of attachment 9017300 [details] [diff] [review]:

::: toolkit/recordreplay/ipc/ChildIPC.cpp
@@ +452,5 @@
>  static void* gDrawTargetBuffer;
>  static size_t gDrawTargetBufferSize;
> +// Message for the last paint which the compositor generated.
> +static Maybe<PaintMessage> gPaintMessage;

nit: Obligatory warning about static initializers that add up and show startup down. Maybe a static PaintMessage* pointer would do here (I also see we have a ot of static UniquePtr in the code base so I suppose they are fine as well)?
Attachment #9017300 - Flags: review?(nical.bugzilla) → review+
Attachment #9017301 - Flags: review?(nical.bugzilla) → review+
Pushed by
Part 1 - Allow recording/replaying processes to ignore vsyncs, r=nical.
Part 2 - Only allow one in flight paint at a time, r=nical.
Part 3 - Don't show out of order graphics in the middleman, r=nical.
Bug 1499186 Part 1 - Allow recording/replaying processes to ignore vsyncs, r=nical.
Bug 1499186 Part 2 - Only allow one in flight paint at a time, r=nical.
Bug 1499186 Part 3 - Don't show out of order graphics in the middleman, r=nical.
You need to log in before you can comment on or make changes to this bug.