1593756 - Create a chrome-only async method for retrieving node frame bounds in space of an arbitrary browsing context.

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Brad Werth [:bradwerth]]

GetBoxQuads is used extensively in devtools, usually to draw a highlighter in window coordinates over a DOM node. One-domain-per-process has complicated this since nodes may have to cross process boundaries to get to the top-level document.

GetBoxQuads with no arguments should return values relative to the top-level document. If a referenceTo argument is supplied, it should work regardless of whether or not the node or any intervening ancestors reside in other processes.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

GetBoxQuads is web-exposed and that would expose the position of a cross-origin iframe relative to the parent document, which I think it's not quite desirable, or am I missing something?

Comment 2

[Brad Werth [:bradwerth]]

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

GetBoxQuads is web-exposed and that would expose the position of a cross-origin iframe relative to the parent document, which I think it's not quite desirable, or am I missing something?

I defer to your judgment on security implications. Do you recommend we create a new Chrome-only API for devtools purposes?

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Probably, or some sort of chrome-only option. Seems that getBoxQuads already has a few of those.

Is the issue that relativeTo only works in the same process? We already have appropriate restrictions for that: https://searchfox.org/mozilla-central/rev/3300072e993ae05d50d5c63d815260367eaf9179/layout/base/GeometryUtils.cpp#243

It should be feasible to add a relativeTo: "topLevelContentDocument" or something of that sort that throws for content, and does the right thing for chrome code / devtools.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Anyhow, we definitely:

• Can't tweak the default (since it's web-exposed).
• Can't expose cross-origin data to content.

Other than that I don't particularly mind the API shape.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

What kind of IDL objects can devtools use to indicate "relative to the top level browsing context"? A browsing context id of sorts?

Comment 6

[Alexandre Poirot [:ochameau]]

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

What kind of IDL objects can devtools use to indicate "relative to the top level browsing context"? A browsing context id of sorts?

Yes, we should have access to browsing context IDs if that's any helpful.

Comment 7

[Razvan Caliman [:rcaliman]]

Even though the current use case in DevTools is to get box quads for a node relative to the top-level document, I can see the usefulness and flexibility of passing a reference by BrowsingContext ID to a precise frame whose document we want quads relative to. (For the top-level document, we can pass its browsing context id).

If this abstraction is possible, it would be helpful for us.

Comment 8

[Brad Werth [:bradwerth]]

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

It should be feasible to add a relativeTo: "topLevelContentDocument" or something of that sort that throws for content, and does the right thing for chrome code / devtools.

As I look at the code, it seems that does the right thing means extracting geometry information from the BrowsingContext tree. The tree doesn't appear to have any geometry information in it. I need your advice on how to proceed. Which is best:

1. There is already a working example of how to extract geometry information from the BC tree. Is that so?
2. The fix will involve adding geometry information to the BC structures.
3. The fix will involve cross-process message passing to request geometry information.

No matter what it seems like the fix will require mixing in the BC geometry info in to the existing transformations done by GeometryUtils.cpp. Tricky.

Comment 9

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #8)

3. The fix will involve cross-process message passing to request geometry information.

Such an approach would make GetBoxQuads asynchronous, which would change its web-observable behavior. If this is the way to solve the problem, then we'll need a new chrome-only method.

Comment 10

[Emilio Cobos Álvarez [:emilio]]

1. There is already a working example of how to extract geometry information from the BC tree. Is that so?

BrowserChild already has EffectsInfo, which contains mVisibleRect, which I think is what you want. I'm not all that familiar with Fission, but it seems to me that if that's not what you want, passing the whole viewport down with EffectsInfo should be also pretty straight-forward, no need to make stuff async.

2. The fix will involve adding geometry information to the BC structures.

Probably not, but maybe, see above.

3. The fix will involve cross-process message passing to request geometry information.

I think doing multiple IPC calls per DOM node (which I think is the devtools use case) is probably not going to fly, perf-wise.

No matter what it seems like the fix will require mixing in the BC geometry info in to the existing transformations done by GeometryUtils.cpp. Tricky.

Is it much harder than a 2d translation on top of the root coordinate space?

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Setting the same priority as the blocker has for now.

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

1. There is already a working example of how to extract geometry information from the BC tree. Is that so?

BrowserChild already has EffectsInfo, which contains mVisibleRect, which I think is what you want. I'm not all that familiar with Fission, but it seems to me that if that's not what you want, passing the whole viewport down with EffectsInfo should be also pretty straight-forward, no need to make stuff async.

