1478570 - Crash in mozilla::wr::RenderThread::UnregisterExternalImage

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is
report bp-813fbd77-3a0a-46ac-9776-9c08f0180726.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::wr::RenderThread::UnregisterExternalImage gfx/webrender_bindings/RenderThread.cpp:524
1 xul.dll void mozilla::layers::WebRenderTextureHostWrapper::~WebRenderTextureHostWrapper gfx/layers/wr/WebRenderTextureHostWrapper.cpp:28
2 xul.dll void std::deque<mozilla::layers::AsyncImagePipelineManager::ForwardingTextureHostWrapper, std::allocator<mozilla::layers::AsyncImagePipelineManager::ForwardingTextureHostWrapper> >::_Tidy vs2017_15.6.6/VC/include/deque:1928
3 xul.dll static void nsTHashtable<nsBaseHashtableET<nsUint64HashKey, nsAutoPtr<mozilla::layers::AsyncImagePipelineManager::PipelineTexturesHolder> > >::s_ClearEntry xpcom/ds/nsTHashtable.h:465
4 xul.dll mozilla::layers::AsyncImagePipelineManager::~AsyncImagePipelineManager gfx/layers/wr/AsyncImagePipelineManager.cpp:46
5 xul.dll void mozilla::layers::CompositorBridgeParent::StopAndClearResources gfx/layers/ipc/CompositorBridgeParent.cpp:524
6 xul.dll mozilla::layers::CompositorBridgeParent::RecvWillClose gfx/layers/ipc/CompositorBridgeParent.cpp:551
7 xul.dll mozilla::layers::PCompositorBridgeParent::OnMessageReceived ipc/ipdl/PCompositorBridgeParent.cpp:1187
8 xul.dll void mozilla::ipc::MessageChannel::DispatchMessageW ipc/glue/MessageChannel.cpp:2164
9 xul.dll mozilla::ipc::MessageChannel::MessageTask::Run ipc/glue/MessageChannel.cpp:2047

=============================================================

There is 1 crash in nightly 63 with buildid 20180725220116. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1477608.

[1] https://hg.mozilla.org/mozilla-central/rev?node=7bb705198ab3

Comment 1

[Sotaro Ikeda [:sotaro]]

Thanks, I take a look.

Comment 2

[Sotaro Ikeda [:sotaro]]

When "it == mRenderTextures.end()" happens, it seems to cause the crash. When I intentionally caused the situation by modifying the code, it caused the crash.

Comment 3

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #2)
> When "it == mRenderTextures.end()" happens, it seems to cause the crash.
> When I intentionally caused the situation by modifying the code, it caused
> the crash.

But I could not reproduce the crash with default m-c code.

Comment 4

[Sotaro Ikeda [:sotaro]]

Ah, a way of allocating ExternalImageIds for WebRenderTextureHostWrappers is bad. Each id is unique only within AsyncImagePipelineManager. But the id is not unique if there are multiple browser windows.

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix ExternalImageId allocation of AsyncImagePipelineManager

The patches uses "IdNamespace = 0". It is OK, since "IdNamespace = 0" is already used in AsyncImagePipelineManager. The patch just make the allocation is unique across AsyncImagePipelineManager instances.

And added error handling to RenderThread::UnregisterExternalImage() to avoid the crash.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

This seems to be the highest-frequency WR-specific crash over the last three days (that hasn't been fixed already).

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8995455 [details] [diff] [review]
patch - Fix ExternalImageId allocation of AsyncImagePipelineManager

Review of attachment 8995455 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/wr/AsyncImagePipelineManager.cpp
@@ +73,5 @@
>  
> +wr::ExternalImageId
> +AsyncImagePipelineManager::GetNextExternalImageId()
> +{
> +  static uint32_t sNextId = 0;

Should we be using a uint64_t here?

Comment 8

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> Comment on attachment 8995455 [details] [diff] [review]
> patch - Fix ExternalImageId allocation of AsyncImagePipelineManager
> > +wr::ExternalImageId
> > +AsyncImagePipelineManager::GetNextExternalImageId()
> > +{
> > +  static uint32_t sNextId = 0;
> 
> Should we be using a uint64_t here?

It is better to keep uint32_t, since gecko allocates external image id as (IdNamespace:32bit + ResourceId:32bit). And AsyncImagePipelineManager uses IdNamespace = 0. I am going to add comment about it.

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix ExternalImageId allocation of AsyncImagePipelineManager

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix ExternalImageId allocation of AsyncImagePipelineManager

Comment 11

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=943a283517a272d480743b78739ef4085137734c

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99cda6aef9e4
Fix ExternalImageId allocation of AsyncImagePipelineManager r=jrmuizel

Comment 13

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/99cda6aef9e4