Closed Bug 1686603 Opened 5 years ago Closed 5 years ago

Implement a mechanism to override style sizes when constructing ReflowInput

Categories

(Core :: Layout, task)

Product:
Core
Component:
Layout
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
87 Branch
Tracking Flags:
Tracking Status
firefox87 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

Attachments

(8 files)

Bug 1686603 Part 1 - Remove unused ReflowInput::GetContainingBlockContentISize().
48 bytes, text/x-phabricator-request
Details | Review
Bug 1686603 Part 2 - Add StyleSizeOverrides parameter to ReflowInput's constructor & co. to override data from style system data.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1686603 Part 3 - Extract a helper method AxisTracker::IsInlineAxisMainAxis.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1686603 Part 4 - Use StyleSizeOverrides to revise flex base size resolution.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1686603 Part 5 - Use StyleSizeOverrides to override flex main size when main size can influence cross size.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1686603 Part 6 - Use StyleSizeOverrides to override flex item's sizes in its final reflow.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1686603 Part 7 - Change two StyleSize variables from pointers to references, and rename them.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1686603 Part 8 - Update test expectations.
48 bytes, text/x-phabricator-request
Details | Review

Currently, when we want to compute sizes for a child frame by using certain restrictions (like treating block-size as 'auto'), or just want to tell the child frame to use a computed size, we use some customize code and ComputeSizeFlag in nsIFrame::ComputeSize() or nsContainerFrame::ComputeSizeWithIntrinsicDimensions(). Or we just build a ReflowInput and throw away computed inline-size or block-size by setting them afterwards.

I can think of some use cases that can be easier to implement if we have a mechanism to override style sizes when constructing ReflowInput.

  1. When resolving flex base size, we might need to replace the preferred main size with flex-basis property. Currently, we have similar logic in both nsIFrame::ComputeSize() and nsContainerFrame::ComputeSizeWithIntrinsicDimensions().
  2. When measuring flex / grid item's content block-size, we use UseAutoBSize flag.
  3. Flex container can override main size & cross size for flex item.

I'm planning to add a struct containing StyleSizes to ReflowInput's constructor, nsIFrame::ComputeSize, etc, so a parent frame can have a easy way to tell a child to compute the size in a desirable way, and convert a few use case in flex container as a proof. There might be more opportunities to use the mechanism. Any callsite of SetComputedBSize may be a good candidate.

This patch adds the struct as a parameter to various functions.

The struct is cached in ReflowInput so that we don't need to pass it
down to the internal method where nsIFrame::ComputeSize() is called.

In the subsequent patches, we'll use it to revise the implementation of
flex container's flex base size resolution, and size overrides.

Depends on D101791

FlexItem::StyleCrossSize() hasn't been used in this patch, but it will
be used in a later patch that revises size overrides in flex item's
final reflow.

Depends on D101795

We don't need to tweak ReflowIpnut flags as much as we did because
ReflowInput::InitResizeFlags().

Depends on D101796

ComputeSizeFlag::UseAutoISize and ComputeSizeFlag::UseAutoBSize [1] can also be converted by using StyleSizeOverride at callsites, but the patch stack is getting large, so I decide to stop here. They can be done in other bugs assuming the idea of StyleSizeOverride sounds good in general.

[1] https://searchfox.org/mozilla-central/rev/2a24205479519e70c0574929f45730d285141584/layout/base/LayoutConstants.h#47-61

Blocks: 1674302

One high-level thing that feels a bit confusing to me... What sort of guarantees do we have here (or intend to ultimately have) about when/where these override args are used (in place of the style-system values)?

There are many functions whose signatures are being modified in part 2, but it doesn't look like we're changing all of their implementations, so I'm guessing that a lot of them just ignore these override values and probably still use the values from the style system -- is that right?

If so: that ignoring might be fine for the way we're using the override values right now (to simplify some flex layout sizing stuff), but it makes these functions a bit more confusing / unpredictable, perhaps... I'm curious if you have thoughts on this, or if I'm misunderstanding this to some extent. (Maybe this could be addressed to some extent with clearer documentation about what sorts of sizing operations will & won't use these values...)

Flags: needinfo?(aethanyc)

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

One high-level thing that feels a bit confusing to me... What sort of guarantees do we have here (or intend to ultimately have) about when/where these override args are used (in place of the style-system values)?

There are many functions whose signatures are being modified in part 2, but it doesn't look like we're changing all of their implementations, so I'm guessing that a lot of them just ignore these override values and probably still use the values from the style system -- is that right?

All the ComputeSize and ComputeAutoSize implementation that access inline and block sizes from style system should be audited, and should use the sizes provided by StyleSizeOverride instead. Part 4 should already audit all of them.

But you are right that many replaced elements override ComputeAutoSize() to provide their intrinsic size, and they don't look at style inline size, or ComputeSizeFlag::{UseAutoISize,UseAutoBSize}. So we are already kind of in the same situation with the ComputeSizeFlag.

If so: that ignoring maybe fine for the way we're using the override values right now (to simplify some flex layout sizing stuff), but it makes these functions a bit more confusing / unpredictable, perhaps... I'm curious if you have thoughts on this, or if I'm misunderstanding this to some extent. (Maybe this could be addressed to some extent with clearer documentation about what sorts of sizing operations will & won't use these values...)

Ideally, I would only add the struct StyleSizeOverride to ReflowInput and ComputeSize, but not to ComputeAutoSize. However, I discovered two issues while writing my patches.

  1. Due to the current contract of nsIFrame::ComputeAutoSize, the implementation may return garbage inline-size if StylePosition()->ISize() is not 'auto' [1]. So we still need to add StyleSizeOverride to make them compute inline-size if the caller wants to override inline-size with 'auto' value.
  2. Also, table wrapper's ComputeAutoSize can call table frame's ComputeSize via ChildShrinkWrapISize() [2]. If we want to let the size overrides to table wrapper really affects internal table, this seems necessary unless we want to have table wrapper to override ComputeSize instead.

And I agree that it's confusion to add StyleSizeOverride to ComputeAutoSize while most of the implementation don't even take a look at it. I wonder whether it makes senses to disallow the optimization in 1) and change the implementation of table wrapper's so that we don't need to provide StyleSizeOverride to ComputeAutoSize? Note 2) is just a preparation towards fixing <table> as a flex item, and doesn't need to happen in this bug.

[1] https://searchfox.org/mozilla-central/rev/03224522336f60a1a61a87e1fcd4feb0a0315a9b/layout/generic/nsIFrame.h#2773-2775
[2] https://searchfox.org/mozilla-central/rev/03224522336f60a1a61a87e1fcd4feb0a0315a9b/layout/tables/nsTableWrapperFrame.cpp#386-387

Flags: needinfo?(aethanyc) → needinfo?(dholbert)

FWIW, the only caller for nsIFrame::ComputeAutoSize is in nsIFrame::ComputeSize and it's called unconditionally, so it makes sense for implementation of ComputeAutoSize to skip calculating inline size if nsIFrame::ComputeSize won't use the value at all. Only nsContainerFrame and nsTextControlFrame have this optimization though.

Thanks, that makes sense. I think this approach is fine, as long as things are clearly documented. (RE the optimization that you mentioned: I would bet we lean on the nsContainerFrame "return garbage" optimization, to avoid computing the intrinsic widths of subtrees when we don't need to; so we probably shouldn't change that contract for item (1) at this point.)

BTW, it looks like your Part 1 patch needs a rebase, due to some contextual code (InitFrameType()) having been recently removed in bug 1686310.

Flags: needinfo?(dholbert)

After Part 5 & Part 6, we don't need to override flex item's flex-basis
proprety or preferred main size in ComputeSize() and
ComputeSizeWithIntrinsicDimensions(), we can change the two style sizes
to references, and give both of them to shorter names.

Depends on D101797

Pushed by aethanyc@gmail.com: https://hg.mozilla.org/integration/autoland/rev/b1328ee45f43 Part 1 - Remove unused ReflowInput::GetContainingBlockContentISize(). r=emilio https://hg.mozilla.org/integration/autoland/rev/965dbe8de1e1 Part 2 - Add StyleSizeOverrides parameter to ReflowInput's constructor & co. to override data from style system data. r=dholbert https://hg.mozilla.org/integration/autoland/rev/8766ce79ba5e Part 3 - Extract a helper method AxisTracker::IsInlineAxisMainAxis. r=dholbert https://hg.mozilla.org/integration/autoland/rev/63020922e2fd Part 4 - Use StyleSizeOverrides to revise flex base size resolution. r=dholbert https://hg.mozilla.org/integration/autoland/rev/f1f672cdd0c6 Part 5 - Use StyleSizeOverrides to override flex main size when main size can influence cross size. r=dholbert https://hg.mozilla.org/integration/autoland/rev/3a0d0155ea5e Part 6 - Use StyleSizeOverrides to override flex item's sizes in its final reflow. r=dholbert https://hg.mozilla.org/integration/autoland/rev/19cba7d34a7f Part 7 - Change two StyleSize variables from pointers to references, and rename them. r=emilio

See bug 1688115 for the description of an existing bug.

Depends on D102475

Part 8 adjusts the assertion count seen in comment 14.

Blocks: 1688115
Flags: needinfo?(aethanyc)
Pushed by aethanyc@gmail.com: https://hg.mozilla.org/integration/autoland/rev/fd1d95a1e706 Part 1 - Remove unused ReflowInput::GetContainingBlockContentISize(). r=emilio https://hg.mozilla.org/integration/autoland/rev/c82593b13a61 Part 2 - Add StyleSizeOverrides parameter to ReflowInput's constructor & co. to override data from style system data. r=dholbert https://hg.mozilla.org/integration/autoland/rev/7717a0f2a37b Part 3 - Extract a helper method AxisTracker::IsInlineAxisMainAxis. r=dholbert https://hg.mozilla.org/integration/autoland/rev/cf7a55638aef Part 4 - Use StyleSizeOverrides to revise flex base size resolution. r=dholbert https://hg.mozilla.org/integration/autoland/rev/7013b95266ae Part 5 - Use StyleSizeOverrides to override flex main size when main size can influence cross size. r=dholbert https://hg.mozilla.org/integration/autoland/rev/4c9271f07178 Part 6 - Use StyleSizeOverrides to override flex item's sizes in its final reflow. r=dholbert https://hg.mozilla.org/integration/autoland/rev/e7edf6fcc41d Part 7 - Change two StyleSize variables from pointers to references, and rename them. r=emilio https://hg.mozilla.org/integration/autoland/rev/94b5d0986d27 Part 8 - Update test expectations. r=dholbert

Backed out for causing crashtest failures

backout: https://hg.mozilla.org/integration/autoland/rev/f8446e5139b64669581d5ba6667ea74f4a57ff87

push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=U6z6lKELQr6skeApIDsiKQ.1&searchStr=linux%2C18.04%2Cx64%2Casan%2Copt%2Creftests%2Ctest-linux1804-64-asan%2Fopt-crashtest-e10s%2Cc&revision=94b5d0986d2778c7a0b936ed0d94ef7e0ee1b899

failure log: https://treeherder.mozilla.org/logviewer?job_id=327780914&repo=autoland&lineNumber=18094

