1656266 - ThreadSanitizer: data race [@ nsWindow::NativeMoveResize] vs. [@ mozilla::wr::RenderCompositorEGL::GetBufferSize]

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Jason Kratzer [:jkratzer]]

Attachment: log_ffp_asan_24211.log.26972.txt

The attached crash information was detected while running CI tests with ThreadSanitizer on mozilla-central revision 20200724-b3a3c131a279.

For detailed crash information, see attachment.
General information about TSan reports
Why fix races?

Data races are undefined behavior and can cause crashes as well as correctness issues. Compiler optimizations can cause racy code to have unpredictable and hard-to-reproduce behavior.
Rating

If you think this race can cause crashes or correctness issues, it would be great to rate the bug appropriately as P1/P2 and/or indicating this in the bug. This makes it a lot easier for us to assess the actual impact that these reports make and if they are helpful to you.
False Positives / Benign Races

Typically, races reported by TSan are not false positives [1], but it is possible that the race is benign. Even in this case it would be nice to come up with a fix if it is easily doable and does not regress performance. Every race that we cannot fix will have to remain on the suppression list and slows down the overall TSan performance. Also note that seemingly benign races can possibly be harmful (also depending on the compiler, optimizations and the architecture) [2][3].

[1] One major exception is the involvement of uninstrumented code from third-party libraries.
[2] http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong
[3] How to miscompile programs with "benign" data races: https://www.usenix.org/legacy/events/hotpar11/tech/final_files/Boehm.pdf
Suppressing unfixable races

If the bug cannot be fixed, then a runtime suppression needs to be added in mozglue/build/TsanOptions.cpp. The suppressions match on the full stack, so it should be picked such that it is unique to this particular race. The bug number of this bug should also be included so we have some documentation on why this suppression was added.

Comment 1

[Christian Holler (:decoder)]

The 2 top-most frames of the second stack seem broken, but this is likely still a legit race and should be investigated.

Comment 2

[Aria Beingessner [:Gankra]]

This looks like another instance of a problem I've seen webrender run into a few times. The Renderer thread is looking up the window's size to find out the surface it has to render to, while the native (X11) window event management code is handling a system resize event.

Thankfully this looks like a less messy one to handle than usual. I can't remember how important it is for the frame(s) that occur during a resize to have the "right" size information. It's possible we can "just" guard the size with an Atomic, but someone with this stuff more in cache should give their thoughts.

Any opinion, gw?

Comment 3

[Glenn Watson [:gw]]

If the wrong size is present during a resize, you'll see a strip of background color pixels along the edge(s) (if the reported size is smaller than actual window size) or some cropping (if reported size is larger than actual window size).

That's from WR's perspective, however there may be other impacts in the Gecko code related to framebuffer resizes and allocations that I'm unaware of.

Markus might have a better idea of what's involved here, or know who would.

Comment 4

[Aria Beingessner [:Gankra]]

Attachment: Bug 1656266 - Make the window client size atomic to avoid a read/write race. r?mstange

Comment 5

[Aria Beingessner [:Gankra]]

Here's the naive solution with my understanding of the situation, in case we decide it's acceptable.

Comment 6

[Markus Stange [:mstange]]

Yes, this is a problem, and it's a pretty long-standing one. This not just a potential data race, but also racy with respect to rendering correctness. If the compositor draws at the new size before the main thread has submitted the browser chrome paint at the new size, we will surely paint something that's cropped or incomplete. So we really want to deliver the widget size along with the browser paint transaction. But then we need to check, per platform, what the compositor should do while its widget size is stale. For example, on macOS, we currently suspend "presents" (but not composites) during that time window; so the compositor does a bunch of unnecessary work compositing incomplete content which is ultimately discarded, until the up-to-date content is composited and presented.

I think we should fix the Linux data race in this bug and file a new bug to fix things properly. There is also a macOS bug for the thread-unsafe widget size access, bug 1533550.

Comment 7

[Markus Stange [:mstange]]

I have filed bug 1665726 on fixing things properly.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be6bd5dfe991
Make the window client size atomic to avoid a read/write race. r=mstange

Comment 9

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae330325e9c6
Backed out changeset be6bd5dfe991 for causing bustages in GtkCompositorWidget.h

Comment 10

[Alexandru Michis [:malexandru]]

Backed out changeset be6bd5dfe991 (Bug 1656266) for causing bustages in GtkCompositorWidget.h

