Closed Bug 1593756 Opened 11 months ago Closed 7 months ago

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

Categories

(Core :: Layout, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla75
Fission Milestone M6
Tracking Status
firefox75 --- fixed

People

(Reporter: bradwerth, Assigned: bradwerth)

References

(Regressed 1 open bug)

Details

(Whiteboard: dt-fission-m2-mvp)

Attachments

(4 files, 2 obsolete files)

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.

Summary: GetBoxQuads should return values relative to top level document, and work with cross-process referenceTo arguments.. → GetBoxQuads should return values relative to top level document, and work with cross-process referenceTo arguments.

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?

(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?

Flags: needinfo?(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.

Flags: needinfo?(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.

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

Summary: GetBoxQuads should return values relative to top level document, and work with cross-process referenceTo arguments. → GetBoxQuads should add Chrome-only param to return values relative to top level document.

(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.

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.

(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.

Flags: needinfo?(emilio)

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

  1. 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.

  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.

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

Probably not, but maybe, see above.

  1. 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?

Flags: needinfo?(emilio)

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)

Priority: -- → P3

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?

Flags: needinfo?(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.

Flags: needinfo?(rcaliman)

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

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 :)

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.

(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.

Summary: GetBoxQuads should add Chrome-only param to return values relative to top level document. → Create a chrome-only async method for retrieving node frame bounds in space of an arbitrary browsing context.

(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?

(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.

(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.

(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.

(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.

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.

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

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.

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!

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.

(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.

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.

Flags: needinfo?(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.

Flags: needinfo?(rhunt)

(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?

Flags: needinfo?(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.

Flags: needinfo?(emilio)
Priority: P3 → P2
Whiteboard: dt-fission-m2-mvp
Status: NEW → ASSIGNED
Priority: P2 → P1

Tracking DevTools bugs for Fission Nightly (M6) milestone

Fission Milestone: --- → M6
Attachment #9108301 - Attachment description: Bug 1593756 Part 1: Add a chrome-only param to GetBoxQuads to return values relative to top level browsing context. → Bug 1593756 Part 2: Add a chrome-only param to GetBoxQuads to return values relative to another browsing context.
Attachment #9108302 - Attachment description: Bug 1593756 Part 2: Rename an existing test in anticpation of new sibling tests. → Bug 1593756 Part 3: Rename an existing test in anticpation of new sibling tests.
Attachment #9108303 - Attachment description: Bug 1593756 Part 3: Add a new test of getBoxQuads across cross-process iframe boundaries. → Bug 1593756 Part 4: Add a new test of getBoxQuads across cross-process iframe boundaries.

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.

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

Attachment #9125224 - Attachment description: Bug 1593756 Part 1: Add a field EmbeddingOffset to BrowsingContext, and fill it in after reflow. → Bug 1593756 Part 2: Add a field EmbeddingOffset to BrowsingContext, and fill it in after reflow.
Attachment #9108301 - Attachment description: Bug 1593756 Part 2: Add a chrome-only param to GetBoxQuads to return values relative to another browsing context. → Bug 1593756 Part 3: Add a chrome-only param to GetBoxQuads to return values relative to another browsing context.
Attachment #9108302 - Attachment description: Bug 1593756 Part 3: Rename an existing test in anticpation of new sibling tests. → Bug 1593756 Part 4: Rename an existing test in anticpation of new sibling tests.
Attachment #9108303 - Attachment description: Bug 1593756 Part 4: Add a new test of getBoxQuads across cross-process iframe boundaries. → Bug 1593756 Part 5: Add a new test of getBoxQuads across cross-process iframe boundaries.

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.

Attachment #9108301 - Attachment description: Bug 1593756 Part 3: Add a chrome-only param to GetBoxQuads to return values relative to another browsing context. → Bug 1593756 Part 1: Add a chrome-only param to GetBoxQuads to return values relative to the top-level browsing context.
Attachment #9108302 - Attachment description: Bug 1593756 Part 4: Rename an existing test in anticpation of new sibling tests. → Bug 1593756 Part 2: Rename an existing test in anticpation of new sibling tests.
Attachment #9108303 - Attachment description: Bug 1593756 Part 5: Add a new test of getBoxQuads across cross-process iframe boundaries. → Bug 1593756 Part 3: Add a new test of getBoxQuads across cross-process iframe boundaries.
Attachment #9108301 - Attachment description: Bug 1593756 Part 1: Add a chrome-only param to GetBoxQuads to return values relative to the top-level browsing context. → Bug 1593756 Part 1: Add a chrome-only param to GetBoxQuads to return values relative to the window origin.
Attachment #9126022 - Attachment is obsolete: true
Attachment #9125224 - Attachment is obsolete: true

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.

See Also: → 1617154
Attachment #9108301 - Attachment description: Bug 1593756 Part 1: Add a chrome-only param to GetBoxQuads to return values relative to the window origin. → Bug 1593756 Part 1: Add a new method GetBoxQuadsFromWindowOrigin.
Attachment #9108303 - Attachment description: Bug 1593756 Part 3: Add a new test of getBoxQuads across cross-process iframe boundaries. → Bug 1593756 Part 3: Add a test of getBoxQuadsFromWindowOrigin across cross-process iframe boundaries.
Pushed by bwerth@mozilla.com:
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
Regressions: 1619836
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
Duplicate of this bug: 1593645
You need to log in before you can comment on or make changes to this bug.