Closed Bug 1440537 Opened 2 years ago Closed 4 months ago

Don't propagate style/layout flushes up across docgroup boundaries

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: bzbarsky, Assigned: emilio)

References

(Blocks 1 open bug, Regressed 2 open bugs)

Details

(Whiteboard: [pre-oop-frames])

Attachments

(9 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Generally the stuff in a different docgroup could be in a different process, so we shouldn't synchronously poke it.
For what it's worth, other related things I'd like to do here include bug 1368348 (for which I have https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/71bb0c798686/condition-promote-flush-type lying around) and also https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/71bb0c798686/style-flush-not-to-parent (which I never filed a bug for, and which is probably heavily bitrotted by stylo).
Priority: -- → P3
Whiteboard: [pre-oop-frames]
Assignee: nobody → jwatt
Component: DOM → DOM: Core & HTML
No longer blocks: oop-frames
No longer blocks: fission

Stealing since I already wrote a patch for this in the context of bug 1545516 (I just discovered this bug when I was going to file it to send the patch).

Hopefully you don't mind Jonathan :)

Assignee: jwatt → emilio
Depends on: 1545516

We don't need to flush layout in the parent document if the parent and child
documents can't observe each other.

This will also match our behavior in a Fission world.

I'm not attached to the name of the function, better ideas welcome.

We're going to stop propagating these flushes up since they're not observable by
content and it matches what would happen in a fission world.

This test relies on the parent document layout tree being up-to-date by the time
we run the iframe load handler. Also improve diagnostics in the case the
assertion fails.

Depends on D28217

Just make the iframe to test cloning same-origin using srcdoc, instead of using
SpecialPowers to access cross-origin objects.

Depends on D28300

The iframe test runs on an cross-docgroup iframe, even though chrome JS can
observe it in this test.

This test is relying on the getBoundingClientRect() call below in order to flush
the parent document layout as well, but that's going to stop happening (see the
bug and patch).

Depends on D28301

This flush used to happen from snapshotWindow's innerWidth / innerHeight /
scroll* getters.

Depends on D28302

See Also: → 1218456

This subtest (of test_iframe_sandbox_navigation.html) starts intermittently
failing with my first patch of this bug. It relied on the pres-context being
created when sendMouseEvent is called in order to navigate away (we only
navigate away by clicking a link if there's a link handler).

sendMouseEvent calls getBoundingClientRect() which used to do this. It no longer
does though.

I could make sendMouseEvent do that automatically using SpecialPowers or such,
or from the DOMWindowUtils code, but I think I'd prefer not to do that. This is
the only test that wasn't trivially fixable, and this awkwardness can be removed
when bug 1218456 is fixed.

Same as the previous commit, I could make sendMouseEvent do something fancy /
special, but I'd rather not, since it's trivially fixable in the test.

Depends on D28910

See the rest of the patches in this bug for more context :)

Depends on D28911

Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/08d39c306ffb
Fix test_media_queries to not rely on flushing across docgroup boundaries. r=mats
https://hg.mozilla.org/integration/autoland/rev/e7be3478e08c
Fix an a11y test to not rely on flushing the parent document layout across docgroup boundaries. r=surkov
https://hg.mozilla.org/integration/autoland/rev/40ccb621b5d1
Introduce an explicit flush in text_composition_querycontent.xul. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/a07956b430eb
Fix docshell swap test to not rely on cross-docgroup flushing. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/5b63c5b38dcc
Fix a cross-origin navigation test to navigate reliably. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/b65d6e86de49
Fix a web-extensions test that relies on layout flushes working across cross-origin iframes. r=kmag
https://hg.mozilla.org/integration/autoland/rev/b1ca00276478
Don't unnecessarily use cross-origin iframes on a devtools test. r=pbro
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/f7c391670061
Don't propagate flushes across docgroup boundaries. r=bzbarsky

The actual fix was backed out in comment 13.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

swapFrameLoaders relies on frame information, but doesn't ensure it's
up-to-date.

The test for this (test_swapFrameLoaders.xul) is relying right now on one of
flushes from the inner documents to also flush the parent document and thus
ensure there's a frame created.

With the patch for this bug, that flush no longer propagates to the parent
document, and the test fails because we throw in:

https://searchfox.org/mozilla-central/rev/66086345467c69685434dd1c5177b30a7511b1a5/dom/base/nsFrameLoader.cpp#1634

This API could probably be made to work without that requirement, but it's
probably not worth it. For now just flush.

Hmm. In practical use of this API (tab dragging and the like), will the frame information always be present? It would be unfortunate to have tab drags randomly fail based on frame tree state...

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #17)

Hmm. In practical use of this API (tab dragging and the like), will the frame information always be present? It would be unfortunate to have tab drags randomly fail based on frame tree state...

I suspect in practice it will, though before my patch it won't have that guarantee. My patch adds the flush in the API entry-point, so it ensures that frame information is up-to-date.

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f6766ba4ac77
SwapFrameLoaders should flush frames. r=bzbarsky
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/87f038262ac3
Don't propagate flushes across docgroup boundaries. r=bzbarsky
Status: REOPENED → RESOLVED
Closed: 4 months ago4 months ago
Resolution: --- → FIXED
Regressions: 1547845
Regressions: 1547851
Regressions: 1547841
Regressions: 1547489
Regressions: 1547848
You need to log in before you can comment on or make changes to this bug.