1861259 - Minor refactoring of nsRefreshDriver::Tick

Status: RESOLVED FIXED

(Core :: Layout, task)

Description

[Daniel Holbert [:dholbert]]

Filing this bug to do some refactoring in nsRefreshDriver::Tick, to make that massive function slightly-less-massive and to make debugging a bit more straightforward.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Bug 1861259 part 1: Refactor out a helper function for loop body in nsRefreshDriver::Tick. r?tnikkel

This doesn't affect behavior; it's just a pure refactoring, moving code from
being inline to being in a helper function.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1861259 part 2: Unroll the observer-array-loop in nsRefreshDriver::Tick. r?tnikkel

This is a bit gross, but it helps us make better use of the backtrace to figure
out what's going on in a given crash report.

Depends on D191908

Comment 3

[Daniel Holbert [:dholbert]]

Try (static-analysis to answer the annotation question in part 1, and linux-reftests to get some minimal functional testing, even though this shouldn't change behavior):
https://treeherder.mozilla.org/jobs?repo=try&revision=d04e24ce6e3d25131babb2819c90e194ac71ac1e

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Bug 1861259 part 3: Coalesce redundant array-index checks in nsRefreshDriver. r?tnikkel

This patch doesn't change behavior; it just removes some redundant checks.

(Note that the tight-scoping around nsAutoMicroTask/ReduceAnimations is
important for the RAII object to be torn down at the intended time; but it
doesn't need its own if-check.)

Depends on D191909

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7eddf33b21eb
part 1: Refactor out a helper function for loop body in nsRefreshDriver::Tick. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/e92559c7af9c
part 2: Unroll the observer-array-loop in nsRefreshDriver::Tick. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/f19db7c059b3
part 3: Coalesce redundant array-index checks in nsRefreshDriver. r=tnikkel

Comment 6

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/7eddf33b21eb
https://hg.mozilla.org/mozilla-central/rev/e92559c7af9c
https://hg.mozilla.org/mozilla-central/rev/f19db7c059b3

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: Bug 1861259 part 1: Refactor out a helper function for loop body in nsRefreshDriver::Tick. r=tnikkel

This doesn't affect behavior; it's just a pure refactoring, moving code from
being inline to being in a helper function.

Original Revision: https://phabricator.services.mozilla.com/D191908

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: Bug 1861259 part 2: Unroll the observer-array-loop in nsRefreshDriver::Tick. r?tnikkel

This patch doesn't change behavior.

This is a bit gross, but it helps us make better use of the backtrace to figure
out what's going on in a given crash report.

Original Revision: https://phabricator.services.mozilla.com/D191909

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: Bug 1861259 part 3: Coalesce redundant array-index checks in nsRefreshDriver. r?tnikkel

This patch doesn't change behavior; it just removes some redundant checks.

(Note that the tight-scoping around nsAutoMicroTask/ReduceAnimations is
important for the RAII object to be torn down at the intended time; but it
doesn't need its own if-check.)

Original Revision: https://phabricator.services.mozilla.com/D191911

Comment 11

[Phabricator Automation]

Uplift Approval Request

• Risk associated with taking this patch: Low
• Explanation of risk level: No behavior change; this patch-stack is purely a refatoring of existing code.
• Needs manual QE test: no
• Fix verified in Nightly: yes
• Steps to reproduce for manual QE testing: N/A
• User impact if declined: None
• Code covered by automated testing: yes
• String changes made/needed: None
• Is Android affected?: no

Comment 12

[Ryan VanderMeulen [:RyanVM]]

We should take this on Beta as well if it's wanted on ESR this cycle.

Comment 13

[Daniel Holbert [:dholbert]]

Sounds good. For whatever reasons, the crashes that this might help with are most-prevalent on ESR so that's where I ultimately care about this reaching; but there's no problem having it on beta as well. I'll request uplift there.

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: Bug 1861259 part 1: Refactor out a helper function for loop body in nsRefreshDriver::Tick. r?tnikkel

This doesn't affect behavior; it's just a pure refactoring, moving code from
being inline to being in a helper function.

Original Revision: https://phabricator.services.mozilla.com/D191908

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: Bug 1861259 part 2: Unroll the observer-array-loop in nsRefreshDriver::Tick. r?tnikkel

This patch doesn't change behavior.

This is a bit gross, but it helps us make better use of the backtrace to figure
out what's going on in a given crash report.

Original Revision: https://phabricator.services.mozilla.com/D191909

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: Bug 1861259 part 3: Coalesce redundant array-index checks in nsRefreshDriver. r?tnikkel

This patch doesn't change behavior; it just removes some redundant checks.

(Note that the tight-scoping around nsAutoMicroTask/ReduceAnimations is
important for the RAII object to be torn down at the intended time; but it
doesn't need its own if-check.)

Original Revision: https://phabricator.services.mozilla.com/D191911

Comment 17

[Phabricator Automation]

Uplift Approval Request

• Is Android affected?: no
• String changes made/needed: None
• Code covered by automated testing: yes
• User impact if declined: None
• Steps to reproduce for manual QE testing: N/A
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Explanation of risk level: No behavior change; this patch-stack is purely a refatoring of existing code.
• Risk associated with taking this patch: Low

Comment 18

[Dianna Smith [:diannaS]]

Comment on attachment 9360456 [details]
Bug 1861259 part 1: Refactor out a helper function for loop body in nsRefreshDriver::Tick. r?tnikkel

Approved for 120.0b4

Comment 19

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/f28354147bcf
https://hg.mozilla.org/releases/mozilla-beta/rev/e67b162f7903
https://hg.mozilla.org/releases/mozilla-beta/rev/f7a6096fda1c

Comment 20

[Dianna Smith [:diannaS]]

Comment on attachment 9360450 [details]
Bug 1861259 part 1: Refactor out a helper function for loop body in nsRefreshDriver::Tick. r=tnikkel

Approved for 115.5esr

Comment 21

[Dianna Smith [:diannaS]]

Comment on attachment 9360450 [details]
Bug 1861259 part 1: Refactor out a helper function for loop body in nsRefreshDriver::Tick. r=tnikkel

:dholbert the esr115 patches need to be rebased with esr115 due to conflicts with bug 1830820

Comment 22

[Daniel Holbert [:dholbert]]

(In reply to Dianna Smith [:diannaS] from comment #21)

:dholbert the esr115 patches need to be rebased with esr115 due to conflicts with bug 1830820

Hmm, it's not actually a conflict with that bug; it's rather just that the contextual code is different on ESR115. (The uplift for bug 1830820 actulaly removed a difference from esr115 vs. central, and hence made for a cleaner rebase operation here, not a messier one. But there are still other contextual differences that lando didn't detect when generating the ESR-specific uplift revisions.)

(I don't expect lando/phabricator to be able to resolve those conflicts, but it's odd that it didn't notice them.)

Comment 23

[Daniel Holbert [:dholbert]]

Specifically: esr115-rebase-wise, this snippet has a EvaluateMediaQueriesAndReportChanges() call on mozilla-central (and beta) but not on ESR115, because it was added in bug 1451717:
https://searchfox.org/mozilla-central/rev/4f15c940fa206cb29e4c6992a9d8ba2eb4e7df88/layout/base/nsRefreshDriver.cpp#2619-2621

Here's the equivalent snippet on esr115, missing that middle line (because bug 1451717 didn't land there):
https://searchfox.org/mozilla-esr115/rev/d39c91545f0f5086bb26e2dee46d31ae5344b2b0/layout/base/nsRefreshDriver.cpp#2616-2617

This is ~fine, but it does cause a merge conflict -- the "part 1" patch here is moving that whole chunk of code from one place to another, so that trivially creates a merge conflict. (Trivial for a human to work around, but should have been detected/flagged when generating the lando ESR uplift request, presumably.)

Comment 24

[Daniel Holbert [:dholbert]]

The other merge-conflict is from some telemetry code that we removed on central in https://hg.mozilla.org/mozilla-central/rev/65677e6aa05d#l1.57 but still exists on ESR115.

That change is slightly awkward to rebase around (since it involves some state that straddles code that's now split between two functions).

Given that the lines in question are just for unused telemetry (per the commit message in the commit that removed it from central), it seems like we should just uplift that patch as well, rather than trying to delicately edit this bug's patch to preserve those lines to still collect apparently-unused telemetry.

Comment 25

[Daniel Holbert [:dholbert]]

So I'd like to request ESR115 uplift for bug 1843534's https://phabricator.services.mozilla.com/D185007 specifically [EDIT: here's the esr115 uplift version of that patch] as a prereq to the patch stack here. (It's the patch that's creating a merge conflict per comment 24). It's a pure unused-code-removal (with the removed code being otherwise slightly awkward to work around).

Not sure what the cleanest way to do that is. I'll start out by trying to do a specific uplift request for that patch via moz-phab if I can figure out how to do that without creating uplift-requests for the rest of the patches on that bug...

Comment 26

[Daniel Holbert [:dholbert]]

ESR115 patches (part 1 in particular) is now updated to:
(1) work around the missing line in the code-that's-being-moved-with-no-changes (per comment 23)
(2) be based off of the code-removal referenced in comment 24 - comment 25 (which makes parts 1 and 2 here simpler to rebase & reduces opportunity for human error). I've incorporated that into the ESR115 patch stack via "edit related revisions".

I've verified that the patch stack here now applies cleanly onto up-to-date mozilla-esr115 (with moz-phab patch D191987 --apply-to esr115).

DiannaS: sorry for the trouble; the ball's back in your court, and this is good to land assuming you're OK to approve the uplift request on the supporting code-removal patch.

Comment 27

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #22)

(I don't expect lando/phabricator to be able to resolve those conflicts, but it's odd that it didn't notice them.)

side note: I filed bug 1861780 on making the lando-based uplift process more helpful here, FWIW.

Comment 28

[Dianna Smith [:diannaS]]

Thank you!!!
The only issue i have now is that https://phabricator.services.mozilla.com/D191985 seems to be for m-c instead of esr115. not sure what happened otherwise i could land them all at once base on the dependencies you set. Im gonna take bug 1843534 for now.

nvm i think i fixed it will land shortly

Comment 29

[Daniel Holbert [:dholbert]]

Thanks!

Comment 30

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/3e67c049c9d5
https://hg.mozilla.org/releases/mozilla-esr115/rev/9044fbb8cfd0
https://hg.mozilla.org/releases/mozilla-esr115/rev/b3b57a7219e9

Comment 31

[Mathew Hodson]

(In reply to Dianna Smith [:diannaS] from comment #28)

Thank you!!!
The only issue i have now is that https://phabricator.services.mozilla.com/D191985 seems to be for m-c instead of esr115. not sure what happened otherwise i could land them all at once base on the dependencies you set. Im gonna take bug 1843534 for now.

nvm i think i fixed it will land shortly

The repo was probably set to mozilla-central when the patch was updated due to bug 1828731

Comment 32

[Daniel Holbert [:dholbert]]

Yup, that sounds likely - thanks for the reference. (I did use moz-phab submit when updating the uplift patch here.)