1156238 - "Assertion failure: nsLayoutUtils::IsAncestorFrameCrossDoc(Builder()->RootReferenceFrame(), aAnimatedGeometryRoot)"

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase (asserts after 1-2 seconds)

Assertion failure: nsLayoutUtils::IsAncestorFrameCrossDoc(Builder()->RootReferenceFrame(), aAnimatedGeometryRoot), at layout/base/FrameLayerBuilder.cpp:2504

This assertion was added in https://hg.mozilla.org/mozilla-central/rev/10e5dd951795 (bug 1148855)

Comment 1

[Jesse Ruderman]

Attachment: stack

mozilla::dom::CanvasRenderingContext2DBinding::drawWindow is on the stack. I guess this is related to some Firefox frontend feature.

Comment 2

[Timothy Nikkel (:tnikkel)]

If we are drawWindow'ing some iframe the root reference frame will be the root frame of the iframe. The animated geometry root could be a frame in the parent doc.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

I hit this assertion dragging buttons in the toolbar's "customize" mode on Linux. Is that likely to be fixed by a patch for this bug?

Comment 4

[Markus Stange [:mstange]]

Interesting, I don't know yet.

Comment 5

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1156238 - Replace erroneous assert with something that ensures what we used to assert. r?tn

Bug 1156238 - Replace erroneous assert with something that ensures what we used to assert. r?tn

Comment 7

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8655645 [details]
MozReview Request: Bug 1156238 - Replace erroneous assert with something that ensures what we used to assert. r?tn

https://reviewboard.mozilla.org/r/17961/#review16617

I don't think this will fix the problem of calling ComputeFrameMetrics on scrollframes for which BuildDisplayList hasn't been called.

I looked into a testcase (loading a google groups thread, select all, click and drag selection). In that case the root reference frame was the block frame for the body element. We follow placeholders and build items for an absolute element whose frame is a child of the canvas frame for the body element. The canvas frame is an ancestor of the block frame. So the ancestors of the absolute item never go through the root reference frame.

This can only happen when the root reference frame is not a root frame, which can only happen when we are drawing to non-retained, non-widget layer managers. So all the extra async scroll processing we do is useless. So maybe we can avoid the problem by avoiding doing the extra async scroll processing for non-retained layer managers?

Alternatively we could make frame layer builder deal with complicated cases like this with AGMs.

Comment 8

[Timothy Nikkel (:tnikkel)]

Can you still reproduce this? I could only reproduce using the steps in bug 1186407 but the testcase and stack indicates that this bug as originally filed could be a different issue and I'd like to understand it.

Comment 9

[Timothy Nikkel (:tnikkel)]

Attachment: Fix the assert

Comment 10

[Timothy Nikkel (:tnikkel)]

Attachment: Make sure no AGMs escape

I audited the code and found a couple cases where we didn't provide a stop at ancestor when computing the AGM. That could cause asserts like this. I made it mandatory to always provide a stop at ancestor. If we computed the AGM for an item without a "stop at" ancestor during display list building, and then with a "stop at" ancestor in FrameLayerBuilder we could have two different AGMs for the same item.

Comment 11

[Mats Palmgren (inactive)]

FYI, I noted that Matt introduced some AGM caching in bug 1205087.
(Just mentioning it in case it affects the patches here in any way.)

Comment 12

[Jesse Ruderman]

I can't reproduce this using the testcase in comment 0, even with an added call to fuzzPriv.callDrawWindow(2).

Comment 13

[Markus Stange [:mstange]]

I just realized that these patches never landed. I think we're going to need something like them in order to fix bug 1199131: In that bug, while painting the <select> popup (non-e10s), the builder's root reference frame is the <select> popup frame, and the animated geometry root of the background-attachment:fixed item is the root frame of the XUL window (which is the animated geometry root of the content page's viewport frame).

Timothy, were you planning on finishing the patches here? Do you know what's left to be done?

Comment 14

[Timothy Nikkel (:tnikkel)]

Yes, I was going to land them, but they likely conflict with bug 1205087, which seems more important. Bug 1205087 is blocked on bug 1208780, where I need to write a new patch because I think we can accomplish the same thing much simpler.

So I need to get bug 1205087 landed, and the rebase the patches here on top of them.

Comment 15

[Markus Stange [:mstange]]

(In reply to Timothy Nikkel (:tn) from comment #14)
> Yes, I was going to land them, but they likely conflict with bug 1205087,
> which seems more important.

Bug 1199131 is already on release, so we might want to have patches that apply without bug 1205087 anyway.

Comment 16

[Timothy Nikkel (:tnikkel)]

Attachment: Always stop at root reference frame when looking for an AGR

This is a large chunk of Matt's patch in bug 1205087 minus the actual caching of the agr on display items.

I liked the approach there better then my previous patch here.

Getting bug 1205087 landed is taking a little longer, but we should fix this because it is causing crashes. Also separating out this change from the caching makes sense.

The only new bit is the ShouldFixToViewport part.

Comment 17

[Timothy Nikkel (:tnikkel)]

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

Comment 18

[Timothy Nikkel (:tnikkel)]

Attachment: fix try server failures with previous patches

The patches had some try server failures. The explanation is all in the commit msg.

Comment 19

[Timothy Nikkel (:tnikkel)]

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

Comment 20

[Timothy Nikkel (:tnikkel)]

And finally a green try run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3adbfe011e8f

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d71945686e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/515229f96686
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb0f1211b033

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d71945686e0
https://hg.mozilla.org/mozilla-central/rev/515229f96686
https://hg.mozilla.org/mozilla-central/rev/cb0f1211b033

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/1d71945686e0
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/515229f96686
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/cb0f1211b033

Comment 24

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion