1633791 - Crash in [@ IPCError-browser | ShutDownKill | mozilla::CrossProcessSemaphore::Wait]

Status: RESOLVED FIXED

(Core :: Canvas: 2D, defect, P1)

Description

[Bob Owen (:bobowen)]

This appears to be happening when a content process is shutting down and the CanvasTranslator has stopped processing, but hasn't closed the IPC channel or the close hasn't registered in the content process.

This bug is for crash report bp-66d83d8d-663f-46f8-b8dd-859130200427.

Top 10 frames of crashing thread:

0 ntdll.dll NtWaitForSingleObject 
1 kernelbase.dll WaitForSingleObjectEx 
2 xul.dll mozilla::CrossProcessSemaphore::Wait ipc/glue/CrossProcessSemaphore_windows.cpp:58
3 xul.dll mozilla::layers::CanvasEventRingBuffer::WaitForReadCount gfx/layers/CanvasDrawEventRecorder.cpp:398
4 xul.dll mozilla::layers::CanvasChild::GetDataSurface gfx/layers/ipc/CanvasChild.cpp:271
5 xul.dll mozilla::layers::SourceSurfaceCanvasRecording::EnsureDataSurfaceOnMainThread gfx/layers/ipc/CanvasChild.cpp:79
6 xul.dll mozilla::layers::SourceSurfaceCanvasRecording::GetDataSurface gfx/layers/ipc/CanvasChild.cpp:68
7 xul.dll mozilla::dom::CanvasRenderingContext2D::GetImageDataArray dom/canvas/CanvasRenderingContext2D.cpp:5076
8 xul.dll mozilla::dom::CanvasRenderingContext2D::GetImageData dom/canvas/CanvasRenderingContext2D.cpp:5019
9 xul.dll mozilla::dom::CanvasRenderingContext2D_Binding::getImageData dom/bindings/CanvasRenderingContext2DBinding.cpp:4334

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Notes from brainstorming with jld:

• Ask the ContentChild singleton if it's shutting down? (Make sure it's the right kind of “shutting down”; there are several stages.)
• Except that won't work if the content process main thread didn't handle the shutdown message because it's blocked here.
• Is this required to be blocking for Web standards?

Comment 2

[Release mgmt bot [:sylvestre / :calixte / :marco for bugbug]]

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

Comment 3

[Release mgmt bot [:sylvestre / :calixte / :marco for bugbug]]

The priority flag is not set for this bug.
:lsalzman, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[Bob Owen (:bobowen)]

I think these are probably mainly existing crashes, where the canvas is causing the GPU to hang/be slow and people close the tab.
Before remote canvas, the ShutdownKills would occur in various places in Direct2D and the drivers.
With remote canvas, they are all concentrated in a few places ... mainly this one.

The good thing is because of the async nature now and as this is our code, we can do something about the ShutdownKill at least.
Once I'd done that, my example hang then crashed the GPU because the stream was bad and we didn't handle it.
So I've also gone through to add checks for sizes being read properly before using them for allocation and made the allocation all fallible, so we hopefully handle these cases more gracefully.

Comment 5

[Bob Owen (:bobowen)]

Try push where I had forgotten to lock the monitor before sending:
https://treeherder.mozilla.org/#/jobs?repo=try&duplicate_jobs=visible&selectedTaskRun=ATV09-PAQ1iVpwG39z3NUw-0&revision=3ffaa7044dfb74069b62b9abb6768fe6752e07f3

New try push with the lock added:
https://treeherder.mozilla.org/#/jobs?repo=try&duplicate_jobs=visible&selectedTaskRun=duI6iv8lRmGdJeH1XwMn_A-0&revision=bcfbef3dc68778dc5ed3cc33261958c6ed926e5c

Comment 6

[Bob Owen (:bobowen)]

Attachment: Bug 1633791 part 1: Add ability to notify the child processes of an impending shutdown. r=nika!

This also adds a call to the new function in ContentParent::StartForceKillTimer.

Comment 7

[Bob Owen (:bobowen)]

Attachment: Bug 1633791 part 2: Use ExpectingShutdown to prevent ShutDownKill for remote canvas. r=jrmuizel!

Depends on D75507

Comment 8

[Bob Owen (:bobowen)]

