1870681 - Crash in [@ mozilla::gfx::MemWriter::write]

Status: RESOLVED FIXED

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

Description

[Andrew Osmond [:aosmond] (he/him)]

Crash report: https://crash-stats.mozilla.org/report/index/d10d3586-99cd-42bc-8d64-8a6620231218

Reason: EXCEPTION_ACCESS_VIOLATION_WRITE

Top 10 frames of crashing thread:

0  xul.dll  mozilla::gfx::MemWriter::write  gfx/2d/RecordedEvent.h:219
0  xul.dll  mozilla::gfx::ElementStreamFormat<mozilla::gfx::MemWriter, mozilla::gfx::RecordedEvent::EventType>::Write  gfx/2d/RecordingTypes.h:21
0  xul.dll  mozilla::gfx::WriteElement  gfx/2d/RecordingTypes.h:52
0  xul.dll  mozilla::layers::CanvasDrawEventRecorder::WriteInternalEvent  gfx/layers/CanvasDrawEventRecorder.cpp:150
0  xul.dll  mozilla::layers::CanvasDrawEventRecorder::DropFreeBuffers  gfx/layers/CanvasDrawEventRecorder.cpp:234
1  xul.dll  mozilla::detail::RunnableMethodArguments<>::apply<nsMemoryReporterManager, nsresult  const  xpcom/threads/nsThreadUtils.h:1164
1  xul.dll  std::invoke  /builds/worker/fetches/vs/VC/Tools/MSVC/14.29.30133/include/type_traits:1524
1  xul.dll  std::_Apply_impl  /builds/worker/fetches/vs/VC/Tools/MSVC/14.29.30133/include/tuple:974
1  xul.dll  std::apply  /builds/worker/fetches/vs/VC/Tools/MSVC/14.29.30133/include/tuple:979
1  xul.dll  mozilla::detail::RunnableMethodArguments<>::apply  xpcom/threads/nsThreadUtils.h:1162

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

Bug 1869822 landed but we still see this crash signature in nightly.

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

Probably the null pointer is MemWriter::mPtr:
https://searchfox.org/mozilla-central/rev/91cc8848427fdbbeb324e6ca56a0d08d32d3c308/gfx/2d/RecordedEvent.h#219

And mCurrentBuffer.Writer() does still exist:
https://searchfox.org/mozilla-central/rev/91cc8848427fdbbeb324e6ca56a0d08d32d3c308/gfx/layers/CanvasDrawEventRecorder.cpp#150

We know we can initialize it with a nullptr though:
https://searchfox.org/mozilla-central/rev/91cc8848427fdbbeb324e6ca56a0d08d32d3c308/gfx/2d/RecordedEvent.h#252

As used here:
https://searchfox.org/mozilla-central/rev/91cc8848427fdbbeb324e6ca56a0d08d32d3c308/gfx/layers/CanvasDrawEventRecorder.cpp#217
https://searchfox.org/mozilla-central/rev/91cc8848427fdbbeb324e6ca56a0d08d32d3c308/gfx/layers/CanvasDrawEventRecorder.cpp#223

DropFreeBuffers which triggers all of this could conceivably happen after one of the edge cases above.

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

This is probably a consequence of bug 1869658 actually, just kicking the crash further down the line.

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

If we have no buffer, we have no need to write the drop event, since nothing is supposed to be waiting on it:
https://searchfox.org/mozilla-central/rev/91cc8848427fdbbeb324e6ca56a0d08d32d3c308/gfx/layers/CanvasDrawEventRecorder.cpp#234

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

The other path of using WriteInternalEvent is guarded checking for the sanity of the buffer:
https://searchfox.org/mozilla-central/rev/91cc8848427fdbbeb324e6ca56a0d08d32d3c308/gfx/layers/CanvasDrawEventRecorder.cpp#156

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1870681 - Check for invalid buffers when clearing a canvas recorder's cached buffer pool.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/159cab18c3a7
Check for invalid buffers when clearing a canvas recorder's cached buffer pool. r=gfx-reviewers,lsalzman

Comment 8

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/159cab18c3a7

Comment 9

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

Set release status flags based on info from the regressing bug 1863914

Comment 10

[Donal Meehan [:dmeehan]]

:aosmond could you nominate this for a beta uplift?

Comment 11

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9369266 [details]
Bug 1870681 - Check for invalid buffers when clearing a canvas recorder's cached buffer pool.

Beta/Release Uplift Approval Request

• User impact if declined: Moderate volume content process crash
• Is this code covered by automated tests?: Yes
• 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): Trivial change to avoid writing events to invalid buffers
• String changes made/needed:
• Is Android affected?: No

Comment 12

[Donal Meehan [:dmeehan]]

Comment on attachment 9369266 [details]
Bug 1870681 - Check for invalid buffers when clearing a canvas recorder's cached buffer pool.

Approved for 122.0b6

Comment 13

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/bd328c3551c5