Backout link: https://hg.mozilla.org/integration/autoland/rev/ae330325e9c64b0b885c0475649b91b8ae196861

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&tochange=7cdb3eb5959809406cf73b4082b36f027953a010&fromchange=cbedf38923ba0a5425ae5f36b82a797435b8a8e2

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=316058238&repo=autoland&lineNumber=14617

[task 2020-09-18T08:55:09.399Z] 08:55:09    ERROR -  /builds/worker/workspace/obj-build/dist/include/mozilla/widget/GtkCompositorWidget.h:87:31: error: implicit instantiation of undefined template 'mozilla::Atomic<mozilla::gfx::IntSizeTyped<mozilla::LayoutDevicePixel>, mozilla::SequentiallyConsistent, void>'
[task 2020-09-18T08:55:09.399Z] 08:55:09     INFO -    Atomic<LayoutDeviceIntSize> mClientSize;
[task 2020-09-18T08:55:09.399Z] 08:55:09     INFO -                                ^
[task 2020-09-18T08:55:09.399Z] 08:55:09     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Atomics.h:367:7: note: template is declared here
[task 2020-09-18T08:55:09.399Z] 08:55:09     INFO -  class Atomic;
[task 2020-09-18T08:55:09.399Z] 08:55:09     INFO -        ^
[task 2020-09-18T08:55:09.399Z] 08:55:09     INFO -  In file included from /builds/worker/checkouts/gecko/gfx/gl/GLContextProviderGLX.cpp:25:
[task 2020-09-18T08:55:09.399Z] 08:55:09    ERROR -  /builds/worker/workspace/obj-build/dist/include/mozilla/widget/GtkCompositorWidget.h:58:60: error: cannot initialize return object of type 'mozilla::widget::CompositorWidgetDelegate *' with an rvalue of type 'mozilla::widget::GtkCompositorWidget *'
[task 2020-09-18T08:55:09.399Z] 08:55:09     INFO -    CompositorWidgetDelegate* AsDelegate() override { return this; }
[task 2020-09-18T08:55:09.399Z] 08:55:09     INFO -                                                             ^~~~
[task 2020-09-18T08:55:09.399Z] 08:55:09     INFO -  2 errors generated.
[task 2020-09-18T08:55:09.400Z] 08:55:09     INFO -  /builds/worker/checkouts/gecko/config/rules.mk:723: recipe for target 'GLContextProviderGLX.o' failed
[task 2020-09-18T08:55:09.400Z] 08:55:09    ERROR -  make[4]: *** [GLContextProviderGLX.o] Error 1
[task 2020-09-18T08:55:09.400Z] 08:55:09     INFO -  make[4]: Leaving directory '/builds/worker/workspace/obj-build/gfx/gl'
[task 2020-09-18T08:55:09.401Z] 08:55:09     INFO -  /builds/worker/checkouts/gecko/config/recurse.mk:72: recipe for target 'gfx/gl/target-objects' failed
[task 2020-09-18T08:55:09.402Z] 08:55:09    ERROR -  make[3]: *** [gfx/gl/target-objects] Error 2
[task 2020-09-18T08:55:09.402Z] 08:55:09     INFO -  make[3]: *** Waiting for unfinished jobs....

Comment 11

[Aria Beingessner [:Gankra]]

Whoops, mistook our Atomic as something which can wrap anything in a mutex automagically like std::atomic. I'll push up a proper version on monday.

In the meantime, it appears that Bug 1607712 is essentially a dupe, in that it's another race on this same field, but when using Basic Compositor instead of Webrender.

Comment 12

[Aria Beingessner [:Gankra]]

Attachment: Bug 1656266 - Wrap gtk window mClientSize in a lock to avoid a datarace. r?mstange

This is also a fix for Bug 1607712 which covers the BasicCompositor case

Comment 13

[Aria Beingessner [:Gankra]]

Attachment: Bug 1656266 - Remove fixed TSAN supression. r?mstange

This supression was setup for Bug 1607712, but it was the same underlying issue as this bug.

Depends on D91036

Comment 14

[Aria Beingessner [:Gankra]]

Went down a weird rabbit hold with linking/configure bugs around std::atomic, so going with the more explicit and in-tree mozilla::DataMutex instead.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f16043fa384a
Wrap gtk window mClientSize in a lock to avoid a datarace. r=mstange
https://hg.mozilla.org/integration/autoland/rev/d4b32c0c7e0f
Remove fixed TSAN supression. r=decoder

Comment 16

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/f16043fa384a
https://hg.mozilla.org/mozilla-central/rev/d4b32c0c7e0f

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:Gankra, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 20

[Aria Beingessner [:Gankra]]

Doesn't need an uplift; problem wasn't causing any known issues in the wild.