1662838 - Remove nsDocumentViewer::CallChildren usage in PausePainting/ResumePainting

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Kris Maglione [:kmag]]

These functions are intended to recursively call themselves on all descendant content viewers, but CallChildren only handles in-process content viewers. I'm not sure whether we need to care about out-of-process content viewers here, but we at least probably do want to call them on in-process content viewers with out-of-process interstitials, which means we probably want to use BrowsingContext::PreOrderWalk.

Comment 1

[Cameron McCormack (:heycam)]

There only seems to be one caller of pausePainting, here:

https://searchfox.org/mozilla-central/rev/b58ca45005fe02077c92779483d1b60e9a49687c/dom/base/test/browser_promiseDocumentFlushed.js#171

and that doesn't involve any OOP subdocuments. And there are no callers to resumePainting. Perhaps we should try to re-write the test to not need this.

Comment 2

[Neha Kochar [:neha]]

Moving to M6c based on comment 1. We should modify the test if it doesn't need this, but we should also fix the definition itself to future-proof any uses of it, or maybe add a disclaimer about where and how to correctly use it?

Comment 3

[Neha Kochar [:neha]]

Cameron, can you fix the test?

Comment 4

[Neha Kochar [:neha]]

Moving to M7 since this is test-only fix.

Comment 5

[Neha Kochar [:neha]]

This is not a Fission blocker but we should get this fixed so we don't accidentally break stuff in the future. Should be trivial to fix. Clearing the Fission flag and the severity so it can be re-triaged and assigned appropriately by the module owner.

Comment 6

[Neha Kochar [:neha]]

Daniel, FYI, in case this didn't yet get your attention. Should be trivial to fix.

Comment 7

[Daniel Holbert [:dholbert]]

Historical note, it looks like we tried to remove pausePainting once before (in bug 1059014 part 3) but we reverted that (bringing it back) because there were some b2g use-cases.

(And then later on, the usage in testcase browser_promiseDocumentFlushed.js was added, in bug 1434376.)

I'll take a closer look at this tomorrow.

Comment 8

[Daniel Holbert [:dholbert]]

So looking at this snippet of the test in question:
https://searchfox.org/mozilla-central/rev/f6ffb71dca9eb491e85aa95042380b2602008b00/dom/base/test/browser_promiseDocumentFlushed.js#159-185

It looks like it uses pausePainting() to make the test more robust (checking what happens if we happen to do something before a refresh driver tick fires -- and conveniently, pausePainting() prevents refresh diver ticks from firing.

Given that the pausePainting() API is going away (due to having only this one caller, and not being fission-friendly), I suspect we can just remove this call. This will mean the test becomes a bit less robust -- i.e. less reliable at catching the regression that this piece was trying to test for -- but it will probably still test this condition some of the time. (And in any case, I don't know that the thing we're testing for there is worth maintaining a special-purpose layout API for.)

mconley, I'll check my thinking here with you, since you wrote the test -- does this sound reasonable to you?

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: Bug 1662838 part 1: Adjust browser_promiseDocumentFlushed.js to use advanceTimeAndRefresh() instead of pausePainting(), as a way to pause the refresh driver. r?mconley,tnikkel

The test doesn't seem to depend on this pausePainting() call in order to
pass; I think it only depends on this call in order to make its tested
conditions more robust/strict. The pausePainting() API is going away, though
(in a later patch in this series), so this test can't rely on it anymore.

Hopefully this test will still be able to detect the same sorts of regressions
that it could before; they'll just manifest as intermittent failures rather
than reliable failures.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: Bug 1662838 part 2: Remove nsIContentViewer's unused pausePainting() and resumePainting() APIs. r?tnikkel

These APIs are entirely unused (aside from one usage in a test, which part 1 in
this patch series removed), so this patch shouldn't impact behavior at all.

Historical note: we briefly removed these APIs once before, in this commit:
https://hg.mozilla.org/mozilla-central/rev/c216ff19d690
...but we brought them back because we had a motivating use case at the time.
We don't have any such motivating use cases any more, though. So, this patch
here is essentially a modernized version of that older commit.

Depends on D108147

Comment 11

[Mike Conley (:mconley) (:⚙️) (PTO Aug 24-28)]

I think it makes sense to remove a bunch of outdated / hard-to-maintain machinery if it's only supporting this one test, but I have some questions:

1. Since promiseDocumentFlushed is only exposed to the parent process and never called within iframes (in-process or otherwise), does that change any of the calculus here? Could we keep the pausePainting API for just the parent process browser UI?
2. If not (1), then is there any other way we can increase the likelihood that a paint won't occur before closing the window in the test?
3. If not (1) or (2), then yes, let's go ahead and remove this, but maybe add a comment in the test saying that it's intentionally race-y.

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos) from comment #11)

1. [...] Could we keep the pausePainting API for just the parent process browser UI?

Yeah, I suppose that's an option. So then we'd just add an "is this the parent doucment" check in the various functions here, and bail out if we're not, I guess. With that, the behavior should be consistent between fission & non-fission, which is the primary goal here, I think. I'll explore this option later today.

(It does feel odd to keep this API around to support just a single test's usage, but I also admit I haven't thoroughly read the test or grokked its implications, so maybe it's sufficiently valuable to merit this.)

is there any other way we can increase the likelihood that a paint won't occur before closing the window in the test?

I don't know. (tnikkel had a suggestion in https://phabricator.services.mozilla.com/D108147 that sometimes we put things in background tabs to get their refresh drivers to pause, but I don't think we can do that for an actual browser-window.)

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: Bug 1662838: Restrict nsIContentViewer::pausePainting() and resumePainting() APIs to be parent-process-only. r?tnikkel

Note that pausePainting() only has a single caller, which is inside of a
browser-chrome mochitest that uses the API to predictably test a particular
scenario.

resumePainting() doesn't actually have any callers right now, so we could
conceivably just remove it, too. But we might as well keep it around for
symmetry, as long as pausePainting() needs to exist.

Comment 14

[Daniel Holbert [:dholbert]]

Thanks for the reviews!
Here's a try run, which looks good aside from a few oranges that seem to be unrelated intermittents:
https://treeherder.mozilla.org/jobs?repo=try&revision=fd64fe7832534395518d318f7751dd4a9d601929

I'll go ahead and land.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16475ea5d03c
part 1: Adjust browser_promiseDocumentFlushed.js to use advanceTimeAndRefresh() instead of pausePainting(), as a way to pause the refresh driver. r=mconley,tnikkel
https://hg.mozilla.org/integration/autoland/rev/d1a29802e1be
part 2: Remove nsIContentViewer's unused pausePainting() and resumePainting() APIs. r=tnikkel

Comment 16

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/16475ea5d03c
https://hg.mozilla.org/mozilla-central/rev/d1a29802e1be