1726807 - Wayland: Some crashes with "gfx.webrender.compositor.force-enabled:true" don't trigger crash reports

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[antheo.lerouzic]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Firefox/91.0

Steps to reproduce:

Enable "gfx.webrender.compositor.force-enabled" on recent version of firefox, on gnome wayland at least.

Actual results:

Firefox crash very often, leaving no crash report unfortunately.

Expected results:

Firefox should not crash.

Comment 1

[antheo.lerouzic]

There is no crash report, but if I open firefox in a terminal, I have these lines while it crash :

Exiting due to channel error.
Exiting due to channel error.
Exiting due to channel error.
Processus arrêté

Thanks you!

Comment 2

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

The Bugbug bot thinks this bug should belong to the 'Core::Graphics: WebRender' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 3

[Jan Alexander Steffens [:heftig]]

I think crash reports aren't created when the Wayland compositor runs into a protocol error and closes the connection?

Comment 4

[Laurențiu Nicola]

It might not be the same problem, but I have a repro for a Wayland compositor crash that doesn't get logged to about:crashes: open https://www.amazon.de/-/en/WER05056490001-Wera-Bit-Assortment/dp/B00I8MYMT2/257-0702117-8099114?psc=1 and click on the video (last thumbnail) under the product image.

Comment 5

[Teoh Han Hui]

This is an experimental feature, right? So I'm sure it should be tried on nightly, not on a stable version.

Comment 6

[Robert Mader [:rmader]]

Antheon, do you see this issue on FF stable (91) or nightly (93)? As said in comment 5, it's an experimental feature, which is why we don't backport fixes to the stable channel - thus there's a high chance that the issue is fixed already.

Comment 7

[Laurențiu Nicola]

@rmader, I see the crash in comment 4 in Nightly (2021-08-16), and I think it's the same as this one. Do you want me to file another issue?

Comment 8

[antheo.lerouzic]

The issue is on stable (91) at least.
I will try on the last nightly.

Comment 9

[Robert Mader [:rmader]]

(In reply to Laurențiu Nicola from comment #7)

@rmader, I see the crash in comment 4 in Nightly (2021-08-16), and I think it's the same as this one. Do you want me to file another issue?

The issue here is still quite unspecific (crash without crash report) - lets continue here then. Could you try to reproduce that crash when running FF with WAYLAND_DEBUG=1? Thanks!

Comment 10

[Laurențiu Nicola]

Attachment: compositor crash

```

Comment 11

[antheo.lerouzic]

It seems to work with nightly, thanks!

Comment 12

[Laurențiu Nicola]

Still crashes for me on the latest Nightly.

Comment 13

[Robert Mader [:rmader]]

(In reply to antheo.lerouzic from comment #11)

It seems to work with nightly, thanks!

Thanks for testing!

(In reply to Laurențiu Nicola from comment #10)

So we have

[1496219.901] [e-mail](wp_viewport@342, 0, "x and y values must be zero or positive and width and height valuest must be positive or all values must be -1 to unset the vie")
Gdk-Message: 17:10:37.995: Error 71 (Protocol error) dispatching to Wayland display.

which gets triggered in https://searchfox.org/mozilla-central/source/gfx/layers/NativeLayerWayland.cpp#203 - and it's indeed is not obvious that we can't run into forbidden conditions. Lets take over the issue and make sure that such crashes at least trigger a proper crash report.

Comment 14

[Robert Mader [:rmader]]

Attachment: Bug 1726807 - Assert on valid wp_viewport values, r=#gfx-reviewers

Submitting invalid values to the Wayland compositor will trigger a
protocol error, which in turn will trigger an unhandled crash.
In order to create crash reports in such cases, assert on valid
values in such cases.

In the long run we can replace this with a general Wayland protocol
error handler.

Comment 15

[Markus Stange [:mstange]]

Do we have a bug on the fact that the crash reporter isn't catching this crash? It seems worth filing one.

Comment 16

[Robert Mader [:rmader]]

Laurențiu: I can reproduce the issue and it looks like a rounding issue. The values I get are: 0,000000 -0,015625 854,000000 480,000000 - just enough to trigger the protocol error :) Thanks for spotting and finding a reproducer!

Comment 17

[Robert Mader [:rmader]]

(In reply to Markus Stange [:mstange] from comment #15)

Do we have a bug on the fact that the crash reporter isn't catching this crash? It seems worth filing one.

Right, filed bug 1726923. The last time I checked there was still something missing from the libwayland side to make this straight forward, but that may have changed by now.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d4a2898ecf1
Assert on valid wp_viewport values, r=gfx-reviewers,mstange

Comment 19

[Robert Mader [:rmader]]

Attachment: Bug 1726807 - Avoid crashes caused by floating point errors, r=#gfx-reviewers

We always expect integer values here, even with fractional scaling,
and if floating point errors cause bufferClip.x or bufferClip.y to
fall slightly below 0, this will cause a protocol error.
We can savely avoid this by simply rounding the rect.

Comment 20

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/7d4a2898ecf1

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23f3a0afb545
Avoid crashes caused by floating point errors, r=gfx-reviewers,lsalzman

Comment 22

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/23f3a0afb545

Comment 23

[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 24

[Laurențiu Nicola]

@rmader: my crash seems fixed now, thanks!

Comment 25

[Robert Mader [:rmader]]

Comment on attachment 9237354 [details]
Bug 1726807 - Assert on valid wp_viewport values, r=#gfx-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Some crashes do not create crash reports (first patch).
  Crashes because of floating point errors (second patch).
  Both happens only with the experimental feature gfx.webrender.compositor.force-enabled set to true.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Both patches are rather trivial and the code paths are exclusively used with the experimental setting enabled.
• String changes made/needed:

Comment 26

[Robert Mader [:rmader]]

(In reply to Laurențiu Nicola from comment #24)

@rmader: my crash seems fixed now, thanks!

Thanks for helping discovering and fixing this bug!

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9237354 [details]
Bug 1726807 - Assert on valid wp_viewport values, r=#gfx-reviewers

Approved for 92.0b8.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5ead28ececf4
https://hg.mozilla.org/releases/mozilla-beta/rev/30a4566c5805