[task 2021-01-26T02:19:52.574Z] 02:19:52 INFO - REFTEST TEST-START | layout/style/crashtests/1383981.html
[task 2021-01-26T02:19:52.575Z] 02:19:52 INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/reftest/tests/layout/style/crashtests/1383981.html | 3176 / 3880 (81%)
[task 2021-01-26T02:19:52.713Z] 02:19:52 INFO - AddressSanitizer:DEADLYSIGNAL
[task 2021-01-26T02:19:52.714Z] 02:19:52 INFO - =================================================================
[task 2021-01-26T02:19:52.715Z] 02:19:52 ERROR - ==1663==ERROR: AddressSanitizer: stack-overflow on address 0x7fff0b426d48 (pc 0x55a83d609056 bp 0x7fff0b427590 sp 0x7fff0b426d50 T0)
[task 2021-01-26T02:19:52.879Z] 02:19:52 INFO - #0 0x55a83d609056 in __asan_memcpy /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:22:3
[task 2021-01-26T02:19:55.635Z] 02:19:55 INFO - #1 0x7f41abfcbb54 in GetTransform /builds/worker/workspace/obj-build/dist/include/mozilla/gfx/2D.h:1580:40
[task 2021-01-26T02:19:55.635Z] 02:19:55 INFO - #2 0x7f41abfcbb54 in gfxFont::GetRoundOffsetsToPixels(mozilla::gfx::DrawTarget*) /builds/worker/checkouts/gecko/gfx/thebes/gfxFont.cpp:847:20
[task 2021-01-26T02:19:55.635Z] 02:19:55 INFO - #3 0x7f41abfe1e62 in bool gfxFont::SplitAndInitTextRun<unsigned char>(mozilla::gfx::DrawTarget*, gfxTextRun*, unsigned char const*, unsigned int, unsigned int, mozilla::unicode::Script, nsAtom*, mozilla::gfx::ShapedTextFlags) /builds/worker/checkouts/gecko/gfx/thebes/gfxFont.cpp:3068:28
[task 2021-01-26T02:19:55.656Z] 02:19:55 INFO - #4 0x7f41ac1562c1 in void gfxFontGroup::InitScriptRun<unsigned char>(mozilla::gfx::DrawTarget*, gfxTextRun*, unsigned char const*, unsigned int, unsigned int, mozilla::unicode::Script, gfxMissingFontRecorder*) /builds/worker/checkouts/gecko/gfx/thebes/gfxTextRun.cpp:2711:25
[task 2021-01-26T02:19:55.656Z] 02:19:55 INFO - #5 0x7f41ac128e2a in void gfxFontGroup::InitTextRun<unsigned char>(mozilla::gfx::DrawTarget*, gfxTextRun*, unsigned char const*, unsigned int, gfxMissingFontRecorder*) /builds/worker/checkouts/gecko/gfx/thebes/gfxTextRun.cpp:2572:7
[task 2021-01-26T02:19:55.656Z] 02:19:55 INFO - #6 0x7f41ac127499 in gfxFontGroup::MakeTextRun(unsigned char const*, unsigned int, gfxTextRunFactory::Parameters const*, mozilla::gfx::ShapedTextFlags, nsTextFrameUtils::Flags, gfxMissingFontRecorder*) /builds/worker/checkouts/gecko/gfx/thebes/gfxTextRun.cpp:2462:3
[task 2021-01-26T02:19:55.684Z] 02:19:55 INFO - #7 0x7f41b199b68e in BuildTextRunsScanner::BuildTextRunForFrames(void*) /builds/worker/checkouts/gecko/layout/generic/nsTextFrame.cpp:2566:28
[task 2021-01-26T02:19:55.684Z] 02:19:55 INFO - #8 0x7f41b1997916 in BuildTextRunsScanner::FlushFrames(bool, bool) /builds/worker/checkouts/gecko/layout/generic/nsTextFrame.cpp:1662:17
[task 2021-01-26T02:19:55.684Z] 02:19:55 INFO - #9 0x7f41b19a3f49 in BuildTextRuns /builds/worker/checkouts/gecko/layout/generic/nsTextFrame.cpp:1586:11
[task 2021-01-26T02:19:55.685Z] 02:19:55 INFO - #10 0x7f41b19a3f49 in nsTextFrame::EnsureTextRun(nsTextFrame::TextRunType, mozilla::gfx::DrawTarget*, nsIFrame*, nsLineList_iterator const*, unsigned int*) /builds/worker/checkouts/gecko/layout/generic/nsTextFrame.cpp:3003:7
[task 2021-01-26T02:19:55.685Z] 02:19:55 INFO - #11 0x7f41b19de3d2 in nsTextFrame::ReflowText(nsLineLayout&, int, mozilla::gfx::DrawTarget*, mozilla::ReflowOutput&, nsReflowStatus&) /builds/worker/checkouts/gecko/layout/generic/nsTextFrame.cpp:9198:7
[task 2021-01-26T02:19:55.700Z] 02:19:55 INFO - #12 0x7f41b1952812 in nsLineLayout::ReflowFrame(nsIFrame*, nsReflowStatus&, mozilla::ReflowOutput*, bool&) /builds/worker/checkouts/gecko/layout/generic/nsLineLayout.cpp:878:40
[task 2021-01-26T02:19:55.721Z] 02:19:55 INFO - #13 0x7f41b171c74d in nsBlockFrame::ReflowInlineFrame(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) /builds/worker/checkouts/gecko/layout/generic/nsBlockFrame.cpp:4531:15
[task 2021-01-26T02:19:55.722Z] 02:19:55 INFO - #14 0x7f41b171b821 in nsBlockFrame::DoReflowInlineFrames(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) /builds/worker/checkouts/gecko/layout/generic/nsBlockFrame.cpp:4333:5
[task 2021-01-26T02:19:55.722Z] 02:19:55 INFO - #15 0x7f41b171367e in nsBlockFrame::ReflowInlineFrames(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /builds/worker/checkouts/gecko/layout/generic/nsBlockFrame.cpp:4218:9
[task 2021-01-26T02:19:55.722Z] 02:19:55 INFO - #16 0x7f41b170c103 in nsBlockFrame::ReflowLine(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /builds/worker/checkouts/gecko/layout/generic/nsBlockFrame.cpp:3190:5
[task 2021-01-26T02:19:55.722Z] 02:19:55 INFO - #17 0x7f41b1703741 in nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&) /builds/worker/checkouts/gecko/layout/generic/nsBlockFrame.cpp:2724:7
[task 2021-01-26T02:19:55.725Z] 02:19:55 INFO - #18 0x7f41b16fd291 in nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /builds/worker/checkouts/gecko/layout/generic/nsBlockFrame.cpp:1375:3
[task 2021-01-26T02:19:55.726Z] 02:19:55 INFO - #19 0x7f41b1718eda in nsBlockReflowContext::ReflowBlock(mozilla::LogicalRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, mozilla::ReflowInput&, nsReflowStatus&, mozilla::BlockReflowInput&) /builds/worker/checkouts/gecko/layout/generic/nsBlockReflowContext.cpp:288:11
[task 2021-01-26T02:19:55.727Z] 02:19:55 INFO - #20 0x7f41b170fc11 in nsBlockFrame::ReflowBlockFrame(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /builds/worker/checkouts/gecko/layout/generic/nsBlockFrame.cpp:3854:11

