1351114 - Crash in BasicDisplayItemLayer::Paint with outset box shadows enabled by default

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Description

[Mason Chang [Inactive] [:mchang]]

Attachment: Properly track display item layers in the basic layer manager.

See https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a9ea13620bb71ae7a8394b55c5d0deb2a17fc56

The crash was happening because now that we create an active layer for some display items, we sometimes also create a basic layer manager to paint those display items. When the basic layer manager paints the active display item, it'll create a display item layer and track it. [1] Once the transaction ends, the basic layer manager would nullify the display items in the basic display item layer and clear all references to the display item layer. [2]

However, we try to recycle display item layers. When we do that, we'd set pointers to the display items in the display item layer, BUT the basic layer manager would no longer be tracking the actual display item layer to clear later on. [3] Once a transaction finished, the display item layer would be pointing to invalid display items.

In this particular test case, we'll do one regular transaction and clear out all the display item layers. Then we'll reuse the display item layer, set the display items again, but because the basic layer manager wasn't tracking the display item layer, the results wouldn't be nulled out at the end of the transaction even though the display item died. Next, an empty transaction would come along and we'd try to paint the now dead display item.

This patch properly tracks reused display item layers with a basic layer manager.

The only question I have is, how come we'll try to paint the basic display item layer? If we don't need a display item layer for this particular transaction, why does the layer tree still have this layer? Does it take one extra transaction to remove the layer?


[1] http://searchfox.org/mozilla-central/source/gfx/layers/basic/BasicDisplayItemLayer.cpp#79
[2] http://searchfox.org/mozilla-central/source/gfx/layers/basic/BasicLayerManager.cpp#550
[3] http://searchfox.org/mozilla-central/source/layout/painting/nsDisplayList.cpp#2744

Comment 1

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8851826 [details] [diff] [review]
Properly track display item layers in the basic layer manager.

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

This seems reasonable, but I think TrackDisplayItemLayer should be part of generic LayerManager, not BasicLayerManager.

I'm not sure I entirely understand the last question. An empty transaction happens when we determine that the only changes required are a trivial update to a layer property (like setting a new image on an ImageLayer, or updating a transform), so we do that, skip the full DL/FLB cycle, and then proceeed to rendering the layer tree. The display item layer will still be a necessary part of this layer tree (just one that we determined to be unchanged) and we'll crash when trying to access it.

Comment 2

[Mason Chang [Inactive] [:mchang]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> Comment on attachment 8851826 [details] [diff] [review]
> Properly track display item layers in the basic layer manager.
> 
> Review of attachment 8851826 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems reasonable, but I think TrackDisplayItemLayer should be part of
> generic LayerManager, not BasicLayerManager.
> 
> I'm not sure I entirely understand the last question. An empty transaction
> happens when we determine that the only changes required are a trivial
> update to a layer property (like setting a new image on an ImageLayer, or
> updating a transform), so we do that, skip the full DL/FLB cycle, and then
> proceeed to rendering the layer tree. The display item layer will still be a
> necessary part of this layer tree (just one that we determined to be
> unchanged) and we'll crash when trying to access it.

In an empty transaction, is the layer tree is exactly the same? I guess that means we only create / change layers in a full DL/FLB cycle?

So that means in this particular case, we created a display item layer. The regular transaction ended and we deleted the display item, but still had dangling pointers to a dead display item. Once the empty transaction happened, the display item layer was never cleared out since that only happens with a full DL/FLB cycle, hence we tried to paint a dead display item.

Comment 3

[Matt Woodrow (:mattwoodrow)]

(In reply to Mason Chang [:mchang] from comment #2)
 
> In an empty transaction, is the layer tree is exactly the same? I guess that
> means we only create / change layers in a full DL/FLB cycle?
> 
> So that means in this particular case, we created a display item layer. The
> regular transaction ended and we deleted the display item, but still had
> dangling pointers to a dead display item. Once the empty transaction
> happened, the display item layer was never cleared out since that only
> happens with a full DL/FLB cycle, hence we tried to paint a dead display
> item.

Yes, exactly.

Comment 4

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

Crash-stats is showing a bunch of crashes that look like they're the same as this bug. Linux and Windows so far. The crash reports from today show "WR! WR+" in the app notes which means the user manually set the WR pref to on.

Comment 5

[Mason Chang [Inactive] [:mchang]]

Attachment: Properly track display item layers in layer managers

Comment 7

[Mason Chang [Inactive] [:mchang]]

Try looks good - https://treeherder.mozilla.org/#/jobs?repo=try&revision=35e2d8b5536b15c5177155d0f517ac3b47cf446d

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/bad38a8000a3
Properly track display item layers in layer managers. r=mattwoodrow

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/bad38a8000a3