Attachment: Bug 1633791 part 3: Add MakeUniqueForOverwrite and MakeUniqueForOverwriteFallible helpers. r=froydnj!

These are the equivalent of std::make_unique_for_overwrite and std::make_unique_for_overwrite with fallible allocation.

Depends on D75508

Comment 9

[Bob Owen (:bobowen)]

Attachment: Bug 1633791 part 4: Add Buffer::AllocForOverwrite to fallibly create a default-initialized Buffer. r=froydnj!

This also uses MakeUniqueForOverwrite* in two places where we immediately copy
over the Buffer from a Span.
Adds move assignment operator for use in a later patch as well.

Depends on D75509

Comment 10

[Bob Owen (:bobowen)]

Attachment: Bug 1633791 part 5: Make mPayload allocation in RecordedFilterNodeSetAttribute fallible. r=jrmuizel!

This is done by changing the vector to a Buffer and using its fallible methods.

Depends on D75510

Comment 11

[Bob Owen (:bobowen)]

Attachment: Bug 1633791 part 6: Make mDashPatternStorage allocation for StrokeOptions fallible. r=jrmuizel!

This adds an OwningStrokeOptions, which has a Buffer so that it can own its dash
pattern storage and allocate it fallibly.

Depends on D75511

Comment 12

[Bob Owen (:bobowen)]

Attachment: Bug 1633791 part 7: Make mData allocation in RecordedFontDescriptor fallible. r=jrmuizel!

Depends on D75512

Comment 13

[Bob Owen (:bobowen)]

Attachment: Bug 1633791 part 8: Make mInstanceData allocation in RecordedUnscaledFontCreation fallible. r=jrmuizel!

Depends on D75513

Comment 14

[Bob Owen (:bobowen)]

Attachment: Bug 1633791 part 9: Make mInstanceData and Variations allocation in RecordedScaledFontCreation fallible. r=jrmuizel!

Depends on D75514

Comment 15

[Bob Owen (:bobowen)]

Attachment: Bug 1633791 part 10: Make mGlyphs allocation in RecordedFillGlyphs fallible. r=jrmuizel!

Depends on D75515

Comment 16

[Bob Owen (:bobowen)]

Attachment: Bug 1633791 part 11: Make mStops allocation in RecordedGradientStopsCreation fallible. r=jrmuizel!

Depends on D75517

Comment 17

[Bob Owen (:bobowen)]

Attachment: Bug 1633791 part 12: Add remaining checks that sizes have been read correctly. r=jrmuizel!

This adds checks that we believe the size has been read correctly, because the
stream is still good, to RecordedSourceSurfaceCreation and RecordedFontData
where allocation was already fallible.

Depends on D75518

Comment 18

[Bob Owen (:bobowen)]

Attachment: Bug 1633791 part 13: Change mPathData in PathOps from vector to MemStream. r=jrmuizel!

This means that allocation for the path data is now fallible.

Depends on D75519

Comment 19

[Phabricator Automation]

Comment on attachment 9149316 [details]
Bug 1633791 part 3: Add MakeUniqueForOverwrite and MakeUniqueForOverwriteFallible helpers. r=froydnj!

Revision D75509 was moved to bug 1639958. Setting attachment 9149316 [details] to obsolete.

Comment 20

[Phabricator Automation]

Comment on attachment 9149317 [details]
Bug 1633791 part 4: Add Buffer::AllocForOverwrite to fallibly create a default-initialized Buffer. r=froydnj!

Revision D75510 was moved to bug 1639958. Setting attachment 9149317 [details] to obsolete.

Comment 21

[Bob Owen (:bobowen)]

Attachment: Bug 1633791 part 3: Check that sizes have been read correctly from stream before allocation. r=jrmuizel!

Depends on D75508

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e8d16c595f5
part 1: Add ability to notify the child processes of an impending shutdown. r=nika
https://hg.mozilla.org/integration/autoland/rev/d457d4a5b0b1
part 2: Use ExpectingShutdown to prevent ShutDownKill for remote canvas. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/151f9f0eaada
part 3: Check that sizes have been read correctly from stream before allocation. r=jrmuizel

Comment 23

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/1e8d16c595f5
https://hg.mozilla.org/mozilla-central/rev/d457d4a5b0b1
https://hg.mozilla.org/mozilla-central/rev/151f9f0eaada