Don't use window.top.promiseDocumentFlushed in inspector.js, since the frame it manipulates can get swapped out
Categories
(DevTools :: Inspector, enhancement, P3)
Tracking
(firefox66 fixed)
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: mconley, Assigned: mconley)
References
Details
Attachments
(2 files)
Consider the following scenario:
- 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
- Suppose some JS running in S1 does the following:
let someElement = document.getElementById("something");
let rect = await window.top.promiseDocumentFlushed(() => {
return someElement.getBoundingClientRect();
});
-
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.
-
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
-
Also suppose that S1 now runs some code so that its DOM is dirtied, and layout and style both need to be flushed.
-
Finally, let's say that W1 finally computed that it and its subframe (now S2) no longer require flushes, and it resolves.
-
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.
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Comment 2•6 years ago
|
||
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.
Assignee | ||
Comment 3•6 years ago
|
||
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.
Updated•6 years ago
|
Assignee | ||
Comment 4•6 years ago
|
||
Updated•6 years ago
|
Comment 6•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f5e32a61d194
https://hg.mozilla.org/mozilla-central/rev/959450489bcf
Updated•6 years ago
|
Description
•