1670575 - macOS Crash in [@ webrender::render_task_graph::RenderTaskGraph::generate_passes_impl::assign_task_depth]

Status: RESOLVED WORKSFORME

(Core :: Graphics: WebRender, defect, P3)

Description

[Gabriele Svelto [:gsvelto]]

Crash report: https://crash-stats.mozilla.org/report/index/e3089c20-4fd3-4916-84d9-6ca0a0201010

Reason: EXC_BAD_ACCESS / EXC_I386_GPFLT

Top 10 frames of crashing thread:

0 XUL webrender::render_task_graph::RenderTaskGraph::generate_passes_impl::assign_task_depth gfx/wr/webrender/src/render_task_graph.rs:172
1 XUL webrender::render_task_graph::RenderTaskGraph::generate_passes_impl::assign_task_depth gfx/wr/webrender/src/render_task_graph.rs:170
2 XUL webrender::render_task_graph::RenderTaskGraph::generate_passes_impl::assign_task_depth gfx/wr/webrender/src/render_task_graph.rs:170
3 XUL webrender::render_task_graph::RenderTaskGraph::generate_passes_impl gfx/wr/webrender/src/render_task_graph.rs:186
4 XUL webrender::frame_builder::FrameBuilder::build gfx/wr/webrender/src/frame_builder.rs:574
5 XUL webrender::render_backend::Document::build_frame gfx/wr/webrender/src/render_backend.rs:621
6 XUL webrender::render_backend::RenderBackend::update_document gfx/wr/webrender/src/render_backend.rs:1523
7 XUL webrender::render_backend::RenderBackend::process_api_msg gfx/wr/webrender/src/render_backend.rs:1230
8 XUL std::sys_common::backtrace::__rust_begin_short_backtrace src/libstd/sys_common/backtrace.rs:130
9 XUL core::ops::function::FnOnce::call_once{{vtable.shim}} src/libcore/ops/function.rs:232

This is a macOS-specific use-after-free crash. The affected code seems to be platform-independent so I'm wary that it might be yet another instance of bug 1665411. I'm tentatively blocking that bug, feel free to revert in case this is something else.

Comment 1

[Gabriele Svelto [:gsvelto]]

Unfortunately this is the only crash which I thought was caused by bug 1665411 which had a report after that fix landed. I hope it's just a fluke otherwise it means that this is another issue. I guess we'll be sure when bug 1665411 gets uplifted.

Comment 2

[Glenn Watson [:gw]]

Attachment: Bug 1670575 - Use Vec instead of SmallVec for child tasks.

Unlikely to fix the reported crash, but doing this as a test to
see if it affects the crash signatures at all.

Comment 3

[Glenn Watson [:gw]]

We discussed during wr-triage meeting that we don't understand the crash frame here since it appears to be in what should be safe rust code. One possibility, if the stack trace is correct, is that there is a bug in the SmallVec crate that is used here. The attached patch changes that to a normal Vec - this is a very slight performance cost (some extra heap allocations) but will allow us to see if the crash is related to that.

Comment 4

[Gabriele Svelto [:gsvelto]]

This crash is macOS-only so it's more likely to be related to bug 1665411 especially given it's not happening on nightly where we landed a fix.

Comment 5

[Glenn Watson [:gw]]

Comment on attachment 9185059 [details]
Bug 1670575 - Use Vec instead of SmallVec for child tasks.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Unknown but seems very unlikely - this is a test patch to see if it makes the crash go away, since we have no other explanation for how it occurs right now.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which older supported branches are affected by this flaw?: Not applicable (see note above)
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: It might cause a very small performance regression on some pages. The intent here is to land this and see if the crash disappears - and then work out a better fix based on the outcome of this test.

Comment 6

[Glenn Watson [:gw]]

(In reply to Gabriele Svelto [:gsvelto] from comment #4)

This crash is macOS-only so it's more likely to be related to bug 1665411 especially given it's not happening on nightly where we landed a fix.

Do you think we should ignore it from a gfx perspective then and not worry about landing this patch to test if it's related?

Comment 7

[Gabriele Svelto [:gsvelto]]

I will be uplifting the patch for bug 1665411 to beta this week. I'd suggest waiting for a week after that happens to see if the crash disappears; if it does then you can be pretty sure it was bug 1665411.

Comment 8

[Tom Ritter [:tjr]]

Comment on attachment 9185059 [details]
Bug 1670575 - Use Vec instead of SmallVec for child tasks.

It sounds like we're going to wait on this for a little bit, so clearing the sec-approval flag - please re-request if ready to land.

Comment 9

[Daniel Veditz [:dveditz]]

Changing the dependency direction since bug 1665411 is a potential fix for this, and was able to land without being "blocked" by this one.

Comment 10

[Jim Mathies [:jimm]]

Hey Gabriele, any updates here?

Comment 11

[Gabriele Svelto [:gsvelto]]

This seems to be following the pattern I saw in other bugs related to bug 1665411. After landing the fix the issue is effectively solved for macOS 10.13 and earlier but still appears to affect 10.14+ though with lower volume. Bug 1676343 should solve it for good and I'm working on it.

Comment 12

[Jim Mathies [:jimm]]

Investigation in the parent is currently stalled.

Comment 13

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

The severity field for this bug is set to S4. However, the bug is flagged with the sec-high keyword.
:bhood, could you consider increasing the severity of this security bug?

For more information, please visit auto_nag documentation.

Comment 14

[(PTO June 19-July 7) Ryan VanderMeulen]

No recent reports from any modern versions.

Comment 15

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

Since the bug is closed, the stalled keyword is now meaningless.
For more information, please visit auto_nag documentation.