Closed Bug 1519407 Opened 5 years ago Closed 5 years ago

Don't use window.top.promiseDocumentFlushed in inspector.js, since the frame it manipulates can get swapped out

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox66 fixed)

RESOLVED FIXED
Firefox 66
Tracking Status
firefox66 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

Details

Attachments

(2 files)

Consider the following scenario:

  1. Two browser chrome windows (W1 and W2), each with a non-remote subframe (S1 is framed by W1 and S2 is framed by W2)

W1
|--S1

W2
|--S2

  1. Suppose some JS running in S1 does the following:
let someElement = document.getElementById("something");
let rect = await window.top.promiseDocumentFlushed(() => {
  return someElement.getBoundingClientRect();
});
  1. Since window.top refers, in this case to W1, when W1 runs promiseDocumentFlushed, suppose it finds that there are pending layout or style flushes, so it sets up its refresh driver observer to wait for a clean set of frames where not require flushes.

  2. Now, here's the kicker: suppose, during this window of time where promiseDocumentFlushed still has not yet resolved, some other code runs that swaps S1 with S2. Now we have this arrangement:

W1
|--S2

W2
|--S1

  1. Also suppose that S1 now runs some code so that its DOM is dirtied, and layout and style both need to be flushed.

  2. Finally, let's say that W1 finally computed that it and its subframe (now S2) no longer require flushes, and it resolves.

  3. The callback will fire, but is executing in the scope of S1, which is dirty, and so we'll incur a layout flush (because of 5), even though we're running inside a promiseDocumentFlushed callback.

So chrome document reparenting here is breaking promiseDocumentFlushed.

Speaking with ehsan, we think the best solution here might be to iterate any pending promiseDocumentFlushed promises inside a subframe that's being swapped, and cause them all to be rejected.

Blocks: 1441168
Instead of using promiseDocumentFlushed, we optimistically use getBoundsWithoutFlushing to
get a recent width for the inspector-splitter-box without flushing, which is probably
sufficient to determine whether or not to enter landscape mode.

So I've thought about this more, and now I'm kinda doubtful that rejecting any in-flight promiseDocumentFlushed Promises during a frameloader swap is the right move here.

I think there are still legitimate reasons why one might want to use promiseDocumentFlushed during a frameloader swap, where it should resolve properly.

The use case we're trying to prevent is if the thing that's getting swapped out is what's being queried by the promiseDocumentFlushed callback - and only if it had asked its parent to run promiseDocumentFlushed for it before running the callback.

Maybe I'm wrong, and it's the right move to just unilaterally reject the promiseDocumentFlushed Promises on any and all nsGlobalWindowInner's that have children participating in a frameloader swap, but for now that feels a bit heavy handed and non-obvious to me.

So I'm going to re-summarize this bug, and add some additional documentation to the promiseDocumentFlushed Window.webidl commentary, as well as the MDN wiki page that mentions its usage.

Component: DOM → Inspector
Product: Core → DevTools
Summary: All pending promiseDocumentFlushed promises for an inner window global should reject during a DocShell swap → Don't use window.top.promiseDocumentFlushed in inspector.js, since the frame it manipulates can get swapped out

To expand on comment 2, one of the cases I'm thinking of is this:

Suppose again we have our two windows and their subframes.

W1
|--S1

W2
|--S2

Now suppose W1 gets window.promiseDocumentFlushed on it, while there are still pending layout flushes. Suppose that the callback is primarily concerned with querying content within W1, and does not care about the content of S1. Suppose also that the promiseDocumentFlushed callback does not yet fire because there are pending layout flushes.

Not suppose that while we wait for the refresh drivers to tick forward and for layout to flush, that other completely unrelated JS code runs that swaps S1 and S2. With the original intention of this bug, we'd have rejected the window.promiseDocumentFlushed Promise, even though in this case, the callback (which doesn't query the S1 subframe at all).

Rejecting the Promise int hat case doesn't seem at all right to me.

I'm afraid this is one of those places where I think users of the promiseDocumentFlushed callback have to be aware if the things that are being touched within the callback might have been swapped to other windows.

Attachment #9036025 - Attachment description: Bug 1519407 - Don't use promiseDocumentFlushed inside a frame that can be reparented. r?gl → Bug 1519407 - Don't use promiseDocumentFlushed inside a frame that can be reparented. r=gl
Priority: -- → P3
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f5e32a61d194
Don't use promiseDocumentFlushed inside a frame that can be reparented. r=gl
https://hg.mozilla.org/integration/autoland/rev/959450489bcf
Add documentation to Window.webidl about promiseDocumentFlushed and subframe swaps. r=Ehsan
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Assignee: nobody → mconley
You need to log in before you can comment on or make changes to this bug.