1214212 - Crash in mozilla::layers::InstallLayerClipPreserves3D when loading the THREE.CSS3DRenderer demo

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

+++ This bug was initially created as a clone of Bug #1031866 +++

Tested on both Win10 and Linux. We reliably crash with Nightly on this url:

http://mrdoob.com/lab/javascript/threejs/css3d/

Here is the stack:
https://crash-stats.mozilla.com/report/index/8750eadf-8ab4-428e-973b-6fc882151013

Also horrible rendering issues prior to crashing. Regressed by bug 1097464. As expected, this doesn't reproduce on Aurora43 since bug 1097464 was backed out from there.

Comment 1

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

Thinker, do we back this out, or is there a fix we can have quickly?

Comment 2

[Thinker Li [:sinker]]

It seems a dup of Bug 1211360.  There is a patch.  Could you check it?

Comment 3

[Thinker Li [:sinker]]

I don't get crash issue on linux.  So, I need more time to study crash problem.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Still crashes with the patch from bug 1211360 applied.

Comment 5

[Thinker Li [:sinker]]

Attachment: Remove clips from separator transform items

Comment 6

[Thinker Li [:sinker]]

Ryan, could you check this patch to make sure it stopping the crash?

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Still crashes.

Comment 8

[Thinker Li [:sinker]]

Attachment: Remove clips from separator transform items, v2

--
Attachment #8674797 [details] [diff] - Attachment is obsolete: true

Comment 9

[Thinker Li [:sinker]]

Ryan, this is a more complete solution.  I can not reproduce the bug on my desktop anymore, so if it does still not cure your problem, please try a debug build and give me a log.  Thank you!

Comment 10

[Thinker Li [:sinker]]

Comment on attachment 8675253 [details] [diff] [review]
Remove clips from separator transform items, v2

Robert, the problem here is elements participating a 3D rendering context having clips too.  It run into the MOZ_CRASH() we had put in InstallLayerClipPreserves3D(), clips of layers could not be applied if the layer's effective transform can not be draw in 2D (not CanDraw2D()), it can happen only for preserves 3D cases.

My solution is making every child item of a transformed, and extending 3D, item as a child of an additional separator transform item with ID transform.  If there is any clips on children, an intermediate surface will be created for the layer of it's containing separator item, and the clips would be applied on the intermediate surface without running into MOZ_CRASH().

Comment 11

[Thinker Li [:sinker]]

I forget to mentioned, children that is transformed and participating the same 3D rendering context will not be a child of a separator item.  They will be direct children of the transformed, and extending 3D, item.  There are two cases for this type of children:
 1. the frame of the child item is extending 3D too, or
 2. the frame of the child item is a leaf; participating the context, but flattening.
They can not have clips.  So, we can make them as direct children of the parent item, and be direct children on layer tree too.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8675253 [details] [diff] [review]
Remove clips from separator transform items, v2

Appears to have fixed the crash, but there's still visual glitches when the page first loads and when you click and drag the mouse to rotate the cube.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Attachment: screenshot after initial page load

This is what things look like immediately after the page loads with attachment  8675253 [details] [diff] [review] applied on m-c tip (rev bb5c1f7cc078).

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Attachment: screenshot after rotating the cube

This shows the artifacting the occurs when dragging the mouse to rotate the cube around.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

No warnings/asserts in a debug build. Seems like some kind of invalidation issue - if I resize the window, the cube renders normally until moved again.

Comment 16

[Thinker Li [:sinker]]

Do you mean it not fix glitches even applied the patch of Bug 1211360?

Comment 17

[Thinker Li [:sinker]]

I need to correct comment #16, the right patch is on bug 1210784.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Attachment: screenshot showing partially-frozen drawing

With this patch and the patches from bug 1211360 and bug 1210784 applied, things are definitely heading in the right direction. No crashing and invalidation issues AFAICT.

That said, there are still some issues remaining as shown in the attached screenshot and the one coming next. The side of the cube showing burning fire appears to partially stop drawing after a couple seconds. In this screenshot, the left side continued to animate while the right side was frozen.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Attachment: screenshot showing black rectangle on side and partially-drawn cube side

This screenshot shows two different issues: the blue side of the cube should be drawn consistently (the left side is the correct rendering). Also, the black rectangle of the left side of the screenshot starts as the full height of the viewport, then drops to the size shown in the screenshot, before finally fully disappearing. Another possible invalidation issue I guess since they tend to go away when resizing the window.

