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

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Generally the stuff in a different docgroup could be in a different process, so we shouldn't synchronously poke it.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1440537 - Don't propagate flushes across docgroup boundaries. r=bzbarsky

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.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1440537 - Fix an a11y test to not rely on flushing the parent document layout across docgroup boundaries. r=surkov,eeejay

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

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1440537 - Fix test_media_queries to not rely on flushing across docgroup boundaries. r=mats

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

Depends on D28300

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1440537 - Introduce an explicit flush in text_composition_querycontent.xul. r=masayuki

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1440537 - Fix docshell swap test to not rely on cross-docgroup flushing. r=bzbarsky

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

Depends on D28302

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1440537 - Fix a cross-origin navigation test to navigate reliably. r=bzbarsky

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.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1440537 - Fix a web-extensions test that relies on layout flushes working across cross-origin iframes. r=kmag

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

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1440537 - Don't unnecessarily use cross-origin iframes on a devtools test. r=pbro,gl

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

Depends on D28911

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7c391670061
Don't propagate flushes across docgroup boundaries. r=bzbarsky

Comment 13

[Cristian Brindusan [:cbrindusan]]

Backed out changeset f7c391670061 (Bug 1440537) for mochitest failures at test_swapFrameLoaders.xul.

Backout: https://hg.mozilla.org/integration/autoland/rev/bdc963e3823832db9bc80b35be67206c25e33381

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=f7c39167006152340a7347a16c349bb54a8d1711&selectedJob=243088445

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=243088445&repo=autoland&lineNumber=59477

Comment 14

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/08d39c306ffb
https://hg.mozilla.org/mozilla-central/rev/e7be3478e08c
https://hg.mozilla.org/mozilla-central/rev/40ccb621b5d1
https://hg.mozilla.org/mozilla-central/rev/a07956b430eb
https://hg.mozilla.org/mozilla-central/rev/5b63c5b38dcc
https://hg.mozilla.org/mozilla-central/rev/b65d6e86de49
https://hg.mozilla.org/mozilla-central/rev/b1ca00276478

Comment 15

[Emilio Cobos Álvarez (:emilio)]

The actual fix was backed out in comment 13.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1440537 - SwapFrameLoaders should flush frames. r=smaug

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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

[Emilio Cobos Álvarez (:emilio)]

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6766ba4ac77
SwapFrameLoaders should flush frames. r=bzbarsky

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87f038262ac3
Don't propagate flushes across docgroup boundaries. r=bzbarsky

Comment 21

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/f6766ba4ac77

Comment 22

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/87f038262ac3