1867784 - DoorDash site fails to load. Last working in version 104

Status: VERIFIED FIXED

(Core :: Layout: Columns, defect)

Description

[Ali]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:121.0) Gecko/20100101 Firefox/121.0

Steps to reproduce:

Visited a restaurant page on DoorDash
https://www.doordash.com/en-NZ/store/kk-malaysia-cuisine-auckland-25028628/

Actual results:

The page fails to load. Elements are missing, scrolling is barely usable and the page is no longer interactable.

Profile from 121b5:
https://share.firefox.dev/3RyqTfH

Expected results:

The page should load fully, as is the case in version 104 and earlier.
Also tested and working correctly in Edge.

Profile from 104:
https://share.firefox.dev/3Na5czE

Comment 1

[Ali]

Attachment: about-support.txt

Comment 2

[Ada C.]

Hi, Ali

I managed to reproduce this issue on my end, using latest Nightly 122.0a1 (2023-12-06), on Windows 10. I will set a component for this issue and set the status to NEW in order to get our developers involved, if it's not the right component please feel free to change it to an appropriate one.

Comment 3

[Dennis Schubert [:denschub]]

The profile is full of jank, as it's very busy with reflows. I don't know the exact course, but this is something for the performance teams to look into.

Comment 4

[Markus Stange [:mstange]]

It is one very long reflow. https://share.firefox.dev/3NbLZxC

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

This bug has been marked as a regression. Setting status flag for Nightly to affected.

Comment 6

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7cae93f348a7f209c35b545a3b2a18dacc0a8477&tochange=cad5fc248dfe0d6116682b8a1d7ee9cbaeab1763

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

:TYLin, since you are the author of the regressor, bug 793686, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 8

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

It looks like Firefox is trapping in a multi-column layout, possibly creating columns indefinitely. If I open the doordash page in debug build, I see the following log.

[Child 119322: Main Thread]: D/ColumnSet FindBestBalanceBSize: Last attempt to call ReflowColumns
[Child 119322: Main Thread]: D/ColumnSet ReflowColumns: Doing column reflow pass: mLastBalanceBSize=26579, mColBSize=26580, RTL=0, mUsedColCount=2, mColISize=28920, mColGap=960
[Child 119322: Main Thread]: D/ColumnSet ReflowColumns: Reflowing child #1 7fa5fcaa9370: availSize=(28920,26580), kidCBSize=(28920,1073741823), child's mIsTopOfPage=0
[Child 119322: Main Thread]: D/ColumnSet ReflowColumns: Reflowed child #1 7fa5fcaa9370: status=[Complete=N,NIF=Y,Break=N,FirstLetter=N], desiredSize=(28920,26580), childContentBEnd=26580, CarriedOutBEndMargin=0 (ignored)
[Child 119322: Main Thread]: D/ColumnSet ReflowColumns: Next childOrigin.iCoord=29880
[Child 119322: Main Thread]: D/ColumnSet ReflowColumns: Reflowing last column with unconstrained block-size. Change available block-size from 26580 to 1073741823
[Child 119322: Main Thread]: D/ColumnSet ReflowColumns: Reflowing child #2 7fa5f4ca99a0: availSize=(28920,1073741823), kidCBSize=(28920,1073741823), child's mIsTopOfPage=0
[Child 119322, Main Thread] ###!!! ASSERTION: We shouldn't have any incomplete children if the available block-size is unconstrained!: 'childrenStatus.IsFullyComplete() || aAvailableSizeForItems.BSize(flexWM) != NS_UNCONSTRAINEDSIZE', file /home/aethanyc/Projects/gecko/layout/generic/nsFlexContainerFrame.cpp:5815
[Child 119322, Main Thread] ###!!! ASSERTION: Shouldn't be incomplete if availableBSize is UNCONSTRAINED.: 'aReflowInput.AvailableBSize() != NS_UNCONSTRAINEDSIZE', file /home/aethanyc/Projects/gecko/layout/generic/nsBlockFrame.cpp:2281

I'll need to debug it to understand why the columns are incomplete under an unconstrained available block-size.

Set the severity field to S3 since not all doordash page is affected.

Comment 9

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

In the Doordash page below the "Ratings & Reviews", there is a container <div class="sc-8ca4acab-1 juHsrD> with columns: 2 that displays all the reviews. Each review has a container <div class="sc-e0d21f9c-3 fWLDfK"> with break-inside: avoid. So this confirms bug 793686 is the regression since it changed how break-inside:avoid works in multicol layout.

This bug occurs when a nsColumnSetFrame performs the last attempt to reflow columns in a column balancing subroutine. There are abspos decedents that are skipped the reflow due to kidNeedsReflow is false [1], so they report overflow-incomplete status in the tracker.Skip() call [3]. However, we might reflow them in the last column where the available block-size in unconstrained, so they really should be reflowed again and report complete status.

I have a patch to force the reflow for all the columns in this last attempt scenario, which works with the DoorDash link, but I failed to find a reduced testcase with that.

