Closed Bug 1867784 Opened 7 months ago Closed 7 months ago

DoorDash site fails to load. Last working in version 104

Categories

(Core :: Layout: Columns, defect)

Product:
Core
Component:
Layout: Columns
Version:
Firefox 121
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
122 Branch
Tracking Flags:
Tracking Status
relnote-firefox --- 121+
firefox-esr115 --- verified
firefox120 --- wontfix
firefox121 --- verified
firefox122 --- verified

People

(Reporter: ali.nz2005, Assigned: TYLin)

References

(Regression)

Details

(5 keywords)

Attachments

(3 files)

about-support.txt
35.48 KB, text/plain
Details
Bug 1867784 - Force reflow all kids in the last column balancing reflow.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1867784 - Force reflow all kids in the last column balancing reflow.
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-release+
RyanVM
: approval-mozilla-esr115+
Details | Review

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

Attached file about-support.txt —

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.

Status: UNCONFIRMED → NEW
Component: Untriaged → Desktop
Ever confirmed: true
Product: Firefox → Web Compatibility

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.

Component: Desktop → Performance
Product: Web Compatibility → Core
Component: Performance → Layout: Flexbox
Keywords: regression, regressionwindow-wanted
Performance Impact: --- → ?

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

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

status-firefox122: --- → affected
Keywords: hang, perf

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

Flags: needinfo?(aethanyc)

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.

Severity: -- → S3

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

Assignee: nobody → aethanyc
Performance Impact: ? → ---
Component: Layout: Flexbox → Layout: Columns
Flags: needinfo?(aethanyc)
Keywords: testcase-wanted

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.

Pushed by aethanyc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/815bf21f166b
Force reflow all kids in the last column balancing reflow. r=layout-reviewers,dholbert

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

Status: NEW → RESOLVED
Closed: 7 months ago
status-firefox122: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 122 Branch

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.

Flags: needinfo?(aethanyc)

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

Attachment #9368011 - Flags: approval-mozilla-beta?
Flags: needinfo?(aethanyc)
Attachment #9368011 - Flags: approval-mozilla-release?
Attachment #9368011 - Flags: approval-mozilla-beta?
QA Whiteboard: [qa-triaged]

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.

status-firefox122: fixed → verified
Flags: qe-verify+

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
Flags: qe-verify+
Attachment #9368011 - Flags: approval-mozilla-release? → approval-mozilla-release+

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.

status-firefox121: fixed → verified
Flags: qe-verify+

Added to the 121.0.1 relnotes:

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

relnote-firefox: --- → 121+

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

Flags: needinfo?(aethanyc)

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

Flags: needinfo?(aethanyc)

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.
Attachment #9368011 - Flags: approval-mozilla-esr115?

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

Approved for 115.8esr.

Attachment #9368011 - Flags: approval-mozilla-esr115? → approval-mozilla-esr115+

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.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
status-firefox-esr115: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: