Async OMTP doesn't display the nightly search bar correctly

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mchang, Assigned: mchang)

Tracking

unspecified
mozilla57
x86
Windows 10
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
Created attachment 8894008 [details]
Screenshot

See attached screenshot. Sometimes I can get two search bars, but mostly its the cursor is being rendered somewhere wrong. Eventually though, the page repaints correctly.
(Assignee)

Comment 1

a year ago
Oh even happens with sync OMTP.
(Assignee)

Comment 2

a year ago
Created attachment 8895587 [details] [diff] [review]
Sync Textures once all async omtp paints are done

The problem was that we weren't always syncing the D3D11 sync object correctly and we would have a race. Pre-OMTP, each client painted layer would finish painting into it's textures then at the end of a transaction, we'd sync via the D3D11 sync texture.

With OMTP, we could have a race where the async omtp on the paint thread hasn't finished yet so incomplete data would be in the texture, and we'd sync on the main thread, resulting in incomplete data on the compositor side, therefore showing random things.

This patch does something like having an incomplete transaction. If we have a async omtp paint, we set a flag in the ClientLayerManager. Once a transaction is complete, we check if we have to forward on the main thread or paint thread. The sync code really isn't thread safe, but it won't race since we'll either only call synchronize on the paint thread or the main thread.
Attachment #8895587 - Flags: review?(matt.woodrow)
(Assignee)

Comment 3

a year ago
Also, we're guaranteed that the sync will happen AFTER the async omtp paints because we append it at the end of the paint thread's event loop.
(Assignee)

Updated

a year ago
Blocks: 1387326
Comment on attachment 8895587 [details] [diff] [review]
Sync Textures once all async omtp paints are done

Review of attachment 8895587 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, can you also add comments about the sync code not being thread safe, but why we think we won't actually race.

Either that, or just throw a mutex into the sync object and not need to worry about it.

::: gfx/layers/PaintThread.cpp
@@ +235,5 @@
> +{
> +  MOZ_ASSERT(IsOnPaintThread());
> +  MOZ_ASSERT(aForwarder);
> +
> +  aForwarder->GetSyncObject()->Synchronize();

Can we ever have a race where the sync object goes away on the main thread, and GetSyncObject returns null here?

Why don't we grab the sync object on the main thread, and put a reference to that into the runnable, rather than the ShadowLayerForwarder?
Attachment #8895587 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 5

a year ago
Created attachment 8896059 [details] [diff] [review]
Sync Textures once all async OMTP paints are done

Carrying r+
Attachment #8895587 - Attachment is obsolete: true
Attachment #8896059 - Flags: review+
(Assignee)

Comment 6

a year ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Comment on attachment 8895587 [details] [diff] [review]
> Sync Textures once all async omtp paints are done
> 
> Review of attachment 8895587 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, can you also add comments about the sync code not being thread
> safe, but why we think we won't actually race.
> 
> Either that, or just throw a mutex into the sync object and not need to
> worry about it.

I threw a mutex in it. Also fixed lots of crashes!

> 
> ::: gfx/layers/PaintThread.cpp
> @@ +235,5 @@
> > +{
> > +  MOZ_ASSERT(IsOnPaintThread());
> > +  MOZ_ASSERT(aForwarder);
> > +
> > +  aForwarder->GetSyncObject()->Synchronize();
> 
> Can we ever have a race where the sync object goes away on the main thread,
> and GetSyncObject returns null here?

It shouldn't since we don't start the next transaction until all paints are done. If someone closes the process, then it doesn't matter. But I added a ref to the sync object instead. 


> Why don't we grab the sync object on the main thread, and put a reference to
> that into the runnable, rather than the ShadowLayerForwarder?

Good idea, did that in the updated version. Thanks!

Comment 8

a year ago
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7f2be7b6b95
Sync Textures once all async OMTP paints are done. r=mattwoodrow

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e7f2be7b6b95
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.