1946545 - Crash in [@ mozilla::layers::CanvasTranslator::AddBuffer]

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/b2615140-5247-4741-8558-b32560250206

MOZ_CRASH Reason:

MOZ_CRASH(mHeader->readerState == State::Paused)

Top 10 frames:

0  libxul.so  MOZ_CrashSequence(void*, long)  mfbt/Assertions.h:245
0  libxul.so  mozilla::layers::CanvasTranslator::AddBuffer(mozilla::UniquePtr<int, mozilla:...  gfx/layers/ipc/CanvasTranslator.cpp:282
1  libxul.so  mozilla::layers::CanvasTranslator::HandleCanvasTranslatorEvents()  gfx/layers/ipc/CanvasTranslator.cpp:743
2  libxul.so  mozilla::detail::RunnableMethodArguments<>::apply<nsObserverService, void (ns...  xpcom/threads/nsThreadUtils.h:1085
2  libxul.so  std::__invoke_impl<void, mozilla::detail::RunnableMethodArguments<>::apply<ns...  /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/c++/8/bits/invoke.h:60
2  libxul.so  std::__invoke<mozilla::detail::RunnableMethodArguments<>::apply<nsObserverSer...  /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/c++/8/bits/invoke.h:95
2  libxul.so  _ZSt12__apply_implIZN7mozilla6detail23RunnableMethodArgumentsIJEE5applyI17nsO...  /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/c++/8/tuple:1678
2  libxul.so  std::apply<mozilla::detail::RunnableMethodArguments<>::apply<nsObserverServic...  /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/c++/8/tuple:1687
2  libxul.so  mozilla::detail::RunnableMethodArguments<>::apply<nsObserverService, void (ns...  xpcom/threads/nsThreadUtils.h:1083
2  libxul.so  mozilla::detail::RunnableMethodImpl<mozilla::ScriptPreloader*, void (mozilla:...  xpcom/threads/nsThreadUtils.h:1134

Steady trickle of these crashes. I don't know if this is of interest or not.

Comment 1

[Lee Salzman [:lsalzman]]

Bob, any idea what's going on here?

Comment 2

[Bob Owen (:bobowen) (PTO until 5th Oct)]

(In reply to Lee Salzman [:lsalzman] from comment #1)

Bob, any idea what's going on here?

I haven't looked at canvas for a while and nearly all of these seem to be coming from CanvasTranslator::HandleCanvasTranslatorEvents, which I don't recognise.
It seems we did occasionally hit this before, but I'm not sure if HandleCanvasTranslatorEvents altered the state change logic somewhat.
Doesn't look like crash-stats has old enough data to try and pin-point anything.

Comment 3

[Sotaro Ikeda [:sotaro]]

The crash reports had the following error log.

|[G0][GFX1-]: CanvasTranslator::AddBuffer bad state 0

Then when the crashes happened, state was State::Processing. It needs to be State::Paused. The Paused state was added by Bug 1863914.

Comment 4

[Sotaro Ikeda [:sotaro]]

Bug 1899231 added a capability to interrupt TranslateRecording() in CanvasTranslator. The change was done as not to change order of the canvas tasks.

Comment 5

[Sotaro Ikeda [:sotaro]]

:bobowen, do you have any ideas about the problem?

Comment 6

[Bob Owen (:bobowen) (PTO until 5th Oct)]

(In reply to Sotaro Ikeda [:sotaro] from comment #3)

The crash reports had the following error log.

|[G0][GFX1-]: CanvasTranslator::AddBuffer bad state 0

Then when the crashes happened, state was State::Processing. It needs to be State::Paused. The Paused state was added by Bug 1863914.

I understand the immediate issue, it's just that the logic for starting and stopping translation has got a fair bit more complicated.
I've looked through the new code and I think possibly an issue in an original failure case could be the problem here.
The logic in HandleCanvasTranslatorEvents to maintain the ordering relies on TranslateRecording not returning false when the state is still Processing.

Looking at TranslateRecording I think this can only happen if ReadNextEvent returns false with a state of Processing.
I think ReadNextEvent can only return false with a state of Processing if either:

• mFlushCheckpoint is non-zero and HasPendingEvent returns false
• ReadPendingEvent returns false

In the HasPendingEvent case, as far as I can tell mFlushCheckpoint is only set from mHeader->eventCount so it must always be less than or equal to it.
So, it can only return false when mHeader->processedCount >= mFlushCheckpoint, so that would return true from logic in TranslateRecording.

In the ReadPendingEvent case that could return false if the read from the stream failed or the value wasn't a valid EventType. This should only happen if something has gone badly wrong in the content process, however in this case we don't set the state to Failed and I think we should.

I'll create a patch to do this and also address the places where we set Failed in TranslateRecording, to make them clearer and I think fix a separate issue. I'll also simplify the checking of mFlushCheckpoint, because I think we can just do it in one place in TranslateRecording.

Comment 7

[Bob Owen (:bobowen) (PTO until 5th Oct)]

https://treeherder.mozilla.org/jobs?repo=try&revision=916d1c803564f524f06b0a984dc91e158821b645

Comment 8

[Bob Owen (:bobowen) (PTO until 5th Oct)]

Attachment: Bug 1946545 p1: Always mark CanvasTranslator as Failed when the stream is bad. r=lsalzman!

Comment 9

[Bob Owen (:bobowen) (PTO until 5th Oct)]

Attachment: Bug 1946545 p2: Move logic not related to reading the next event out of ReadNextEvent. r=lsalzman!

This moves all the logic for exiting translation for a reason other than there
being no pending events out of ReadNextEvent and into TranslateRecording.
This now means mFlushCheckpoint logic is in one place along with pausing for
IPDL messages.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ca338e4d9d1
p1: Always mark CanvasTranslator as Failed when the stream is bad. r=lsalzman

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56415f74ab24
p2: Move logic not related to reading the next event out of ReadNextEvent. r=lsalzman

Comment 12

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/2ca338e4d9d1
https://hg.mozilla.org/mozilla-central/rev/56415f74ab24

Comment 13

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:bobowen, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox136 to wontfix.

For more information, please visit BugBot documentation.

Comment 14

[Bob Owen (:bobowen) (PTO until 5th Oct)]

(In reply to BugBot [:suhaib / :marco/ :calixte] from comment #13)

The patch landed in nightly and beta is affected.
:bobowen, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox136 to wontfix.

For more information, please visit BugBot documentation.

While the changes seem correct in themselves, this is a rather speculative fix to this crash.
So, I think just letting it ride the trains is the best option.