As far as I know the EffectsInfo is not properly set in the case where the OOP iframe is outside of display port region because we don't build display items for such frames. Probably it doesn't matter for this issue? I am totally unsure. (I filed a bug for the case, but couldn't find it now)

Comment 12

[Brad Werth [:bradwerth]]

As far as I know the EffectsInfo is not properly set in the case where the OOP iframe is outside of display port region because we don't build display items for such frames. Probably it doesn't matter for this issue? I am totally unsure. (I filed a bug for the case, but couldn't find it now)

Drawing of highlighters certainly only matters for items within the display port region. So for devtools purposes, as long as we can scroll an arbitrary DOM node into view then a display port solution will work well enough. Not sure how we do that in devtools. Razvan, how do we scroll a selected DOM node into view? Is that also done with coordinates we get from GetBoxQuads?

Comment 13

[Razvan Caliman [:rcaliman]]

(In reply to Brad Werth [:bradwerth] from comment #12)

Razvan, how do we scroll a selected DOM node into view? Is that also done with coordinates we get from GetBoxQuads?

The native DOM method Element.scrollIntoView() is used (MDN docs):
https://searchfox.org/mozilla-central/rev/d061ba55ac76f41129618d638f4ef674303ec103/devtools/server/actors/inspector/node.js#604-609

No coordinates are passed.

Comment 14

[Brad Werth [:bradwerth]]

Okay, we'll make a display port solution here.

Comment 15

[Alexandre Poirot [:ochameau]]

Note that the involved codebase on the DevTools side will very likely involve a JS Window Actor, which will do some cross process communication between the parent process and the process into which the highlighted element is running.
The highlighter are rendered in the parent process, while getBoxQuads is called from the content processes where the highlighted element is running.

So. One option, if it ends up being very hairy to make getBoxQuads be Fission-compatible, would be to workaround that by using JS Window Actors and compute the box-quads manually. By getting the box quads of the element and the <iframe> element into which it is rendered. But I imagine it could easily be error prone and may also be hairy and try to replicate existing C++ code...
Otherwise, if passing extra info between processes (like EffectsInfo, or others) is helping better support Fission for usecases other than DevTools, please ignore me :)

Comment 16

[Brad Werth [:bradwerth]]

This feels like one of those things we just have to try and see what further obstacles appear. I'm working on a patch now.

Comment 17

[Brad Werth [:bradwerth]]

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

1. There is already a working example of how to extract geometry information from the BC tree. Is that so?

BrowserChild already has EffectsInfo, which contains mVisibleRect, which I think is what you want. I'm not all that familiar with Fission, but it seems to me that if that's not what you want, passing the whole viewport down with EffectsInfo should be also pretty straight-forward, no need to make stuff async.

It appears the path from BrowsingContext to BrowserChild passes through BrowsingContext::GetDOMWindow(), which is only non-null for in-process contexts. So this is further evidence that this must be an asynchronous method. Since GetBoxQuads can't change its signature, we'll need a new method.

Comment 18

[Emilio Cobos Álvarez [:emilio]]

(In reply to Brad Werth [:bradwerth] from comment #17)

It appears the path from BrowsingContext to BrowserChild passes through BrowsingContext::GetDOMWindow(), which is only non-null for in-process contexts. So this is further evidence that this must be an asynchronous method. Since GetBoxQuads can't change its signature, we'll need a new method.

Sure, this is true, but the in-process BrowsingContext (the one you call getBoxQuads from) should have the information relative to all the ancestors, all the way to the root, shouldn't it?

Comment 19

[Brad Werth [:bradwerth]]

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

Sure, this is true, but the in-process BrowsingContext (the one you call getBoxQuads from) should have the information relative to all the ancestors, all the way to the root, shouldn't it?

Perhaps I don't understand BrowsingContext correctly. I believe that the BrowsingContext only has DOMWindow for in-process browsing contexts, and there's no guarantee that all the ancestor BCs are in-process. If you have a way to extract the embedding offsets from just the BCs (in-process or not) then I would appreciate an example.

Comment 20

[Emilio Cobos Álvarez [:emilio]]

(In reply to Brad Werth [:bradwerth] from comment #19)

Perhaps I don't understand BrowsingContext correctly. I believe that the BrowsingContext only has DOMWindow for in-process browsing contexts, and there's no guarantee that all the ancestor BCs are in-process. If you have a way to extract the embedding offsets from just the BCs (in-process or not) then I would appreciate an example.

Sure, this is true, but my point is that EffectsInfo::mVisibleRect in the browsing context that contains the element you call getBoxQuads() on (which is necessarily in-process) should be relative to the top-level browsing-context already, if I understand that correctly.

You're right that you have no guarantee that all the ancestor browsing contexts are in the same process in a fission world, but if you want coordinates relative to the top-level browsing context, then I think you only need to look at the "bottommost" browsing-context to get the right offset. And that browsing context is necessarily in-process (from the getBoxQuads() call), as you're poking at its DOM.

Comment 21

[Brad Werth [:bradwerth]]

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

Sure, this is true, but my point is that EffectsInfo::mVisibleRect in the browsing context that contains the element you call getBoxQuads() on (which is necessarily in-process) should be relative to the top-level browsing-context already, if I understand that correctly.

Ah, thank you. I finally understand. You're right, of course. Thanks for the clarity.

Comment 22

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #21)

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

Sure, this is true, but my point is that EffectsInfo::mVisibleRect in the browsing context that contains the element you call getBoxQuads() on (which is necessarily in-process) should be relative to the top-level browsing-context already, if I understand that correctly.

If the mVisibleRect is relative to the top-level BrowsingContext, then this is all we need. If it's only relative to the embedding BrowsingContext, then we're back to being asynchronous. I'll investigate.

Comment 23

[Brad Werth [:bradwerth]]

Attachment: Bug 1593756 Part 1: Add a new method GetBoxQuadsFromWindowOrigin.

This provides a way to escape beyond the process boundary, but only
to the top level browsing context. This is as far as we can get
without requiring GetBoxQuads to become async, which would be a
change to web spec.

Comment 24

[Brad Werth [:bradwerth]]

Attachment: Bug 1593756 Part 2: Rename an existing test in anticpation of new sibling tests.

Depends on D52788

Comment 25

[Brad Werth [:bradwerth]]

Attachment: Bug 1593756 Part 3: Add a test of getBoxQuadsFromWindowOrigin across cross-process iframe boundaries.

This test is structured with three documents in a 1-1-1 iframe embedding.
Each document is given a unique domain, which in a fission world should
ensure that each resides in a seperate process.

Depends on D52789

Comment 26

[Brad Werth [:bradwerth]]

The attached patches sort-of-work, but the test fails as expected with fission.autostart set to true. I need to restructure the test with a message passing approach so that the innermost document is the one doing the getBoxQuads call, since it is the only document that can directly reference the node of interest. Does this predict trouble for the way devtools will use this API? Not sure. I'll get it working and then see what needs to be changed to make it useful for devtools.

Comment 27

[Brad Werth [:bradwerth]]

The updated test in https://phabricator.services.mozilla.com/D52790 now runs in a fission.autostart=true environment, but fails. This indicates that something is wrong with my assumptions of how EffectsInfo::mVisibleRect works. Debugging commence!

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Which test actually fails? Just curious. I haven't read the test at all, as far as I can tell EffectsInfo::mVisibleRect is set after a proper paint happened on the corresponding iframe element, maybe we need to make sure it happened and the information was delivered to the OOP iframe process before proceeding the test.

Comment 29

[Brad Werth [:bradwerth]]

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

Which test actually fails? Just curious. I haven't read the test at all, as far as I can tell EffectsInfo::mVisibleRect is set after a proper paint happened on the corresponding iframe element, maybe we need to make sure it happened and the information was delivered to the OOP iframe process before proceeding the test.

It's the new test that fails, which means of course it could be poorly constructed. But the new test is pretty simple. It's a window -> iframe -> iframe embedding with 3 different domains, and the innermost iframe calls getBoxQuads with the new relativeToTopLevelBrowsingContext param. I'll debug and see if the paint timing is important.

Comment 30

[Brad Werth [:bradwerth]]

Attachment: iframe_with_margin.html

Ryan, in Bug 1558482, your patch added mVisibleRect to the EffectsInfo structure. In that patch, you included a comment:

// Adjust mItemVisibleRect, which is relative to the reference frame, to be
// relative to this frame

I have a testcase (attached) where an iframe has been styled with a margin, and the BrowserChild of the iframe is getting an mVisibleRect with zero x and zero y. That is a surprising result to me. In trying to debug this I see that

params.mItemVisibleRect = &itemVisibleRectAu;

and earlier

nsRect itemVisibleRectAu = itemContent;

Is itemContent actually relative to the reference/root frame? Or is this.mToReferenceFrame holding an incorrect value that's being subtracted away? All insights appreciated.

Comment 31

[Ryan Hunt [:rhunt]]

Hi Brad. EffectsInfo's purpose was to communicate graphics information to an iframe so it could efficiently paint without direct access to its ancestors. This included a clip rect (mVisibleRect), and two scales. The clip rect ensured that a huge iframe (think height: 10000px) only painted content that was visible. The scales apply when there is a transform on an ancestor of the iframe. These ensure that we paint at a higher or lower resolution so that we don't waste resources or still have legible text (depending on the scale).

EffectsInfo::visibleRect then is relative to the top-left corner of the content rect of the iframe. If the top of the iframe is visible, then the coordinates will be 0,0. If the iframe is scrolled partially off-screen then the coordinates will be the relative coordinates in the iframe where to start painting.

It seems like you need something different than this. It may make sense to extend EffectsInfo to include a new field for your use-case, and re-use the syncing machinery to have it communicated to all sub-processes. Just keep in mind that EffectsInfo are only updated when the iframe is visible. It's a graphics oriented feature.

Comment 32

[Brad Werth [:bradwerth]]

(In reply to Ryan Hunt [:rhunt] from comment #31)

Hi Brad. EffectsInfo's purpose was to communicate graphics information to an iframe so it could efficiently paint without direct access to its ancestors.

Thanks for the detail. Emilio, it sounds like we either need to add embedding rect to the BrowsingContext/BrowserChild, or to make a new async method for this devtools need. Do you have a preference?

Comment 33

[Emilio Cobos Álvarez [:emilio]]

know at least one place where the embedding rect could be handy (though I guess the current behavior of just restricting to the iframe's viewport is ok for cross-origin iframes).

It seems it could be handy for other cases? Worth asking other fission folks... No preference otherwise.

Comment 34

[Chris Peterson [:cpeterson]]

Tracking DevTools bugs for Fission Nightly (M6) milestone

Comment 35

[Brad Werth [:bradwerth]]

Attachment: Bug 1593756 Part 2: Add a field EmbeddingOffset to BrowsingContext, and fill it in after reflow.

Comment 36

[Brad Werth [:bradwerth]]

This is coming along. The proposed patches work in that they can handle the single case of a doubly-embedded content process that can make a privileged call to get the quads relative to the top-level browsing context. Here's what's still untested:

1. Measurements relative to another browsing context that is an ancestor of the calling process.
2. Measurements relative to another browsing context entirely separate from the ancestors of the calling process.
3. Anything that uses the JSWindowActor message passing paradigm to make the call in the first place. Right now it's spoofed.

But I wanted to get this out there to start getting reviewer feedback.

Comment 37

[Brad Werth [:bradwerth]]

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

Comment 38

[Brad Werth [:bradwerth]]

Attachment: Bug 1593756 Part 1: Expose GeometryUtils GetBoxRectForFrame.

This method is useful outside of GeometryUtils to get different
boxes while respecting SVG outer frames.

Comment 39

[Brad Werth [:bradwerth]]

Emilio pointed out that my proposed approach will fail if any embedded iframes are affected by CSS transforms. I'm going to try to build a solution off of the work in Bug 1599795, which accounts for that.

Comment 40

[Brad Werth [:bradwerth]]

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

Comment 41

[Brad Werth [:bradwerth]]

Bug 1617154 seems to be working at the same problem, through the Intersection Observer API. Intersection Observers are about visibility which is not exactly what we're looking for, since we want a fully transformed quad. Still, it would be nice to have a in-process solution.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/54838c3b605f
Part 1: Add a new method GetBoxQuadsFromWindowOrigin. r=emilio,hiro
https://hg.mozilla.org/integration/autoland/rev/1d0e486e2999
Part 2: Rename an existing test in anticpation of new sibling tests. r=emilio
https://hg.mozilla.org/integration/autoland/rev/2220f62fbd3b
Part 3: Add a test of getBoxQuadsFromWindowOrigin across cross-process iframe boundaries. r=emilio

Comment 43

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/54838c3b605f
https://hg.mozilla.org/mozilla-central/rev/1d0e486e2999
https://hg.mozilla.org/mozilla-central/rev/2220f62fbd3b