Comment 20

[Thinker Li [:sinker]]

Ryan, thank you for your feedback!  I have updated bug 1210784.  Both issues you mentioned are gone on my box.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Attachment: screenshot showing black bars on the right side

The latest patch definitely fixed the issues on the cube. However, I'm still seeing intermittent black bars on the sides during initial page load that eventually invalidate and go away given enough time.

Comment 22

[Thinker Li [:sinker]]

Attachment: layersdump.diff

Ryan, I have updated bug 1210784.  If it is still not normal, apply this patch to get layer tree, I may find out what the root cause is with log.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8675253 [details] [diff] [review]
Remove clips from separator transform items, v2

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

::: layout/generic/nsFrame.cpp
@@ +1929,5 @@
> +    nsDisplayTransform *sepIdItem =
> +      new (aBuilder) nsDisplayTransform(aBuilder, aFrame, aSource,
> +                                        aDirtyRect, Matrix4x4(), aIndex);
> +    sepIdItem->SetNoExtendContext();
> +   aTarget->AppendToTop(sepIdItem);

Fix indent

Comment 24

[Thinker Li [:sinker]]

Attachment: Remove clips from separator transform items, v3

--
Attachment #8675253 [details] [diff] - Attachment is obsolete: true

Comment 25

[Thinker Li [:sinker]]

Comment on attachment 8676755 [details] [diff] [review]
Remove clips from separator transform items, v3

please checkin this patch without closing the bug.

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02d17c84afff

Comment 27

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d2579dd7c77c - not everything else has finished yet, but Mulet reftests aren't happy:

https://treeherder.mozilla.org/logviewer.html#?job_id=16072311&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=16072307&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=16072297&repo=mozilla-inbound

Comment 28

[Thinker Li [:sinker]]

https://treeherder.mozilla.org/logviewer.html#?job_id=12857609&repo=try

This reftest (layout/reftests/bugs/745934-1.html) a little bit different in color for every pixel.  Do you think we can just make it in fuzzy?

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The test looks pretty different in the Mulet log:
https://treeherder.mozilla.org/logviewer.html#?job_id=16072311&repo=mozilla-inbound
906199-1.html is also quite different.

Comment 30

[Thinker Li [:sinker]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> The test looks pretty different in the Mulet log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=16072311&repo=mozilla-
> inbound
> 906199-1.html is also quite different.

These are already fixed on my local patch.  For separator transforms are not force active, some cases layers are not created as expected.  But, after fix this problem, comment #28 is happened.

Comment 31

[Thinker Li [:sinker]]

Attachment: Remove clips from separator transform items, v4

--
Attachment #8676755 [details] [diff] - Attachment is obsolete: true

Comment 32

[Thinker Li [:sinker]]

Minor changes
 - Make nsDisplayTransform items being separator to be LAYER_ACTIVE_FORCE.
 - nsDisplayTransform::GetBounds()returns bounds of children (or visible overflow rect) for items being separator.
 - fuzzing reftests

https://treeherder.mozilla.org/#/jobs?repo=try&revision=72a37b5eea7d

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e19090d182fa

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99f9e9779c40

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

More fuzzing for Windows: https://hg.mozilla.org/integration/mozilla-inbound/rev/99f9e9779c40
https://treeherder.mozilla.org/logviewer.html#?job_id=16180149&repo=mozilla-inbound

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e19090d182fa
https://hg.mozilla.org/mozilla-central/rev/99f9e9779c40

Comment 37

[Ryan VanderMeulen [:RyanVM]]

The patch that landed here took care of the crashes. The glitchy rendering appears to be another manifestation of bug 1210784, so let's just re-purpose this bug and close it out.

Comment 38

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked since it's a crash, even though it is already fixed.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Backed out from Aurora44 so that bug 1097464 could be backed out cleanly. It remains fixed on Fx45+ where bug 1097464 is still landed.

https://hg.mozilla.org/releases/mozilla-aurora/rev/b1716d3ede0d

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/b1716d3ede0d

Comment 41

[Ryan VanderMeulen [:RyanVM]]

This along with the change that caused this regression were backed out from Firefox 45.

https://hg.mozilla.org/releases/mozilla-aurora/rev/64ec448f156d