Closed Bug 1387994 Opened 3 years ago Closed 3 years ago

Crash in mozilla::layers::CopyableCanvasRenderer::Initialize

Categories

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

57 Branch
Unspecified
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed

People

(Reporter: calixte, Assigned: ethlin)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, Whiteboard: [clouseau])

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-4c073947-8b4d-4127-9f6d-051a30170805.
=============================================================

There are 5 crashes (from the same installation) in nightly 57 with buildid 20170805100334. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1379920.

[1] https://hg.mozilla.org/mozilla-central/rev?node=d5d815d3ac0b8d4cbb1eb5eb12f0681940c5d5da
Flags: needinfo?(mtseng)
I can check this.
Assignee: nobody → mtseng
Flags: needinfo?(mtseng)
1 user / 5 crashes. not blocking anything at this point.
I can reproduce it. I'll have a patch for it.
Assignee: mtseng → ethlin
Blocks: 1389000
Comment on attachment 8895694 [details]
Bug 1387994 - Ensure the draw target when initializing the canvas context.

https://reviewboard.mozilla.org/r/166978/#review172208

::: dom/canvas/CanvasRenderingContext2D.cpp:6305
(Diff revision 1)
>    data.mPreTransCallback = CanvasRenderingContext2DUserData::PreTransactionCallback;
>    data.mPreTransCallbackData = this;
>    data.mDidTransCallback = CanvasRenderingContext2DUserData::DidTransactionCallback;
>    data.mDidTransCallbackData = this;
>  
> +  EnsureTarget();

It is odd that we don't have a buffer provider by the time we get to display list building. Do you have an idea why this happens?

I am a bit worried about doing this. EnsureTarget will lock the draw target and its underlying shared memory if any, potentially holding on to a mutex, so if we are not very careful about when this will be unlocked, it's easy to dead-lock. In general we unlock in the stable-state callback (which is called shortly after we get out of the javascript event loop. I am not certain whether it is before or after layout, and if it is after layout, we would have to make sure it is called even if there was no JS invoked (I assume it is not the case).

one work around would be to test if the buffer provider is null, only EnsureTarget if it is teh case, and immediately after do ReturnTarget.
But I would rather first understand how we have this canvas without a buffer provider. Because just calling EnsureTarget here may mean we don't have the information necessary to create the appropriate one and just silently fall back to the default one that can't render the canvas in shared memory and performs more copy.
From my understanding, the canvas context may clear the buffer provide when changing dimension (will call CanvasRenderingContext2D::Reset). So it's possible there is no buffer provider when building layer. For layers mode, the layer building code path looks like:

nsDisplayCanvas::BuildLayer
  - nsHTMLCanvasFrame::BuildLayer
    - CanvasRenderingContext2D::GetCanvasLayer
      - CanvasRenderingContext2D::InitializeCanvasRenderer

We call EnsureTarget in CanvasRenderingContext2D::GetCanvasLayer[1]. So we always have buffer provider in InitializeCanvasRenderer().

For layers-free mode, we call InitializeCanvasRenderer() in nsDisplayCanvas::CreateWebRenderCommands[2] directly. That's why we have this bug. Any better way to fix it?


[1] https://dxr.mozilla.org/mozilla-central/rev/5322c03f4c8587fe526172d3f87160031faa6d75/dom/canvas/CanvasRenderingContext2D.cpp#6218
[2] https://dxr.mozilla.org/mozilla-central/rev/5322c03f4c8587fe526172d3f87160031faa6d75/layout/generic/nsHTMLCanvasFrame.cpp#144
> From my understanding, the canvas context may clear the buffer provide when changing dimension

Ok that's fair. It should not be common but it can happen.


> We call EnsureTarget in CanvasRenderingContext2D::GetCanvasLayer[1]. So we always have buffer provider in InitializeCanvasRenderer().

In this case we actually only call EnsureTarget if either we are using an opaque canvas (because we need to paint the canvas in black if it is empty), or if we are using skiagl, because the skiagl code doesn't seem to support empty textures. But as it turns out we also try to avoid calling EnsureTarget just a few lines below if we don't need to in order to not allocate memory unnecessarily and avoid creating a layer manager during paint which I am not sure I understand.

I take back what I said about the dead-lock. EnsureTarget schedules the stable state callback so as long as we don't try to deallocate the texture before the callback happens there should not be deadlocks (it's not very reassuring but not as bad as I thought).

I looked through the code a bit and if I understand correctly right now the canvas context never uses a shared buffer provider if webrender is enabled (because it only does so if there is a LayerManager available and it uses the LAYERS_CLIENT backend). We can fix that later, so we are left with the issue of allocating extra memory. Ideally EnsureTarget is called at the beginning of the frame just before painting. Under the hood we sometimes do a copy-on-write trick to avoid reallocating the canvas's texture and calling EnsureTarget just after painting pretty much undoes that optimization.

> Any better way to fix it?

At the very least we should not call EnsureTarget if we don't need to.
If mBufferProvider is not null, no need to call it for example (should be the most common case).

I propose doing this:

> if (!mBufferProvider) {
>  // Force the creation of a buffer provider.
>  EnsureTarget();
>  returnTarget();
> }

That way we avoid holding the lock longer than we need to but more importantly we don't mess the copy-on-write optimization up.

Long term we could avoid that by either not creating the display item if the canvas is completely transparent (that's assuming we will re-build the display list the next frame there is something in the canvas, I am not sure if that's what we want), or create the display item and pass it a dummy 2x2 transparent or black image so that the next frame can just call update_image but without allocating the full area of the canvas to show nothing.
Thanks! I will update the patch according to the the feedback.
:nical, review ping!
Flags: needinfo?(nical.bugzilla)
This is the #3 Mac topcrash in Nightly 20170818100226, with 10 occurrences across 3 installations.
Comment on attachment 8895694 [details]
Bug 1387994 - Ensure the draw target when initializing the canvas context.

https://reviewboard.mozilla.org/r/166978/#review175844
Attachment #8895694 - Flags: review?(nical.bugzilla) → review+
Flags: needinfo?(nical.bugzilla)
Pushed by ethlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d04c6309b748
Ensure the draw target when initializing the canvas context. r=nical
https://hg.mozilla.org/mozilla-central/rev/d04c6309b748
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.