1351924 - Use nsChangeHint_UpdateComputedBSize to properly set the IsBResize() flag on reflow input

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Right now we are kind of guessing in ReflowInput::InitResizeFlags whether our computed "auto" value means we actually have a block-direction resize or not.  For that matter, we're guessing whether our computed _non-auto_ value means we have a block-direction resize or not.

The right answer, per discussion with dbaron today, is most likely along the lines of "we have a block-direction resize if we had a style change with the nsChangeHint_UpdateComputedBSize hint" or so.  The question is how to feed that information into here.

My gut feeling is that fixing this could fix various cases (e.g. bug 1348848) where we're reflowing too much right now.

Jet, do you know who might have time to look into this?

Comment 1

[Daniel Holbert [:dholbert]]

I'm intermittently away for the next few weeks, but I'm around next week & am hoping to get a chance to investigate this a bit then.

Comment 2

[Daniel Holbert [:dholbert]]

Sorry that I haven't gotten much substantial done here yet -- I'm bumping this to qf:p2 since I'm not convinced this needs to block 57.  I may still circle back to this for 57, though.

Comment 3

[Daniel Holbert [:dholbert]]

Unassigning to reflect reality, as I haven't gotten to this yet and realistically won't get to it soon (as I've changed teams, am prepping to manage some interns, and have code-reviews & high-priority flexbox perf work to catch up on, and I've got a vacation coming up in a couple weeks).

