Bug 1579929 Comment 29 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

I'm starting to understand a thing that goes wrong with interruptible reflow + flexbox here (which may be what's at fault).

* Twitter has tons of flex containers with abspos content
* When we get a pending interrupt during reflow, our flex container doesn't (itself) call `CheckForInterrupt()`
* However, it does call FinishReflowWithAbsoluteFrames which (several layers down) does check for an interrupt in `nsAbsoluteContainingBlock::Reflow`. From this point on (until our next top-level reflow task starts) `aPresContext->HasPendingInterrupt()` will return true.
* When we finish reflowing a given nsFlexContainerFrame, it tests whether we've seen an interrupt and (if so) throws away all of its cached measurements:
```c++
void nsFlexContainerFrame::DidReflow(nsPresContext* aPresContext,
                                     const ReflowInput* aReflowInput) {
  // Remove the cached values if we got an interrupt because the values will be
  // the wrong ones for following reflows.
[...]
  if (aPresContext->HasPendingInterrupt()) {
    for (nsIFrame* frame : mFrames) {
      frame->DeleteProperty(CachedFlexMeasuringReflow());
    }
  }
```
* This would be an appropriate thing to do if we were bailing out all the way. However, we are not necessarily bailing out all the way -- we may get reflowed multiple times due to being e.g. a flex item that needs a measuring reflow + a final reflow, or e.g. due to being in a scrollframe (which reflows its content with + without scrollbar spacing subtracted out).  If we had a cached measurement for our child content measurements, then the subsequent reflows would be cheaper; however, we've just thrown all of those cached measurements away, which ensures that we'll do a more-expensive "measuring" reflow of all our children who need it. And if those children are flex containers, then this recursively applies to them, too (they'll re-measure their own children on each repeated reflow as well, because they won't have any cached measurements either).

So this all blows up quite painfully, because we're throwing away potentially-invalid cached measurements which makes the rest of our reflow much more expensive.

Perhaps we should use a reflow callback to do the throwing-away....
I'm starting to understand a thing that goes wrong with interruptible reflow + flexbox here (which may be what's at fault).

* Twitter has tons of flex containers with abspos content
* When we get a pending interrupt during reflow, our flex container doesn't (itself) call `CheckForInterrupt()`
* However, it does call FinishReflowWithAbsoluteFrames which (several layers down) does check for an interrupt in `nsAbsoluteContainingBlock::Reflow`. From this point on (until our next top-level reflow task starts) `aPresContext->HasPendingInterrupt()` will return true.
* When we finish reflowing a given nsFlexContainerFrame, it tests whether we've seen an interrupt and (if so) throws away all of its cached measurements:
```c++
void nsFlexContainerFrame::DidReflow(nsPresContext* aPresContext,
                                     const ReflowInput* aReflowInput) {
  // Remove the cached values if we got an interrupt because the values will be
  // the wrong ones for following reflows.
[...]
  if (aPresContext->HasPendingInterrupt()) {
    for (nsIFrame* frame : mFrames) {
      frame->DeleteProperty(CachedFlexMeasuringReflow());
    }
  }
```
* This would be an appropriate thing to do if we were bailing out all the way. However, we are not necessarily bailing out all the way -- we may get reflowed multiple times due to being e.g. a flex item that needs a measuring reflow + a final reflow, or e.g. due to being in a scrollframe (which reflows its content with + without scrollbar spacing subtracted out).  If we had a cached measurement for our child content measurements, then the subsequent reflows would be cheaper; however, we've just thrown all of those cached measurements away, which ensures that we'll do a more-expensive "measuring" reflow of all our children who need it. And if those children are flex containers, then this recursively applies to them, too (they'll re-measure their own children on each repeated reflow as well, because they won't have any cached measurements either).

So this all blows up quite painfully, because we're throwing away (potentially-invalid) cached measurements which makes the rest of our interrupted reflow much more expensive.

Perhaps we should use a reflow callback to do the throwing-away....

Back to Bug 1579929 Comment 29