1888904 - Unable to create an account into kia website (unable to click fission iframe)

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P2)

Description

[Emilio Cobos Álvarez (:emilio)]

Attachment: Semi-reduced test-case.

See https://www.reddit.com/r/firefox/comments/1br7pgv/is_the_kia_website_punishing_me_for_using_firefox/

As per the comments there, removing the scale transform works around the issue.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Reduced test-case.

Removing any of the transform / perspective rules make this text selectable. Botond, it seems the perspective property has no effect on rendering but APZ somehow tries to honor it, causing the mismatch between painting and APZ. Do you know where the relevant code is?

Comment 2

[Thomas Wisniewski [:twisniewski]]

Mozregression could only give me this range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c9d499871dc8c87d1f138d1a954b675df5030a43&tochange=06c7b4e2d14be1ee8050d6c1eb1ab1bab19ac3c2

That includes when Fission was enabled for Firefox 97 (in bug 1732358).

For what it's worth, I got the same range when I ran another mozregression on earlier builds (back to Firefox 70) with the webrender and fission autostart prefs on.

Comment 3

[Timothy Nikkel (:tnikkel)]

With fission force enabled I get this regression range

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a35461d1fc07e6e855d064453363a46d32bb5a3d&tochange=bd7c35b7e234712f94f6dbb755ad983c1db8b07b

Comment 4

[Timothy Nikkel (:tnikkel)]

There is perspective specific transform code in gfx/layers/apz/src/APZCTreeManager.cpp and gfx/layers/apz/src/AsyncPanZoomController.h so perhaps that might have something to do with it.

Comment 6

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #3)

With fission force enabled I get this regression range

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a35461d1fc07e6e855d064453363a46d32bb5a3d&tochange=bd7c35b7e234712f94f6dbb755ad983c1db8b07b

This is just when webrender got enabled on the machine I was testing on. Webrender made some changes to how scroll data and transforms were managed with apz.

Comment 7

[Timothy Nikkel (:tnikkel)]

Forcing fission and webrender on I get

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1e3244e602fc0373508049b6fe544332f800f033&tochange=3c70f36ad62c9c714db3199fc00e60800ee82bde

Which unfortunately contains two changes that are likely to be relevant:

Nika Layzell — Bug 1544811 - Use web processes on a per-site basis for fission-enabled windows, r=mconley

Before this the iframe in the testcase wasn't out of process.

Kartikaya Gupta — Bug 1533673 - Allow APZ to send fission matrices with the GPU process. r=hsivonen

But I can pref that on using fission.apz-matrices-with-gpu-process, and when I do I still get the same regression range, so that means this bug goes back to the earliest start of fission with webrender (but not without webrender).

Comment 8

[Botond Ballo [:botond]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)

Removing any of the transform / perspective rules make this text selectable. Botond, it seems the perspective property has no effect on rendering but APZ somehow tries to honor it, causing the mismatch between painting and APZ. Do you know where the relevant code is?

We have special handing in APZ for perspective transforms which dates back to bug 1168263 (with follow-up changes in bug 1424591 and bug 1429373). This code mostly pre-dates WebRender (and definitely Fission), and it's not clear to me whether it's still necessary.

The special handling is gated on the transformIsPerspective flag set here. It would be interesting to see if the problem occurs with that line removed (leaving the flag at its default value of false), and if so, whether removing that line breaks anything else (if not, maybe we can just remove the logic gated on it).

Comment 9

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #8)

The special handling is gated on the transformIsPerspective flag set here. It would be interesting to see if the problem occurs with that line removed (leaving the flag at its default value of false)

Doesn't look like that fixes it (the problem still repros). So maybe it's a different issue (not related to bug 1168263 etc.) then.

Comment 10

[Botond Ballo [:botond]]

In the absence of a more specific lead, the first place in the code I would look for this type of issue is BrowserParent::SendRealMouseEvent, specifically the place where we put child-process coordinates into mRefPoint here.

If the child-process coordinates which are set there look wrong, the issue is probably with the parent-to-child transform matrix (usually with the code in APZ which computes it). If the child-process coordinates look right, the issue is more likely during the event dispatch in the content process.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

would a pernosco trace help here?

Comment 12

[Botond Ballo [:botond]]

(In reply to Randell Jesup [:jesup] (needinfo me) from comment #11)

would a pernosco trace help here?

It would save time for sure!

Comment 13

[Hiroyuki Ikezoe (:hiro)]

I dumped WebRenderLayerScrollData for the test case in comment 2. There are two interesting data;

        WebRenderLayerScrollData(0x7f953d041318), descendantCount=1, item=0x7f9525115c98, metadata0={ [description=] [metrics={ [cb=(x=0, y=0, w=2304, h=1910)] [sr=(x=0, y=0, w=1152, h=955)] [s=(0,0)] [dp=(x=0, y=0, w=1152, h=955)] [rcs=(1152 x 955)] [v=(x=0, y=0, w=1152, h=955)] [z=(ld=2.000 r=1.000 cr=1 z=2 t=1 )] [u=(0 1)] scrollId=2 [rcd] }] [overscroll=auto] [0 scrollupdates] }, ancestorTransform=[ 1 0; 0 1; 846 870; ] (asr=2), transform=[ 1 0; 0 1; 2 2; ], transformIsPerspective, visible=(x=0, y=0, w=0, h=0)
            WebRenderLayerScrollData(0x7f953d0414a0), descendantCount=0, item=0x7f95251162b8, ancestorTransform=[ 0.91 0 0 0; 0 0.91 0 0; -0.575 -0.4765 1 -0.0005; 846 870 0 1; ] (asr=2), visible=(x=0, y=0, w=0, h=0), refLayersId=0x10000000b

To me both ancestorTransforms have perspective, but the second one don't have transformIsPerspective. It should have been set?

Comment 14

[Timothy Nikkel (:tnikkel)]

Interesting. That flag is set here

https://searchfox.org/mozilla-central/rev/159929cd10b8fba135c72a497d815ab2dd5a521c/layout/painting/nsDisplayList.cpp#6698

in nsDisplayTransform::UpdateScrollData. If we look at the old layers code

https://searchfox.org/mozilla-esr78/rev/3c633b1a0994f380032a616eced632398354d83e/layout/painting/nsDisplayList.cpp#8630

it sets it there, but there is an interesting comment/flag:

  // Sort of a lie, but we want to pretend that the perspective layer extends a
  // 3d context so that it gets its transform combined with children. Might need
  // a better name that reflects this use case and isn't specific to
  // preserve-3d.
  container->SetContentFlags(container->GetContentFlags() |
                             Layer::CONTENT_EXTEND_3D_CONTEXT);
  container->SetTransformIsPerspective(true);

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1888904 - A mochitest. r?botond

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Posted a mochitest based on the reduced test case in comment 1, thanks to Emilio.

Comment 17

[Timothy Nikkel (:tnikkel)]

Hmm, the bugs/patches associated with the other tests in the tree called helper_hittest_iframe_perspective* are interesting and perhaps useful.

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

I have an rr recording; I'll submit to pernosco

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Though I have no idea where we should tweak, but I am moderately sure that the real problem is in between two WebRenderLayerScrollData in comment 13. For both data emitAncestorTransform in WebRenderScrollDataWrapper::GetTransform is true, the parent one is Metrics().GetScrollId() == mLayer->GetAncestorTransformId() is true, and the child one is !Metrics().IsScrollable() is true, thus we apply (846 870) translation twice.

Comment 20

[Botond Ballo [:botond]]

Note that one WebRenderLayerScrollData node can produce multiple WebRenderLayerScrollDataWrapper nodes.

The purpose of the emitAncestorTransform check is to ensure that, for a given WebRenderLayerScrollData, only one of its WebRenderLayerScrollDataWrappers contains the ancestor transform.

Here, we have two different WebRenderLayerScrollData nodes with the (846, 870) translation in its ancestor transform. So, it's expected that both of them will have emitAncestorTransform=true once.

(However, it may be a bug that we have two WRLSD nodes with the transform in the first place. If so, the bug is earlier in the pipeline, e.g. in the code during WebRender display list building which builds the WRLSD tree.)

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

https://pernos.co/debug/MzFtEkuxU21AvOM-B_95yQ/index.html

Comment 22

[Botond Ballo [:botond]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #16)

Posted a mochitest based on the reduced test case in comment 1, thanks to Emilio.

Note that there may be an issue with this test: when I run it locally, APZ does not route the event to the iframe's content process at all.

At the time, the compositor-side WebRenderScrollData tree contains a RefLayer node for the iframe, but it does not have any descendant nodes (they appear in later composites), so probably the display list of the iframe's content process has not arrived at the compositor yet.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

(In reply to Botond Ballo [:botond] from comment #22)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #16)

Posted a mochitest based on the reduced test case in comment 1, thanks to Emilio.

Note that there may be an issue with this test: when I run it locally, APZ does not route the event to the iframe's content process at all.

At the time, the compositor-side WebRenderScrollData tree contains a RefLayer node for the iframe, but it does not have any descendant nodes (they appear in later composites), so probably the display list of the iframe's content process has not arrived at the compositor yet.

Okay, probably it's time to implement a new variant of contentTransformsReceived() which will ensure a new transform matrix sent by APZ has been received in the given iframe context after invoking contentTransformsReceived(). I saw a such kind of use case in https://bugzilla.mozilla.org/show_bug.cgi?id=1829615#c26 .

Comment 24

[Botond Ballo [:botond]]

Calling forceLayerTreeToCompositor() inside the iframe before synthesizing the event seems to fix it for me.

Comment 25

[Botond Ballo [:botond]]

Attachment: Display list dump + content-side WebRenderScrollData

Attached is a display list dump and content side WebRenderScrollData dump from the root content process.

Comment 26

[Botond Ballo [:botond]]

There are three nested nsDisplayTransform items in the display list, with the respective transform matrices being:

1. [ 1 0; 0 1; 487 402; ]
2. [ 1 0; 0 1; 1 1; ]
3. [ 0.91 0 0 0; 0 0.91 0 0; -0.152 -0.041 1 -0.001; 0 0 0 1; ]

and two nested WebRenderLayerScrollData nodes with ancestor transforms, with the first node's transform being (1) and the second node's transform being the product of (1) and (3).

Since the two WRLSD nodes are nested, transform (1) is applied twice, which it shouldn't be based on the display list, so I would say this is a bug in WebRenderScrollData building.

Comment 27

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #26)

I would say this is a bug in WebRenderScrollData building

More specifically, I believe this is a bug in the implementation of the "deferred transform" optimization.

Comment 28

[Botond Ballo [:botond]]

Here's a summary of what happens during WebRenderScrollData building to bring about this result, using (1), (2), and (3) to refer to the three nsDisplayTransform items from comment 26, and (A) and (B) to refer to the outer and inner WRLSD nodes which both get the translation transform:

• (1) creates a StackingContextHelper with (1) as its mDeferredTransformItem
• (1) does not create a WRLSD node
• (2) creates a StackingContextHelper with (2) as its mDeferredTransformItem and the transform of (1) (taken from the parent StackingContextHelper) as its mDeferredAncestorTransform
• (2) does create a WRLSD node, (A) because its UpdateScrollData() returns true (because ChildrenHavePerspective() returns true; this is why using perspective is relevant to the bug)
• the created WRLSD node (A) has as its ancestor transform (1) * (2) -- so far, so good
• (3) creates a StackingContextHelper with (3) as its mDeferredTransformItem and (1)*(2) as its mDefereredAncestorTransform
  • even though (1)*(2) has been emitted already, there is nothing to guard against this
• (3) does not create its own WRLSD node
• but (3)'s descendant nsDisplayRemote item (this is why Fission is relevant to the bug) creates a WRLSD node (B) with its ancestor transform being (1)(2)(3), incorrectly duplicating the (1)*(2) component

The bug is reminiscent of bug 1753779, where we discovered (bug 1753779 comment 7) that there wasn't really a mechanism in place to prevent a deferred transform from being emitted multiple times onto ancestor and descendant WRLSD nodes. I put in place such a mechanism in that bug, but it does not handle cases like this where the two deferred transforms are not identical but contain a shared component which should not be duplicated.

Comment 29

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #24)

Calling forceLayerTreeToCompositor() inside the iframe before synthesizing the event seems to fix it for me.

Regarding the test, it seems I spoke too soon, as my attempted changes only fix the test problem intermittently in local runs.

Comment 30

[BugBot [:suhaib / :marco/ :calixte]]

Based on comment #2, this bug contains a bisection range found by mozregression. However, the Regressed by field is still not filled.

:botond, if possible, could you fill the Regressed by field and investigate this regression?

For more information, please visit BugBot documentation.

Comment 31

[Botond Ballo [:botond]]

Attachment: Bug 1888904 - Try harder to avoid emitting the same transform on nested WebRenderLayerScrollData nodes. r=tnikkel,hiro

Comment 32

[Botond Ballo [:botond]]

Attachment: Bug 1888904 - Remove the earlier check against emitting the same transform on nested WebRenderLayerScrollData nodes. r=tnikkel,hiro

This check, added in bug 1753779, should be made redundant by the
more robust check added in the previous patch.

Depends on D207190

Comment 33

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1732358

Comment 34

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1732358

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6c64c0be90df
A mochitest. r=botond
https://hg.mozilla.org/integration/autoland/rev/ef77b802e05d
Try harder to avoid emitting the same transform on nested WebRenderLayerScrollData nodes. r=tnikkel,hiro
https://hg.mozilla.org/integration/autoland/rev/2adaf5befa1e
Remove the earlier check against emitting the same transform on nested WebRenderLayerScrollData nodes. r=tnikkel,hiro

Comment 36

[Aron Cseh]

https://hg.mozilla.org/mozilla-central/rev/6c64c0be90df
https://hg.mozilla.org/mozilla-central/rev/ef77b802e05d
https://hg.mozilla.org/mozilla-central/rev/2adaf5befa1e

Comment 37

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:botond, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox126 to wontfix.

For more information, please visit BugBot documentation.

Comment 38

[Botond Ballo [:botond]]

Attachment: Bug 1888904 - A mochitest. r?botond

Original Revision: https://phabricator.services.mozilla.com/D206715

Comment 39

[Botond Ballo [:botond]]

Attachment: Bug 1888904 - Try harder to avoid emitting the same transform on nested WebRenderLayerScrollData nodes. r=tnikkel,hiro

Original Revision: https://phabricator.services.mozilla.com/D207190

Comment 40

[Botond Ballo [:botond]]

Attachment: Bug 1888904 - Remove the earlier check against emitting the same transform on nested WebRenderLayerScrollData nodes. r=tnikkel,hiro

This check, added in bug 1753779, should be made redundant by the
more robust check added in the previous patch.

Original Revision: https://phabricator.services.mozilla.com/D207191

Comment 41

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Users are unable to click content inside out-of-process iframes on some websites, such as the Kia website, where the iframe is subject to certain types of CSS transforms.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: n/a
• Risk associated with taking this patch: Patch modifies code for transforming coordintes of events sent to out-of-process iframes. There is small risk of regresisng other scenarios involving events targeting out-of-process iframes.
• Explanation of risk level: Low to moderate. The patch touches tricky code, but we've accumulated some automated test coverage from similar bugs over time
• String changes made/needed: n/a
• Is Android affected?: no

Comment 42

[Botond Ballo [:botond]]

Requested uplift. I did confirm that the bug is resolved on the website in the original report.

Comment 43

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/055a21991f34
https://hg.mozilla.org/releases/mozilla-beta/rev/87f09fd4a52b
https://hg.mozilla.org/releases/mozilla-beta/rev/ecee005835e2