Closed Bug 1250954 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Graphics: Layers, defect, critical)

Unspecified
Windows
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: milan, Assigned: ethlin)

References

Details

(Keywords: crash, Whiteboard: [gfx-noted])

Crash Data

Attachments

(1 file, 4 obsolete files)

This bug was filed from the Socorro interface and is 
report bp-c81dafbb-93fa-4c23-b5e4-190f02160224.
=============================================================
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?
Flags: needinfo?(mtseng)
See Also: → 1247611
Whiteboard: [gfx-noted]
Redirect to ethan since morris will take one week PTO from next week.
Flags: needinfo?(mtseng) → needinfo?(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.
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.
Flags: needinfo?(ethlin)
Attachment #8725169 - Flags: review?(jmuizelaar)
Attachment #8725169 - Flags: review?(jmuizelaar) → review?(jgilbert)
Assignee: nobody → ethlin
Ethan, does this also take care of bug 1247611?
Flags: needinfo?(ethlin)
I clear the front buffer too in Morph to prevent wrong TextureClient when changing factory.
Attachment #8725169 - Attachment is obsolete: true
Attachment #8725169 - Flags: review?(jgilbert)
Attachment #8726045 - Flags: review?(jgilbert)
(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.
Flags: needinfo?(ethlin)
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?
Attachment #8726045 - Flags: review?(jgilbert) → review-
(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?
(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
Flags: needinfo?(jgilbert)
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.
Flags: needinfo?(jgilbert)
Validate the buffers in GLScreenBuffer if the forwarder changes in ClientCanvasLayer::Initialize.
Attachment #8726045 - Attachment is obsolete: true
Attachment #8738542 - Flags: review?(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.
Attachment #8738542 - Flags: review?(jgilbert) → review-
I do the clone front buffer when the forwarder changed.
Attachment #8723462 - Attachment is obsolete: true
Attachment #8738542 - Attachment is obsolete: true
Attachment #8741236 - Flags: review?(jgilbert)
Attachment #8741236 - Flags: review?(jgilbert) → review+
Depends on: 1265638
Keywords: checkin-needed
This seems to have caused a rather large glterrain regression, see bug 1268302
Blocks: 1268302
No longer depends on: 1268302
https://hg.mozilla.org/mozilla-central/rev/18d290be0df2
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Duplicate of this bug: 1230394
You need to log in before you can comment on or make changes to this bug.