crash near null in [@ nsContainerFrame::ReflowOverflowContainerChildren]
Categories
(Core :: Layout: Columns, defect, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox-esr60 | --- | unaffected |
| firefox-esr68 | --- | disabled |
| firefox63 | --- | unaffected |
| firefox64 | --- | unaffected |
| firefox65 | --- | disabled |
| firefox67 | --- | disabled |
| firefox68 | --- | disabled |
| firefox69 | --- | disabled |
| firefox70 | --- | fixed |
People
(Reporter: tsmith, Assigned: TYLin)
References
(Blocks 1 open bug, Regression)
Details
(4 keywords)
Crash Data
Attachments
(7 files)
|
Reporter | |
Comment 1•7 years ago
|
||
|
|
Reporter | |
Updated•7 years ago
|
|
||
Updated•7 years ago
|
|
Assignee | |
Updated•7 years ago
|
|
|
Assignee | |
Updated•7 years ago
|
|
|
Assignee | |
Comment 2•7 years ago
|
||
|
|
Assignee | |
Comment 3•7 years ago
|
||
|
|
Assignee | |
Comment 4•6 years ago
|
||
Mats, the review request sits for a while. Needinfo you to put the review onto your radar.
|
||
Comment 5•6 years ago
|
||
Hmm, it seems Phabricator review requests no longer shows up in the Bugzilla dashboard. Sigh. :(
I'll try to get to it soon, sorry for the delay.
|
|
||
Comment 6•6 years ago
|
||
Is there any background discussion documented anywhere regarding
the choice to use non-fluid continuations over a column-span split?
|
|
Assignee | |
Comment 7•6 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #6)
Is there any background discussion documented anywhere regarding
the choice to use non-fluid continuations over a column-span split?
I don't think there's a design documented exists.
When I picked up the WIP patches from Neerja, she was using IB split linkage over a column-span split in [1]. That doesn't feel correct to me because we have many special logic dealing with IB split in frame construction, and column-span split doesn't need that, needless to say a column-span split happens on the block inside of an inline. dbaron may know more about the early decisions.
We also don't want the column-span split being altered in reflow because they're fluid continuations, so using non-fluid continuations is the solution at the time I wrote the patch. (That, of course, leads to something like bug 1520722 that I discovered recently.)
Mats, do you foresee any major flaws in using non-fluid continuations over a column-span split?
[1] https://hg.mozilla.org/mozreview/gecko/rev/e385cc16ee356805da73678daed0615e8c32c344
|
|
||
Comment 8•6 years ago
|
||
(In reply to Ting-Yu Lin [:TYLin] (UTC-8) from comment #7)
(In reply to Mats Palmgren (:mats) from comment #6)
Is there any background discussion documented anywhere regarding
the choice to use non-fluid continuations over a column-span split?I don't think there's a design documented exists.
I'll repeat the same question for dbaron / bz then, hoping they
know more since they reviewed the original landing in bug 1421105.
Mats, do you foresee any major flaws in using non-fluid continuations over a column-span split?
Well, it took me just a few minutes of testing to find
bug 1523582 and bug 1523595, which made me worried...
I suspect we'll find a lot more if we really start digging.
So it's not a question of foreseeing major flaws - I think
it's evident that we already have some major problems to
deal with. I think this (and other) crash/assertion bugs
confirms that too.
If we want to pursue this design we should at least audit
all existing calls to FirstInFlow/Get[Prev|Next][InFlow|Continuation]
and make sure they are doing the right thing.
Clearly, FirstInFlow() itself is broken now, if invoked on
a continuation that is after a column-span:
https://searchfox.org/mozilla-central/rev/c07aaf12f13037c1f5a343d31f8291549e57373f/layout/generic/nsSplittableFrame.cpp#144
Similarly, we also have a lot of code that makes the assumption
that !f->GetPrevInFlow() implies that f is the first-in-flow
(which is now a bogus assumption):
https://searchfox.org/mozilla-central/rev/c07aaf12f13037c1f5a343d31f8291549e57373f/layout/base/nsLayoutUtils.cpp#4518
(just to pick a random example)
So there's likely a lot of hard work to make this work if we
want to pursue using non-fluid continuations for this.
Using fluid continuations does have the problem you point out
though - that a container generally pulls up child boxes from
its next-in-flow until it overflows. This seems like an easier
problem to solve though, since it's similar to a page-break.
I haven't really investigated it deeply though, which is why
I was wondering about how the design discussions went that
came to the current design.
|
|
||
Comment 9•6 years ago
|
||
Also, I don't see anything in the css-multicol spec that precludes
splitting Grid/Flexbox/Table containers. E.g. something like this:
<columns>
<grid>
<item>
<div style="column-span:all">
should split the grid container, IIUC.
https://drafts.csswg.org/css-multicol-1/#column-span
|
||
Comment 10•6 years ago
|
||
I haven't kept track of our continuation handling well enough to answer questions about it, sorry. I mostly reviewed the frame constructor changes for basic sanity. I am hoping David knows more...
|
|
Assignee | |
Comment 11•6 years ago
•
|
||
(In reply to Mats Palmgren (:mats) from comment #9)
Also, I don't see anything in the css-multicol spec that precludes
splitting Grid/Flexbox/Table containers. E.g. something like this:
<columns>
<grid>
<item>
<div style="column-span:all">should split the grid container, IIUC.
https://drafts.csswg.org/css-multicol-1/#column-span
The spec says "The element spans across all columns of the nearest multicol ancestor in the same block formatting context.",
https://drafts.csswg.org/css-multicol-1/#valdef-column-span-all
Grid items establish a new formatting context, which makes "column-span:all" in a different formatting context other than in the multicol container, so the column-span should behave like "none".
ShouldSuppressColumnSpanDescendants is the function to detect that.
https://searchfox.org/mozilla-central/rev/78cd247b5d7a08832f87d786541d3e2204842e8e/layout/base/nsCSSFrameConstructor.cpp#401
|
|
||
Comment 12•6 years ago
|
||
OK, thanks for clarifying.
(Seems like an unnecessary limitation to me though, fwiw.
But let's not digress on that here.)
(In reply to Mats Palmgren (:mats) from comment #6)
Is there any background discussion documented anywhere regarding
the choice to use non-fluid continuations over a column-span split?
So I think the choice is between non-fluid continuations and something more like ib-splits. I don't think fluid continuations make any sense since we don't rewrap where the column-span is located as part of reflow (i.e., push/pull frames across the boundary during reflow); that's the fundamental difference between fluid and non-fluid continuations.
Given that (a) all the frames are the same type and (b) there didn't seem to be any other obvious problems, it seems like using continuations is better since it should require adjustment to less other code.
(In reply to Mats Palmgren (:mats) from comment #8)
If we want to pursue this design we should at least audit
all existing calls to FirstInFlow/Get[Prev|Next][InFlow|Continuation]
and make sure they are doing the right thing.Clearly, FirstInFlow() itself is broken now, if invoked on
a continuation that is after a column-span:
https://searchfox.org/mozilla-central/rev/c07aaf12f13037c1f5a343d31f8291549e57373f/layout/generic/nsSplittableFrame.cpp#144
Similarly, we also have a lot of code that makes the assumption
that !f->GetPrevInFlow() implies that f is the first-in-flow
(which is now a bogus assumption):
https://searchfox.org/mozilla-central/rev/c07aaf12f13037c1f5a343d31f8291549e57373f/layout/base/nsLayoutUtils.cpp#4518
(just to pick a random example)
I don't understand why any of this is the case. Is something maintaining the continuation chain incorrectly? I don't see why any of these things should follow from using non-fluid continuations here. That said, FirstInFlow() and FirstContinuation() are different, just like GetPrevInFlow() and GetPrevContinuation() are different. But all the *InFlow methods should be internally consistent, and all the *Continuation methods should be internally consistent.
It's possible we need some auditing about which code is using *InFlow vs. *Continuation methods since that auditing may not have been done in the past for blocks. (That code should be reasonably solid for inlines since we have had the distinction there for a long time due to bidi continuations. But I can understand that we might still have bugs in code that never applies to inlines and auditing might be needed. But I think that auditing ought to be less than the auditing we'd need if we took an ib-split-like approach here.)
|
|
||
Comment 14•6 years ago
|
||
(In reply to David Baron :dbaron: 🇯🇵 ⌚UTC+9 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it) from comment #13)
So I think the choice is between non-fluid continuations and something more like ib-splits.
I believe we are free to choose from all technically viable solutions, including fluid continuations. They should be evaluated on their pros and cons, not by proclaiming that a specific solution is superior to all other by fiat.
IIUC, ib-split has already been tried (by neerja) and didn't work out well (I assume), so that leaves the choice between non-fluid/fluid continuations, or some new type of frame tree linkage. It's quite clear that using non-fluid continuations causes a lot of regressions to existing [decades-old, complex] code. Using fluid continuations would not do that. It does have the push/pull problem, but this has been solved by page breaks already and it seems like an easier problem to solve.
It's possible we need some auditing about which code is using InFlow vs. Continuation methods
I think we need to audit all such code. Most fixes are probably easy (replace InFlow w. Continuation or vice versa), but I suspect some will need more elaborate changes.
That code should be reasonably solid for inlines since we have had the distinction there for a long time due to bidi continuations.
Apparently our bidi code is confused about this new type of non-fluid continuations (bug 1524431).
====
Please note that I'm not really advocating for any particular solution. Using non-fluid continuations might very well be the best long-term solution even if it's more costly right now. It seems obvious to me though that it requires non-trivial work (both in time and complexity) before it can ship. I'm just trying to raise a warning flag about that.
I also have to say that I'm dismayed at how breaking changes to existing code are introduced without telling anyone. People could be adding code as we speak that will break once column-span is enabled. In the future, it would be nice to include people with long experience and relevant domain knowledge of this code to the design discussion before a decision is made.
If we still want to go ahead with using non-fluid continuations for this, I would suggest that someone:
- send a PSA to dev.platform (or dev.layout? is everyone that could potentially use the continuation methods reading that?) that this change is coming so that they are not unknowingly introducing new bugs
- update https://wiki.mozilla.org/Gecko:Continuation_Model and elaborate on how this change melds with the existing model; Overflow Container Continuations in particular
- update (or replace with the wiki link) in-tree documentation, like:
https://searchfox.org/mozilla-central/rev/152993fa346c8fd9296e4cd6622234a664f53341/layout/generic/nsSplittableFrame.h#29-35
https://searchfox.org/mozilla-central/rev/152993fa346c8fd9296e4cd6622234a664f53341/layout/generic/nsContainerFrame.h#280-315
(We should probably wait with that until we have sorted out how overflow containers should work in this new world... The suggested patch doesn't seem correct to me at first glance.)
|
|
||
Comment 15•6 years ago
|
||
Here's a simpler testcase that triggers this crash...
|
|
||
Comment 16•6 years ago
|
||
... and here's what the frame tree looks like with the suggested patch.
I've highlighted the frames for "Block(x)" in blue.
This block has a constrained height so it's COMPLETE already
in the first column. Hence we create an overflow container
(7fd5a2e1eb50, green) to hold the overflowing child frames.
Then, below the column-span block, the continuations are
on the principal child list. This just doesn't feel
right to me. The continuations for "Block(x)" after
the column-span should be on an OverflowContainersList(s)
of the <div> and be overflow containers.
Also, it seems a bit odd to me that the "Block(x)" child
frame inside the column-span (7fd5a2e1e480) isn't included
at all in the continuation chain. Is that intentional?
|
|
Assignee | |
Comment 17•6 years ago
•
|
||
(In reply to Mats Palmgren (:mats) from comment #16)
Also, it seems a bit odd to me that the "Block(x)" child
frame inside the column-span (7fd5a2e1e480) isn't included
at all in the continuation chain. Is that intentional?
Yes, this is changed by Bug 1421105 Part 5 https://phabricator.services.mozilla.com/D9988
|
|
||
Comment 18•6 years ago
•
|
||
OK, but I still don't follow why we're doing that. It seems rather
unusual that we now can't find all the fragments for an element by
following the continuation chain (GetNextContinuationOrIBSplitSibling)
from the primary frame. Are some of the consumers of those methods now
broken I wonder...
|
|
||
Comment 19•6 years ago
|
||
emilio pointed out to me that excluding frames from the continuation
chain is a problem, and he pointed to this as an example:
https://searchfox.org/mozilla-central/rev/152993fa346c8fd9296e4cd6622234a664f53341/layout/base/RestyleManager.cpp#2812
Re comment 14: OK, I hadn't considered the option of working like page break frames was a possibility, so I didn't see how using fluid continuations was even a possibility, but now that you explain it in slightly more detail it is an option. However, it has some pretty significant disadvantages:
- you'd need to teach block frames about when they're a column-span (and likely also column-span wrapper) so that they emit the correct break statuses, adding a decent amount of additional code to a relatively hot codepath
- you'd end up destroying and recreating a lot of continuations every reflow, or at least dirtying them, in the process of pushing/pulling and reflowing in order to set those break statuses.
Based on what I've seen so far about the issues we've run into with using non-fluid continuations, it still seems to me like it's worth continuing to try to get the non-fluid continuations approach to work. But I'm open to changing that opinion based on hearing about other problems...
That said, I think the thing that the bidi code is doing by assuming that it's the only code that ever creates non-fluid continuations is one of the basic problems that needs to be fixed here, as discussed in the other bug. I'd hope there aren't too many more problems, but I'd be interested to understand what other problems there are..
Re comment 16: I agree that it's bad if you can't get to all the frames by following continuations and ib-splits.
|
|
Assignee | |
Updated•6 years ago
|
|
|
Reporter | |
Updated•6 years ago
|
|
||
Updated•6 years ago
|
|
|
Assignee | |
Comment 21•6 years ago
|
||
In frame construction, a block can be split by column-spanners into
several fragments and each of the fragments are chained together by
non-fluid continuations. Later in reflow, each fragment can create its
own fluid continuations due to reasons such as the constraint of its
available block-size.
The main idea of this patch is that we calculate the block's final
block-size by shrinkwrapping the children for every fragments except for
those fluid-continuations after the final column-spanner. To do that, we
need to correctly tag each non-column-span-wrappers
nsIFrame::HasColumnSpanSiblings() except the last one.
We also need to modify nsSplittableFrame::ConsumedBSize() so that it
includes the block-size for both fluid and non-fluid continuations.
Depends on D15392
|
|
Assignee | |
Comment 22•6 years ago
|
||
-
004a.html and 004b.html test the block-size distribution in balancing mode.
-
005.html tests the block-size distribution in fill mode.
-
006.html is like 004a.html, but it adds a border to the block split by
column-span. It currently fails due to bug 1523582.
Depends on D38124
|
|
Assignee | |
Comment 23•6 years ago
|
||
Re comment 16:
I attached a sample frame tree with my latest patches. With many bugs fixed since I posted my initial patch in this bug, the ColumnSet subtree before column-span should now make more sense.
Then, below the column-span block, the continuations are on the principal child list. This just doesn't feel right to me. The continuations for "Block(x)" after the column-span should be on an OverflowContainersList(s) of the <div> and be overflow containers.
However, the ColumnSet subtree after column-span still have the issue mention above, i.e. the continuations for Block(x)(1)@7f3ece696430 is still on the principal child list, not on OverflowContainersList. This is because Block(x)(1)@7f3ece696430 is created during frame construction in nsCSSFrameConstructor::CreateColumnSpanSiblings, not during reflow when the block is running out of block-size.
I don't have a clear idea on whether we should move the block into its parent's OverflowContainersList when its previous fixed continuation are already in OverflowContainersList during reflow ...
|
||
Comment 24•6 years ago
|
||
Bugbug thinks this bug is a regression, but please revert this change in case of error.
|
|
Assignee | |
Updated•6 years ago
|
|
|
||
Updated•6 years ago
|
|
||
Comment 25•6 years ago
|
||
|
||
Comment 28•6 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/f2893e234358
https://hg.mozilla.org/mozilla-central/rev/e0746a6b821e
https://hg.mozilla.org/mozilla-central/rev/968808feda5a
|
|
||
Updated•3 years ago
|
Description
•