If anyone is able to find a testcase for this, it would be a great help!

[1] https://searchfox.org/mozilla-central/rev/23e7e940337d0e0b29aabe0080e4992d3860c940/layout/generic/nsColumnSetFrame.cpp#1178-1190
[2] https://searchfox.org/mozilla-central/rev/23e7e940337d0e0b29aabe0080e4992d3860c940/layout/generic/nsAbsoluteContainingBlock.cpp#167,173-177
[3] https://searchfox.org/mozilla-central/rev/23e7e940337d0e0b29aabe0080e4992d3860c940/layout/generic/nsAbsoluteContainingBlock.cpp#245

Comment 10

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1867784 - Force reflow all kids in the last column balancing reflow.

The bug occurs because some abspos children are split, but not being reflowed
again in the last column balancing reflow where the available block-size of the
last column might be unconstrained.

This patch makes the callers utilizing ReflowInput::ShouldReflowAllKids()
always reflow in the last column balancing reflow to ensure the correctness of
the layout.

Note: the mIsInLastColumnBalancingReflow flag is inheriting from parent to
child reflow input, but it will stop at the nested nsColumnSetFrame because
the nested one will create its own ReflowConfig::mIsLastBalancingReflow and
assign that flag when creating the reflow input for the children.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/815bf21f166b
Force reflow all kids in the last column balancing reflow. r=layout-reviewers,dholbert

Comment 12

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/815bf21f166b

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:TYLin, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox121 to wontfix.

For more information, please visit BugBot documentation.

Comment 14

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1867784 - Force reflow all kids in the last column balancing reflow.

The bug occurs because some abspos children are split, but not being reflowed
again in the last column balancing reflow where the available block-size of the
last column might be unconstrained.

This patch makes the callers utilizing ReflowInput::ShouldReflowAllKids()
always reflow in the last column balancing reflow to ensure the correctness of
the layout.

Note: the mIsInLastColumnBalancingReflow flag is inheriting from parent to
child reflow input, but it will stop at the nested nsColumnSetFrame because
the nested one will create its own ReflowConfig::mIsLastBalancingReflow and
assign that flag when creating the reflow input for the children.

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

Comment 15

[Phabricator Automation]

Uplift Approval Request

• Steps to reproduce for manual QE testing: See comment 0.
• Fix verified in Nightly: yes
• String changes made/needed: None
• Is Android affected?: yes
• Code covered by automated testing: no
• Explanation of risk level: It is low risk because it makes layout reflow more under this specific scenario, and this doesn't hinder the correctness of the layout.
• User impact if declined: The doordash page in comment 0 is not usable
• Needs manual QE test: yes
• Risk associated with taking this patch: Low

Comment 16

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Reproduced the issue on Firefox 121.0 on macOS 13.6.3 by following the informations provided in Comment 0.

The issue is no longer reproducible on Firefox Nightly 122.0a1 (2023-12-11). Tests were performed on macOS 13.6.3, Windows 11 and Ubuntu 22.04.

Comment 17

[Phabricator Automation]

Uplift Approval Request

• Explanation of risk level: It is low risk because it makes layout reflow more under this specific scenario, and this doesn't hinder the correctness of the layout.
• Fix verified in Nightly: yes
• Code covered by automated testing: no
• Risk associated with taking this patch: Low
• Steps to reproduce for manual QE testing: See comment 0.
• User impact if declined: The doordash page in comment 0 is not usable
• String changes made/needed: None
• Needs manual QE test: yes
• Is Android affected?: yes

Comment 18

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/4afc4359ff0e

Comment 19

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Verified the fix on Firefox 121.0.1 (treeherder build) and the website loads correctly. Tests were performed on macOS 13.6.3, Windows 11 and Ubuntu 22.04.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Added to the 121.0.1 relnotes:

Fixed a hang when loading sites containing column-based layouts under some circumstances

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Did you want to nominate this for ESR approval? It grafts cleanly.

Comment 22

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #21)

Did you want to nominate this for ESR approval? It grafts cleanly.

Yeah, I think the patch is a nice to have for ESR. There is no regression of this bug, and I'm not aware any major changes to multicol layout between ESR 115 and 121 that is a prerequisite of my patch. Will request a ESR uplift shortly.

Comment 23

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 9368011 [details]
Bug 1867784 - Force reflow all kids in the last column balancing reflow.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: The patch is low risk and fixed usability of sites using multi-column layout.
• User impact if declined: The doordash page in comment 0 is not usable.
• Fix Landed on Version: 122 (and uplifted to 121)
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It is low risk because it makes layout reflow more under this specific scenario, and this doesn't hinder the correctness of the layout.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9368011 [details]
Bug 1867784 - Force reflow all kids in the last column balancing reflow.

Approved for 115.8esr.

Comment 25

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/c263e4b3dd95

Comment 26

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Verified the fix on Firefox ESR 115.8.0 and the website loads correctly. Tests were performed on macOS 13.6.4, Windows 11 and Ubuntu 22.04.