363391 - [reflow branch] Hang with setting outline on iframes in tabpanels

Status: RESOLVED WORKSFORME

(Core :: Layout, defect, P4)

Description

[Martijn Wargers (dead)]

See upcoming testcase, it hangs Mozilla with a 2006-12-10 trunk build, but not with a 2006-12-06 trunk build, so very likely a regression from the reflow branch landing.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Mats Palmgren (inactive)]

Attachment: Reflow log

It's an infinite reflow cycle.

Comment 3

[Mats Palmgren (inactive)]

This will eventually crash on OOM because mDirtyRoots grows for each assert.

Comment 4

[Ria Klaassen (not reading all bugmail)]

I see 100% CPU usage on this site http://216.239.59.104/search?q=cache:Y0QmQHul7joJ:www.veilingstart.nl/greepjes.htm+greepjes+laden&hl=nl&ct=clnk&cd=11&gl=nl is this the same bug? 

Comment 5

[Martijn Wargers (dead)]

Ria, that's very likely a different bug, could you file a new bug on that (with a regression range, please?)?

Comment 6

[Ria Klaassen (not reading all bugmail)]

Thanks, filed as bug 383351.

Comment 7

[Andrew Smith]

Looks nasty, but I'm taking this bug to work on unless someone has objections.

Comment 8

[Andrew Smith]

I'll write down what I found so far, before my head bursts.

In nsStackLayout::Layout() child->GetRect() is increased in size twice: once in child->SetBounds(aState, childRect); and once again in child->Layout(aState); That's what causes the infinite loop.

I'm trying to figure out which of the two calls is at fault.

Comment 9

[Andrew Smith]

Here's a walkthrough of child->Layout():

-----------------------------------
mOverflowArea is set to aDesiredSize in nsLeafFrame::Reflow()
mOverflowArea is increased in nsIFrame::FinishAndStoreOverflow(), being set to outlineRect
and again

aDesiredSize is set in nsFrame::BoxReflow() to aDesiredSize.mOverflowArea.XMost(), which is one step less than aDesiredSize.mOverflowArea

mOverflowArea is set to aDesiredSize again back in nsLeafFrame::Reflow()
mOverflowArea is increased in nsIFrame::FinishAndStoreOverflow(), being set to outlineRect
and again

size is set to aDesiredSize in nsFrame::BoxReflow() via nsContainerFrame::FinishReflowChild()
-----------------------------------

Maybe someone reading this bug sees an obvious problem. I don't understand this stuff (reflow) having never worked on it.

For now I will probably try to figure out why child->GetRect() is not increased in size (seemingly) for other cases, and if that doesn't produce results I'll try a comparison between the testcase here and some... thing else.

Comment 10

[Andrew Smith]

Ok, it looks like the 'and again' above is the problem. FinishAndStoreOverflow() is called from nsSubDocumentFrame::Reflow() (nsFrameFrame.cpp) the first time via nsLeafFrame::Reflow() the second time directly, one of those should not be happening.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Good catch ... we probably just want to copy the relevant stuff from nsLeafFrame::Reflow, or better still, refactor that stuff into a method of nsLeafFrame which can be called by both Reflow methods.

Comment 12

[Andrew Smith]

Oh fiddly sticks.. looks like I missed something. Getting rid of one of the calls to FinishAndStoreOverflow() doesn't solve the problem. The size is still increased by 60 things (it used to be increased by 120).

Roc, is calling FinishAndStoreOverflow() twice a bug? Should I continue searching with it fixed or without?

Comment 13

[Andrew Smith]

Attachment: probably bad fix

Interesting - if I get rid of both calls to FinishAndStoreOverflow() that fixes the bug.

I suppose it's there for a reason though; and the fix is wrong; but dbaron - will you take a quick look and say if there's anything obviously wrong with the patch, and why?

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #12)
> Oh fiddly sticks.. looks like I missed something. Getting rid of one of the
> calls to FinishAndStoreOverflow() doesn't solve the problem. The size is still
> increased by 60 things (it used to be increased by 120).
> 
> Roc, is calling FinishAndStoreOverflow() twice a bug? Should I continue
> searching with it fixed or without?

It's certainly a bug.

We do want at least one call to it, though.

Comment 15

[Andrew Smith]

Attachment: possibly good fix

Maybe this is the right fix (the if not grow part).

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 283337 [details] [diff] [review]
possibly good fix

stealing review

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The nsSubdocumentFrame/nsLeafFrame parts are fine.

I think the nsStackLayout change is not right. As soon as one child grows we stop SetBounds on the rest. The real problem is here:
http://mxr.mozilla.org/seamonkey/source/layout/generic/nsFrame.cpp#6162
There's a lot wrong with extending the desired size to include overflow, and it can loop in various ways, but I don't think we can just stop including overflow here, it could break a lot of stuff.

So as a nasty hack, we could check in nsStackLayout if a child has outline style, and don't allow it to set the "grow" flag in that case. That won't solve all problems but it will solve this one. To solve all problems we really have to eliminate that code in in nsFrame.

Comment 18

[Andrew Smith]

I think it's best if someone else takes the bug. Reasigning to nobody.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 283337 [details] [diff] [review]
possibly good fix

Setting r+sr on Andrew's patch, but just the nsFrameFrame and nsLeafFrame parts. They can land on their own.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: exclude outline when updating size from overflow area during BoxReflow

This fixes the rest of this testcase. This reliably removes the effect of 'outline' on our own overflow area so that we don't size our frame to include that part of our overflow.

There's still a more complex form of this bug, though, where the outline is on a child which grows as this frame grows. Unfortunately fixing that is really hard, it really requires separating the concepts of overflow area as I described in my blog. I'm inclined to not try for 1.9.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in attachment 283337 [details] [diff] [review].

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think attachment 297484 [details] [diff] [review] is the wrong approach.  (I've been thinking that for a while; should have said something earlier.)  Instead, I think nsStackLayout::Layout should do something to prevent children from growing infinitely.  (For example, perhaps the child(ren) that cause growth shouldn't get laid out again the second time around?)

It might be tricky to get incremental layouts correct, though.

Comment 23

[Sheila Mooney]

Kairo tried this in the current nightly and the testcase doesn't crash anymore. Resolving as works for me.

Comment 24

[Martijn Wargers (dead)]

The testcase never crashed for me, it did hang for me. Is the hang also gone?

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think the underlying bug is still here even if a particular testcase doesn't trigger it.

Comment 26

[Robert Kaiser]

(In reply to David Baron [:dbaron] from comment #25)
> I think the underlying bug is still here even if a particular testcase
> doesn't trigger it.

Then either a new testcase should be found that triggers it and a new bug opened or this one should be stripped of all information not relating to the symptoms, including the keywords, IMHO. This is not an issue any more that we are actually seeing, so nothing we urge anyone to work on, but we will pressure people to fix bug with crash+regression keywords set.

Comment 27

[Andrei Purice]

Following the reporter's steps I am able to confirm that the issues doesn't happen anymore on Windows 10 on any of the current versions of Firefox Nightly 87.0a1 (2021-02-16), beta 86.0 and release 85.0.2. The example test case doesn't hang/crash Firefox anymore.

Closing this issue as Resolved > Worksforme.
Feel free to re-open or file a new bug if this issue reoccurs again.