Closed Bug 1656266 Opened 3 years ago Closed 3 years ago

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


(Core :: Graphics: WebRender, defect)

Not set



83 Branch
Tracking Status
firefox81 --- wontfix
firefox82 --- wontfix
firefox83 --- fixed


(Reporter: jkratzer, Assigned: Gankra)


(Blocks 2 open bugs)



(3 files, 1 obsolete file)

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.

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.
[3] How to miscompile programs with "benign" data races:
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.

Blocks: tsan

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

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?

Component: Graphics → Graphics: WebRender
Flags: needinfo?(gwatson)
OS: Unspecified → Linux
Hardware: Unspecified → Desktop
Summary: ThreadSanitizer: data race [@ get] vs. [@ mozilla::gfx::PathSink::SetCurrentPoint] → ThreadSanitizer: data race [@ nsWindow::NativeMoveResize] vs. [@ mozilla::wr::RenderCompositorEGL::GetBufferSize]

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.

Flags: needinfo?(gwatson) → needinfo?(mstange.moz)
Assignee: nobody → a.beingessner

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

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.

Flags: needinfo?(mstange.moz)

I have filed bug 1665726 on fixing things properly.

Pushed by
Make the window client size atomic to avoid a read/write race. r=mstange
Backout by
Backed out changeset be6bd5dfe991 for causing bustages in GtkCompositorWidget.h

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

Backout link:

Push with failures:

Failure log:

[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/ 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/ 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....
Flags: needinfo?(a.beingessner)

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.

Flags: needinfo?(a.beingessner)
Attachment #9176320 - Attachment is obsolete: true

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

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

Depends on D91036

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.

Pushed by
Wrap gtk window mClientSize in a lock to avoid a datarace. r=mstange
Remove fixed TSAN supression. r=decoder
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 83 Branch

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

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.

Flags: needinfo?(a.beingessner)

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

Flags: needinfo?(a.beingessner)
You need to log in before you can comment on or make changes to this bug.