1436510 - Crash in PLDHashTable::Search | mozilla::FrameLayerBuilder::DrawPaintedLayer

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P3)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-6326b995-dfc6-4a67-afbe-0a7980180206.
=============================================================

Seen while looking at nightly crash stats: http://bit.ly/2sf3JB9. Seems to affect Windows and Linux only. 19 crashes/11 installs. Crashes started using 20180123102017.

Possible regression range based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5faab9e619901b1513fd4ca137747231be550def&tochange=c5461973d6ee7845b3f560c05e1502429fd63184

Top 10 frames of crashing thread:

0 xul.dll PLDHashTable::Search xpcom/ds/PLDHashTable.cpp:538
1 xul.dll mozilla::FrameLayerBuilder::DrawPaintedLayer layout/painting/FrameLayerBuilder.cpp:6112
2 xul.dll mozilla::layers::BasicPaintedLayer::PaintThebes gfx/layers/basic/BasicPaintedLayer.cpp:94
3 xul.dll mozilla::layers::BasicLayerManager::PaintSelfOrChildren gfx/layers/basic/BasicLayerManager.cpp:708
4 xul.dll mozilla::layers::BasicLayerManager::PaintLayer gfx/layers/basic/BasicLayerManager.cpp:891
5 xul.dll mozilla::layers::BasicLayerManager::PaintSelfOrChildren gfx/layers/basic/BasicLayerManager.cpp:731
6 xul.dll mozilla::layers::BasicLayerManager::PaintLayer gfx/layers/basic/BasicLayerManager.cpp:891
7 xul.dll mozilla::layers::BasicLayerManager::EndTransactionInternal gfx/layers/basic/BasicLayerManager.cpp:616
8 xul.dll RegularFramePaintCallback::Paint layout/svg/nsSVGIntegrationUtils.cpp:424
9 xul.dll nsFilterInstance::BuildSourceImage layout/svg/nsFilterInstance.cpp:489

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

Comment 1

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

Display list optimization in that potential regression range, but also no crashes in a few days.

Comment 2

[Matt Woodrow (:mattwoodrow)]

It looks like  aLayer->Manager()->GetLayerBuilder() is returning nullptr.

I can't see how that would happen though, except if we recursed into painting or similar.

Seems to be restricted to filters too.

Very low volume, so not super worried about this right now.

Comment 3

[Marcia Knous [:marcia]]

Fix-optional for 60 based on volume, per triage team.

Comment 5

[Julien Wajsberg [:julienw]]

Copying over the STR from bug 1441302:

Steps to reproduce:

1) Type in a file URL of an SVG file. The one that I used is:
   file:///C:/mozilla-central/toolkit/themes/shared/incontent-icons/blocked.svg
2) Right click anywhere on the page and click "Inspect Element"
3) Wait for about 2 seconds

Expected result:
  I can inspect the elements on the page.
Actual result:
  The content process crashes.

On my machine, I can reproduce this on remote URLs as well, but it only seems to happen after I have explore through the DOM tree a bit. On a different machine, it seems that remote URLs do not cause the same problem at all.




Note: I can't reproduce the issue myself (Linux / Nightly 20180226230123) :/

Comment 6

[Julien Wajsberg [:julienw]]

Matt, Marcia, looks like there were a lot more reports lately, according to crash-reports. You may want to bump up the priority.

Comment 7

[Marcia Knous [:marcia]]

Looks like in yesterday's build there were 35 crashes, overall over 100 in the last 7 days. I also changed the platform since the crashes are not Windows only on nightly.

Comment 8

[Matt Woodrow (:mattwoodrow)]

I managed to reproduce this consistently on https://www.usnews.com/news/best-states/rankings

The issue is that nsDisplayPerspective is made unique by adding an index that increments each time we create one.

When doing a partial build with retained-dl, we iterate a subset, and the indexes no longer line up.

We should just give up on partial builds with perspective, until bug 1431249 fixes the underlying sadness.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Rebuild all items within perspective

Comment 10

[Miko Mynttinen]

Comment on attachment 8954597 [details] [diff] [review]
Rebuild all items within perspective

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

LGTM.

::: layout/generic/nsFrame.cpp
@@ +2827,5 @@
>        aBuilder->MarkFrameModifiedDuringBuilding(this);
>      }
>    }
>  
> +  if (aBuilder->IsRetainingDisplayList() &&

Could you add a comment about why we are doing this?
And maybe a reference to bug 1431249 with the hope that this code is some day removed.

Comment 11

[Miko Mynttinen]

Comment on attachment 8954597 [details] [diff] [review]
Rebuild all items within perspective

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

LGTM.

::: layout/generic/nsFrame.cpp
@@ +2827,5 @@
>        aBuilder->MarkFrameModifiedDuringBuilding(this);
>      }
>    }
>  
> +  if (aBuilder->IsRetainingDisplayList() &&

Could you add a comment about why we are doing this?
And maybe a reference to bug 1431249 with the hope that this code is some day removed.

@@ +2828,5 @@
>      }
>    }
>  
> +  if (aBuilder->IsRetainingDisplayList() &&
> +      ChildrenHavePerspective()) {

Please use ChildrenHavePerspective(disp) here to avoid unnecessary StyleDisplay() call.

Comment 12

[Miko Mynttinen]

Just for reference, I think that after this patch, the changes in bug 1416991 are not needed anymore.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1fe29595995
Don't attempt partial display list building within perspective as we need to build all children to get consistent indices. r=miko

Comment 14

[Cristina Coroiu [:ccoroiu]]

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

Comment 15

[Ionuț Goldan [:igoldan]]

This brought in some performance improvements:

== Change summary for alert #11877 (as of Thu, 01 Mar 2018 01:54:12 GMT) ==

Improvements:

  3%  displaylist_mutate windows10-64 pgo e10s stylo     6,744.11 -> 6,547.69
  2%  displaylist_mutate linux64 pgo e10s stylo          3,077.69 -> 3,015.95

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11877