Also SUMMARY: AddressSanitizer: stack-overflow /builds/worker/checkouts/gecko/gfx/thebes/gfxTextRun.cpp:2237 in gfxFontGroup::GetFirstValidFont(unsigned int, mozilla::StyleGenericFontFamily*)

Flags: needinfo?(aethanyc)

In Part 8, I adjust the test expectations to skip bug 1383981's crashtests on AddressSanitizer. They were enabled in https://hg.mozilla.org/mozilla-central/rev/bf11f56d91cb7cf31564095d6e4467beeb8a4687, but they start to overflow the stack again because of this patch stack.

Flags: needinfo?(aethanyc)
Pushed by aethanyc@gmail.com: https://hg.mozilla.org/integration/autoland/rev/4568ea8e43b8 Part 1 - Remove unused ReflowInput::GetContainingBlockContentISize(). r=emilio https://hg.mozilla.org/integration/autoland/rev/5279b71d0068 Part 2 - Add StyleSizeOverrides parameter to ReflowInput's constructor & co. to override data from style system data. r=dholbert https://hg.mozilla.org/integration/autoland/rev/824d5be79299 Part 3 - Extract a helper method AxisTracker::IsInlineAxisMainAxis. r=dholbert https://hg.mozilla.org/integration/autoland/rev/4f25881a9ad0 Part 4 - Use StyleSizeOverrides to revise flex base size resolution. r=dholbert https://hg.mozilla.org/integration/autoland/rev/812bd46387a9 Part 5 - Use StyleSizeOverrides to override flex main size when main size can influence cross size. r=dholbert https://hg.mozilla.org/integration/autoland/rev/5c374273e8a6 Part 6 - Use StyleSizeOverrides to override flex item's sizes in its final reflow. r=dholbert https://hg.mozilla.org/integration/autoland/rev/f9d763420a03 Part 7 - Change two StyleSize variables from pointers to references, and rename them. r=emilio https://hg.mozilla.org/integration/autoland/rev/6316c4e1f46e Part 8 - Update test expectations. r=dholbert
Regressions: 1689045
See Also: → 1699468
Regressions: 1786610
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: