1617523 - Crash in [@ mozilla::widget::remote_backbuffer::Client::BorrowDrawTarget]

Status: RESOLVED FIXED

(Core :: Graphics, defect, P2)

Description

[Gabriele Svelto [:gsvelto] (PTO)]

This bug is for crash report bp-da5524a3-560b-4924-90f4-14fed0200221.

Top 10 frames of crashing thread:

0 xul.dll mozilla::widget::remote_backbuffer::Client::BorrowDrawTarget widget/windows/RemoteBackbuffer.cpp:555
1 xul.dll mozilla::widget::CompositorWidgetParent::StartRemoteDrawing widget/windows/CompositorWidgetParent.cpp:82
2 xul.dll mozilla::gl::TextureImage::GetSrcTileRect gfx/gl/GLTextureImage.cpp:82
3 xul.dll mozilla::layers::BasicCompositor::BeginFrameForWindow gfx/layers/basic/BasicCompositor.cpp:899
4 xul.dll mozilla::layers::LayerManagerComposite::Render gfx/layers/composite/LayerManagerComposite.cpp:1153
5 xul.dll mozilla::layers::LayerManagerComposite::UpdateAndRender gfx/layers/composite/LayerManagerComposite.cpp:645
6 xul.dll mozilla::layers::LayerManagerComposite::EndTransaction gfx/layers/composite/LayerManagerComposite.cpp:564
7 xul.dll mozilla::layers::CompositorBridgeParent::CompositeToTarget gfx/layers/ipc/CompositorBridgeParent.cpp:1047
8 xul.dll mozilla::layers::CompositorVsyncScheduler::Composite gfx/layers/ipc/CompositorVsyncScheduler.cpp:250
9 xul.dll mozilla::detail::RunnableMethodImpl<mozilla::layers::CompositorVsyncScheduler*, void  xpcom/threads/nsThreadUtils.h:1212

New crash, first started in buildid 20200213214257. GPU-process only.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Probably caused by bug 1604412

Comment 2

[Chris Martin [:cmartin]]

It looks like the issue is that this code allows mBackbuffer to retain a non-null value even if Initialize() or CreateRemoteFileMapping() fail. The next time it is called, it will see that it already has mBackbuffer and will just return BorrowSameBuffer to Client::BorrowDrawTarget().

void Provider::HandleBorrowRequest(BorrowResponseData* aResponseData) {
  // ...

  bool needNewBackbuffer = !mBackbuffer || (mBackbuffer->GetWidth() != width) ||
                           (mBackbuffer->GetHeight() != height);

  if (!needNewBackbuffer) {
    aResponseData->result = ResponseResult::BorrowSameBuffer;
    return;
  }

  mBackbuffer.reset();

  mBackbuffer = std::make_unique<PresentableSharedImage>();
  if (!mBackbuffer->Initialize(width, height)) {
    return;
  }

  HANDLE remoteFileMapping =
      mBackbuffer->CreateRemoteFileMapping(mTargetProcessId);
  if (!remoteFileMapping) {
    return;
  }

  aResponseData->result = ResponseResult::BorrowSuccess;
  aResponseData->width = width;
  aResponseData->height = height;
  aResponseData->fileMapping = remoteFileMapping;
}

So Client will attempt to create a DrawTarget for its current mBackbuffer, but mBackbuffer==nullptr because the first call failed:

The fix is to defer setting mBackbuffer until both Initialize() and CreateRemoteFileMapping() succeed and we can be sure the Client has had the opportunity to store the backbuffer.

This has also made me realize that the Client doesn't properly handle the case where it gets a BorrowSuccess response from the Provider, but then fails to InitializeRemote() using the info (very unlikely to happen, but technically possible).

In that case, it may keep getting BorrowSameBuffer responses from the Provider, but that response is meaningless if the Client was never able to initialize the buffer in the first place.

I have added a new message to explicitly say that buffer re-use is allowed. Otherwise, Provider will always assume that a new buffer is needed.

Comment 3

[Chris Martin [:cmartin]]

Attachment: Bug 1617523 - Ensure remote backbuffer only reused if initialized properly

This crash was caused because there was a case where the Provider would
store the SharedImage even if it failed to initialize it or send it to the
Client.

When the Client next requested a borrow, the Provider would see it was
already storing a SharedImage and would tell the Client to just
reuse the one it already had.

Since the Client never actually received it, it would end up dereferencing
a null pointer.

The fix for this is to wait until the SharedImage is fully initialized and
shared with the Client before storing it. That way, we know that if we have
it then so does the Client.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/992f5734eb4e
Ensure remote backbuffer only reused if initialized properly r=jrmuizel

Comment 5

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/992f5734eb4e