Webrender: Content area stuff is drawn over the chrome in Firefox

[staktrace]

https://bugzilla.mozilla.org/show_bug.cgi?id=1458373

STR:

• Start firefox nightly with webrender
• Go to https://addons.mozilla.org/en-US/firefox/addon/steven-universe-sky/ and install (or just "tap to preview")
• Go to https://bugzilla.mozilla.org/attachment.cgi?id=8980322

[staktrace]

According to https://bugzilla.mozilla.org/show_bug.cgi?id=1458373#c3 this might be a regression from #2707, although the issue has evolved over time so it's not clear to me if that was what originally regressed it or not.

Edit: never mind, see next comment

[Darkspirit]

https://bugzilla.mozilla.org/show_bug.cgi?id=1458373#c3 was a fix range.

This probably has the same cause as bug 1455839.

I partly shared this assumption because #2707 fixed bug 1455839 and parts of this bug ("better: Those huge rectangles are gone.")

[kvark]

I'll look into this today (assuming #2829 goes through without issues)

[kvark]

Some (hopefully useful) output from the basic chasing of #2710:

Chasing PrimitiveIndex(98)
    preparing a run of length 39 in pipeline PipelineId(1, 5)
    updating clip task with screen rect TypedRect(242×18 at (364,11))
    base combined outer rect Some(TypedRect(240×18 at (366,11)))
    need no task: all clips are within the coordinate system
    considered visible and ready with local rect TypedRect(242.0×18.0 at (-2.0,74.0))

[kvark]

After another dive into gdb, I've got more clues. convert_clip_chain_to_clip_vector produces no clips, even though the scroll frame's coordinate system doesn't match the clip chain's (in which case we are generally expected to build a mask).
This method goes though the clip chain and accumulates clip, but it misses the one we need because:

                // If this clip's inner area contains the area of the primitive clipped
                // by previous clips, then it's not going to affect rendering in any way.
                if node.screen_inner_rect.contains_rect(combined_outer_rect) {
                    return None;
                }

Problem with this is that combined outer rect is constructed from the clip chain itself:

        let mut combined_outer_rect =
            prim_screen_rect.intersection(&prim_run_context.clip_chain.combined_outer_screen_rect);

As a result, this code thinks that the corresponding node has been taken into account, but what follows expect this very code to take responsibility for the proper chain rectangle. In other words, we shouldn't consider the clip chain from both ends (namely, when getting the combined outer rect and when collecting clips).

cc @mrobinson

[mrobinson]

@kvark Wow. Thanks for debugging this! I'll take a look at this today to see if I can make a PR.

[mrobinson]

I did some more debugging on this and it seems like the node.screen_inner_rect.contains_rect(combined_outer_rect) check is a red-herring, simply because removing it did not fix the issue. I confirmed this with the francine demo as well as with this test case: https://bug1470855.bmoattachments.org/attachment.cgi?id=8987464.

[kvark]

@mrobinson you are correct, it's not fixing the issue. Do you see a fault in my explanation of the problem though? I may need to experiment with this a bit more, but I definitely debugged through a case where the coordinate systems of clips were different from the one of the primitive itself, and no clip tasks were generated.

[mrobinson]

@kvark I'm not sure about your explanation. I tried to reproduce it with Wrench YAML, but wasn't able to come up with a test case that tickled the bug the way you described it. Not exactly sure what is going on here.

[staktrace]

@kvark Any update here?

[kvark]

@staktrace nope, still looking at it

[kvark]

I've been looking at the test case from "about:addons" provided by @staktrace earlier, dumping all the things and debugging through the clip chains (PRs are to follow to upstream some of that debug logic). What I found is that the clip chains of the display items in the content IFrame do not have the IFrame's clip chain as a parent. Therefore, they don't get clipped to the content region. Here is some data from the capture:

//Item:
                (
                    item: Text((
                        font_key: ((5), 2),
... // glyphs are omitted
                    )),
                    clip_and_scroll: (
                        scroll_node_id: Clip(5, (1, 5)),
                        clip_node_id: Some(ClipChain((1, (1, 5)))),
                    ),
                    info: (
                        rect: ((-2, 74), (242, 18)),
                        clip_rect: ((-3, 73), (244, 20)),
                        is_backface_visible: true,
                        tag: None,
                    ),
                ),// [12]
// Clip chain:
                    item: ClipChain((
                        id: (1, (1, 5)),
                        parent: None, // <--- LOOK HERE
                    ), [
                        Clip(5, (1, 5)),// [0]
                        Clip(4, (1, 5)),// [1]
                    ]),
                    clip_and_scroll: (
                        scroll_node_id: Clip(1, (1, 5)),
                        clip_node_id: None,
                    ),

Both of those clips don't affect much:

    effective clip chain from CoordinateSystemId(0) (applied)
        CoordinateSystemId(0) TypedRect(819.0×669.0 at (366.0,-63.0))
        CoordinateSystemId(0) TypedRect(819.0×669.0 at (366.0,-63.0))

The actual clip of the content is in the IFrame's clip chain:

// Content IFrame
                (
                    item: Iframe((
                        clip_id: Clip(26, (1, 1)),
                        pipeline_id: (1, 5),
                        ignore_missing_pipeline: true,
                    )),
                    clip_and_scroll: (
                        scroll_node_id: Clip(25, (1, 1)),
                        clip_node_id: Some(ClipChain((32, (1, 1)))),
                    ),
                    info: (
                        rect: ((366, -63), (819, 669)),
                        clip_rect: ((366, -63), (819, 669)),
                        is_backface_visible: true,
                        tag: None,
                    ),
                ),// [161]