I believe this is still ~high-priority, though, in part for perf reasons (e.g. bug 1348848 per bz's comment 0 here), and in part because Tyson says fuzzers are quite frequently tripping over a bug that seems to be a version of this: bug 1403656.

Maire, could you find someone to work on this?

Comment 4

[Maire Reavy [:mreavy]]

(In reply to Daniel Holbert [:dholbert] (away 4/24 - 5/11) from comment #3)
> Unassigning to reflect reality, as I haven't gotten to this yet and
> realistically won't get to it soon (as I've changed teams, am prepping to
> manage some interns, and have code-reviews & high-priority flexbox perf work
> to catch up on, and I've got a vacation coming up in a couple weeks).
> 
> I believe this is still ~high-priority, though, in part for perf reasons
> (e.g. bug 1348848 per bz's comment 0 here), and in part because Tyson says
> fuzzers are quite frequently tripping over a bug that seems to be a version
> of this: bug 1403656.
> 
> Maire, could you find someone to work on this?

Sure, Daniel.  Thanks for the needinfo.  

Cameron -- Can you give me a (rough) assessment of the level-of-effort here and whether this could be a good "first bug" for some of our senior platform engineers who are new to Layout?  Also, this is marked as a P1 perf/quantum flow bug.  Do you agree that's the right priority (relative to other perf work we could do)? Even if it's not, I'm inclined to prioritize this one relatively high because it's slowing down Tyson.  I welcome your thoughts/feedback on all of this. Thanks!

Comment 5

[Cameron McCormack (:heycam)]

Yes, I think we should prioritize this; bug 1348848 is a high profile, table-layout-heavy site, and this kind of table layout is not uncommon.  For someone who's happy to dive into unfamiliar reflow code, it would be a good moderate difficulty first layout bug.

Comment 6

[Maire Reavy [:mreavy]]

Moving to a P2 so it doesn't fall off our radar for prioritizing, but I'll likely create a whiteboard tag for bugs like this that we want to work on soon.

Comment 7

[Daniel Almeida]

Well, asking does not hurt: I'm new, is this feasible for me? Can anybody mentor?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Are you happy to dive into unfamiliar, somewhat complicated code?  Especially when no one is really 100% an expert on it?

I can probably mentor.  I suspect dholbert might be able to as well, once he's back from vacation.

Comment 9

[Daniel Holbert [:dholbert]]

I think we do also have some newly-transferred-over layout engineers who are looking for projects to work on -- this is probably better-suited to one of them, with moderate experience in adjacent areas of Firefox code.

@Daniel de Almeida: you might consider looking at bug 1452829 (as-yet unassigned), if you're looking for a "first bug" to start on! That's an optimization that should be pretty straightforward.

Comment 10

[Daniel Almeida]

daniel/boris: I wasn't sure whether 'good first layout bug' in this case meant good for beginners or good for seasoned people already working on other parts of firefox, so now I guess it's the latter!

I'll look into bug 1452829

thanks for the input.

Comment 11

[Wayne Mery (:wsmwk)]

potentially contributes to crashes according to bug 1403656, so => major

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

I am moving onto WebRender, so I won't be able to complete this. I will upload my patch and testcase. With dbaron's help I managed to discover and fix a correctness issue, but that didn't help with performance here.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Work in progress patch; fixes correctness issue in testcase, not perf issue

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Testcase for correctness issue

This is the testcase dbaron proposed at the Sydney 2018 layout work week which identified a correctness bug. My patch fixed the correctness issue, but turned out to not fix the perf issue.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

This came up again in bug 1482938, which may be a duplicate.  While it is too late to fix in 63, we could still take a patch for 65 nightly.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Fixing this may also fix bug 1403656.  Cam, can you take a look or help find someone to investigate?

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late to fix in 64. Marking this issue as fix-optional for 65; if you land a patch in nightly and think it's low-risk for beta, please request uplift.

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: cpearce's WIP patch, rebased to tip

Here's the same WIP patch (attachment 9002997 [details] [diff] [review]), rebased to tip.

I tested to see if it helps with bug 1403656, but unfortunately it does not (the latest testcase there still crashes in a build with this rebased patch). Not entirely surprising, given comment 14's note about this not (yet) fixing perf issues.

Comment 19

[Sean Voisen (:svoisen)]

Going to get this on our backlog for Q3.

Comment 20

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

I merged this to tip again, made a bunch of revisions, and pushed to try to see what things look like.

Comment 21

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

And for comparison, here's a try run before my additional changes. If the differences between the runs are interesting, it might be worth separating out the change from ConvertsToPercentage() to HasPercent() (which I think is obviously correct) from the other changes.

Comment 22

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

Those two try runs had an identical set of failures: multicol-zero-height-001.xht (two copies), layout/tables/reftests/1031934.html, and layout/reftests/bugs/1263845.html.

Comment 23

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

The failure of layout/tables/reftests/1031934.html is, I think, a separate incremental reflow bug that needs to be fixed. The failure (a) goes away if I add html { overflow-y: scroll } to suppress the horizontal resize that happens when we initially try reflowing with a scrollbar and (b) happens when (with the patch only) I resize the width of the page. I suspect there's something in the handling of visibility:collapse that assumes that the entire table has been reflowed and goes wrong if it hasn't been.

Comment 24

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

At this point I've nearly fixed all of the test failures (2 of the 3 as separate bugs, one of which I still need to write the patch for but I'm pretty sure I know what will work; 1 as a revision to the patch here). There's still the issue that this doesn't fix one of the performance problems it was expected to fix. It does, however, fix some correctness issues and I might try to get it landed once I clean up the patch and write the necessary tests.

Comment 25

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

On top of the fixes for bug 1564308 and bug 1564649, I believe this will now pass all tests.

Although I don't think (though I should double-check with the latest revisions, but I think it's unlikely to have changed) this fixes the bug (bug 1348848) that it was intended to fix, it does:

• fix known correctness bugs
• optimize cases to reflow less where we were previously reflowing more than needed (as shown by the fact that those optimizations uncovered two existing bugs shown in our existing tests -- the ones listed in the previous paragraph -- where I could modify the tests so they showed the bugs today)

Thus I think it's worth landing this as it is so far, and then digging in to bug 1348848 as a followup.

Comment 26

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

Attachment: Bug 1351924 - Add reftests for correctness bug fixed by this bug.

Co-authored-by: L. David Baron <[e-mail]>
Co-authored-by: Chris Pearce <[e-mail]>

Comment 27

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

Attachment: Bug 1351924 - Keep separate flags for whether block-size has changed and whether percentages derived from the block-size have changed, and make better decisions about what needs reflow.

This reduces a bit of code complexity, fixes bugs where we weren't
reflowing enough, and optimizes additional cases that we couldn't
optimize in the past.

Co-authored-by: Chris Pearce <[e-mail]>
Co-authored-by: L. David Baron <[e-mail]>

Depends on D37609

Comment 28

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

Oh, and the test fix that was part of this bug rather than a separate bug was the fix for layout/reftests/bugs/1263845.html which I ended up fixing by moving the clearing of the flag to nsFrame::DidReflow. For a description of that test fix, see the comments in the main patch in both ReflowInput::InitResizeFlags (referencing nsBlockReflowContext::ComputeCollapsedBStartMargin, which was the problem in that case) and nsFrame::DidReflow.

Comment 29

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

Here's a new try run rebased on top of the dependent bugs.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/27bcd63fde99
Add reftests for correctness bug fixed by this bug. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/05f675735d39
Keep separate flags for whether block-size has changed and whether percentages derived from the block-size have changed, and make better decisions about what needs reflow. r=dholbert

Comment 31

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

Also, for the record, I should note that I believe that on 32-bit systems this patch increased the size of nsIFrame by a word. If I'm looking at things correctly, it shouldn't have changed anything on 64-bit systems, though. I'm not aware of spare bits we have elsewhere, though, and I've been saying for a while that we shouldn't be overly conservative about adding 1-bit member variables to nsIFrame despite that at some point (now) it will increase the size of frames by a word.

Comment 32

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

Oops, I think it increased the size by a word on both 32-bit and 64-bit. (I misread mOverflow as a struct when it's actually a union.)

Comment 33

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/27bcd63fde99
https://hg.mozilla.org/mozilla-central/rev/05f675735d39

Comment 35

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

I filed bug 1586175 to back this bug out for Firefox 70 (only) in order to give us more time to fix regression bug 1584018.