Closed Bug 1633791 Opened 5 months ago Closed 4 months ago

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

Categories

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

All
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla78
Tracking Status
firefox-esr68 --- unaffected
firefox75 --- unaffected
firefox76 --- unaffected
firefox77 --- disabled
firefox78 --- fixed

People

(Reporter: bobowen, Assigned: bobowen)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(3 files, 11 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

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

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?

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

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(lsalzman)
Flags: needinfo?(lsalzman)
Priority: -- → P1

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.

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

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

Depends on D75508

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

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

Depends on D75510

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

Depends on D75511

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

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

Depends on D75519

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.

Attachment #9149316 - Attachment is obsolete: true

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.

Attachment #9149317 - Attachment is obsolete: true
Attachment #9149318 - Attachment is obsolete: true
Attachment #9149319 - Attachment is obsolete: true
Attachment #9149320 - Attachment is obsolete: true
Attachment #9149321 - Attachment is obsolete: true
Attachment #9149322 - Attachment is obsolete: true
Attachment #9149324 - Attachment is obsolete: true
Attachment #9149325 - Attachment is obsolete: true
Attachment #9149326 - Attachment is obsolete: true
Attachment #9149328 - Attachment is obsolete: true
Pushed by bobowencode@gmail.com:
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
You need to log in before you can comment on or make changes to this bug.