// Content clip chain
                (
                    item: ClipChain((
                        id: (32, (1, 1)),
                        parent: Some((2, (1, 1))),
                    ), [
                        Clip(25, (1, 1)),// [0]
                        Clip(13, (1, 1)),// [1]
                    ]),
                    clip_and_scroll: (
                        scroll_node_id: Clip(4, (1, 1)),
                        clip_node_id: Some(ClipChain((2, (1, 1)))),
                    ),
                    info: (
                        rect: ((0, 0), (0, 0)),
                        clip_rect: ((0, 0), (0, 0)),
                        is_backface_visible: true,
                        tag: None,
                    ),
                ),// [160]

So, simply by linking the item's clip chain parent to the content clip chain we get the proper clipping here:

                    item: ClipChain((
                        id: (1, (1, 5)),
                        //parent: None, // old one
                        parent: Some((32, (1, 1))),
                    ),

Processed clips now:

    effective clip chain from CoordinateSystemId(0) (applied)
        CoordinateSystemId(0) TypedRect(819.0×669.0 at (366.0,-63.0))
        CoordinateSystemId(0) TypedRect(819.0×669.0 at (366.0,-63.0))
        CoordinateSystemId(0) TypedRect(1027.0×339.0 at (240.0,149.0))
        CoordinateSystemId(0) TypedRect(819.0×669.0 at (366.0,-63.0))
        CoordinateSystemId(0) TypedRect(1284.0×413.0 at (0.0,75.0))

TL;DR: looks like a downstream Gecko issue with providing the clip chains.

[staktrace]

(Some of this is repeated from the IRC discussion)

The iframe item is sent from a different process than the iframe contents, which makes it hard for gecko to link the clip chains the way WR wants. And even if did that, a malicious content process might not do that, and gain the ability to scribble over top of the chrome, which would be bad. So it would be better if WR internally enforced that contents of an iframe cannot escape the clip on the iframe.

@kvark said that this should be possible by replacing None parent clipchain ids with the iframe's clipchain id while flattening the iframe. This would fix this bug.

However I worry that malicious content might still be able to escape by explicitly setting a parent clip to the UI process' root clipchain instead of None which would allow it to place content anywhere in the UI process visible area. That might be something we can handle separately though.

[staktrace]

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1473940 to track that separately.

[mrobinson]

@staktrace @kvark Iframes should already be clipping all their content to the local clip rectangle of the iframe item. WebRender explicitly creates the clip here:

https://github.com/servo/webrender/blob/0e9563688e575cd662570f54bc9d6f849040dbf8/webrender/src/display_list_flattener.rs#L516-L523

This means that there should be no need to explicitly link up ClipChains between different pipelines. This is handled automatically because the root reference frame of the iframe is created as a child of this clip.

[kvark]

@mrobinson but I don't see where this code uses clip_and_scroll_ids.clip_node_id at all. One of those two things need to be fixed:

1. WebRender needs to take that clip_node_id into account, e.g. #2875
2. Gecko shouldn't pass anything important in there and instead provide the clip rectangle by other means (e.g. as a local_clip_rect of the iFrame item?)

[mrobinson]

@kvark Is Gecko not passing an appropriate rectangle here for the local_clip_rect currently?

[kvark]

@mrobinson yes, looks like that:

clip_rect: ((366, -63), (819, 669)),

cc @staktrace

[staktrace]

Gecko should be using the same rect for the rect and clip_rect on the iframe item, since we pass it to LayoutPrimitiveInfo::new. So if the iframe is showing in the right place, the clip should be correct as well... unless they're in different coordinate spaces?

[mrobinson]

@kvark I'm not sure why the clip rectangle for the item is wrong, but I just realized that now that #2871 has landed, it's quite possible for a clip node to be positioned by one node and to be a child clip of another node or even ClipChain. This means that we could theoretically do something useful with the clip part of the ClipScrollInfo.

[mrobinson]

@staktrace These two rectangles should be in the same coordinate space. If not, that's a bug.

[kvark]

@mrobinson I tried to see what the scroll node of the iFrame has, going from scroll_node_id: Clip(25, (1, 1)), in the iFrame item. There simply is no such scroll node in there. The only match is:

(
                    item: Clip((
                        id: Clip(25, (1, 1)),
                        image_mask: None,
                    ), [
                    ]),
                    clip_and_scroll: (
                        scroll_node_id: Clip(12, (1, 1)),
                        clip_node_id: None,
                    ),
                    info: (
                        rect: ((366, -63), (819, 669)),
                        clip_rect: ((366, -63), (819, 669)),
                        is_backface_visible: true,
                        tag: None,
                    ),
                ),// [159]

Is this expected?

Edit: full scene RON for your reference: scene-1-0.ron.zip

[mrobinson]

@kvark Is there any way that I can use this ron with wrench?

[mrobinson]

@kvark If a node's parent is a clip node, the closest spatial node in the hierarchy should be used for positioning.

[kvark]

@staktrace do you still have that (reduced) capture from windows uploaded somewhere?

[staktrace]

https://mozilla.staktrace.com/tmp/1462955.tgz is the capture I have lying around, I think that's the one I sent you earlier.

[kvark]

@mrobinson are you planning to look at this capture, or should we take it from here?

[mrobinson]

@kvark Sorry for the delay. I think something like #2875 is the way to go. Tomorrow I will take another look at the PR.