1250954 - crash in mozilla::layers::TextureClient::WaitForCompositorRecycle | mozilla::layers::CanvasClientSharedSurface::Updated

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

This bug was filed from the Socorro interface and is 
report bp-c81dafbb-93fa-4c23-b5e4-190f02160224.
=============================================================

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

This was the workflow:

Single window Firefox, multiple tabs.
Open http://webglsamples.googlecode.com/hg/aquarium/aquarium.html in a new tab.
Drag that tab out to a separate window.
Drag the tab back to the original window.

Bug 1247611 has perhaps related problems with dragging WebGL tab to separate window, but different workflow.

Morris, any ideas?

Comment 2

[Peter Chang[:pchang]]

Redirect to ethan since morris will take one week PTO from next week.

Comment 3

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Filter out different forwarder when switching CanvasClient.

I'm not sure this patch would help.

Comment 4

[Ethan Lin[:ethlin]]

When using debug build on mac platform, I can get the following assertion when drag the webgl tab out.

    frame #0: 0x00000001037bef07 XUL`mozilla::layers::TextureClient::InitIPDLActor(this=0x000000013183c740, aForwarder=0x0000000154f89de0) + 519 at TextureClient.cpp:687
   684 	  if (mActor && !mActor->mDestroyed && mActor->GetForwarder() == aForwarder) {
   685 	    return true;
   686 	  }
-> 687 	  MOZ_ASSERT(!mActor || mActor->mDestroyed, "Cannot use a texture on several IPC channels.");

Morris's patch should be related. I will do further study about the problem.

Comment 5

[Ethan Lin[:ethlin]]

Attachment: correct textureclient's allocator.

When we set a new factory to GLScreenBuffer, the back buffer's allocator may be different from the factory's. I do the resize in Morph to ensure the allocator is correct.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Ethan, does this also take care of bug 1247611?

Comment 7

[Ethan Lin[:ethlin]]

Attachment: correct textureclient's allocator.

I clear the front buffer too in Morph to prevent wrong TextureClient when changing factory.

Comment 8

[Ethan Lin[:ethlin]]

(In reply to Milan Sreckovic [:milan] from comment #6)
> Ethan, does this also take care of bug 1247611?

No, this only fix the crash problem with e10s disabled.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8726045 [details] [diff] [review]
correct textureclient's allocator.

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

I think we need to think about this more.

::: gfx/gl/GLScreenBuffer.cpp
@@ +456,5 @@
>  {
>      MOZ_ASSERT(newFactory);
>      mFactory = Move(newFactory);
> +
> +    mFront = nullptr;

Why do you clear front? I don't think we want this to happen here.

@@ +458,5 @@
>      mFactory = Move(newFactory);
> +
> +    mFront = nullptr;
> +    if (mBack && mBack->GetAllocator() != mFactory->mAllocator) {
> +        Resize(mBack->Surf()->mSize);

This isn't the original semantics of Morph, so it's hard to tell if this is OK without checking callsites.

Do we need to change the semantics?

Comment 10

[Kelsey Gilbert [:jgilbert]]

(In reply to Ethan Lin[:ethlin] from comment #5)
> Created attachment 8725169 [details] [diff] [review]
> correct textureclient's allocator.
> 
> When we set a new factory to GLScreenBuffer, the back buffer's allocator may
> be different from the factory's. I do the resize in Morph to ensure the
> allocator is correct.

Why is a mismatched allocator bad? GLScreenBuffer is currently designed assuming that mismatched allocators are OK. When and why are they not?


(In reply to Ethan Lin[:ethlin] from comment #7)
> Created attachment 8726045 [details] [diff] [review]
> correct textureclient's allocator.
> 
> I clear the front buffer too in Morph to prevent wrong TextureClient when
> changing factory.

Why?

Comment 11

[Ethan Lin[:ethlin]]

(In reply to Jeff Gilbert [:jgilbert] from comment #10)
> (In reply to Ethan Lin[:ethlin] from comment #5)
> > Created attachment 8725169 [details] [diff] [review]
> > correct textureclient's allocator.
> > 
> > When we set a new factory to GLScreenBuffer, the back buffer's allocator may
> > be different from the factory's. I do the resize in Morph to ensure the
> > allocator is correct.
> 
> Why is a mismatched allocator bad? GLScreenBuffer is currently designed
> assuming that mismatched allocators are OK. When and why are they not?
> 

The CanvasClient will try to do TextureClient::InitIPDLActor when updating. InitIPDLActor[1] checks if the forwarders are the same or we will get the assertion if the mActor exits. So I got the assertions when I dragged the tab to another window because the mismatched allocators. Any suggestions?
 
[1] https://hg.mozilla.org/mozilla-central/annotate/4657041c6f77b36b1fb9647c28f53f4f51757360/gfx/layers/client/TextureClient.cpp#l701

Comment 12

[Ethan Lin[:ethlin]]

Per discussion with nical, each window has it's own LayerManager and Forwarder. When dragging a tab to another window, the forwarder of the tab will change. The buffer kept in GLScreenBuffer should invalidate at this time since we reuse the GLScreenBuffer. I will upload another WIP for this problem.

Comment 13

[Ethan Lin[:ethlin]]

Attachment: Check buffer forwarder when initializing layer.

Validate the buffers in GLScreenBuffer if the forwarder changes in ClientCanvasLayer::Initialize.

Comment 14

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8738542 [details] [diff] [review]
Check buffer forwarder when initializing layer.

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

Actually, I don't think we need to change Morph.

Ultimately, all we need to ensure is that mFront in CanvasClientSharedSurface is never incompatible with the layer manager.
(compatible might be a surface from the new factory, or it might instead just be a 'basic' surface, to be uploaded by the compositor)

Therefore, all we should need to do is CloneSurface any time we have an incompatible GLScreenBuffer::mFront into some layer-manager-compatible surface, which we can then use in CanvasClientSharedSurface.

Comment 15

[Ethan Lin[:ethlin]]

Attachment: Correct front buffer when forwarder changed.

I do the clone front buffer when the forwarder changed.

Comment 16

[Ethan Lin[:ethlin]]

try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d42d4c242778

Comment 17

[Ethan Lin[:ethlin]]

try server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9de464cbba64

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/18d290be0df2

Comment 19

[William Lachance (:wlach)]

This seems to have caused a rather large glterrain regression, see bug 1268302

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/18d290be0df2