Open Bug 363391 Opened 14 years ago Updated 3 years ago

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

Categories

(Core :: Layout, defect, P4)

x86
All
defect

Tracking

()

REOPENED

People

(Reporter: martijn.martijn, Unassigned)

References

Details

(Keywords: hang, regression, testcase, Whiteboard: [dbaron-1.9:RwCr])

Attachments

(4 files, 1 obsolete file)

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.
Attached file testcase
OS: Windows XP → All
Attached file Reflow log
It's an infinite reflow cycle.
This will eventually crash on OOM because mDirtyRoots grows for each assert.
Keywords: crash
Depends on: 363253
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Ria, that's very likely a different bug, could you file a new bug on that (with a regression range, please?)?
Looks nasty, but I'm taking this bug to work on unless someone has objections.
Assignee: nobody → asmith15
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.
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.
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.
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.
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?
Attached patch probably bad fix (obsolete) — Splinter Review
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?
Attachment #282689 - Flags: review?(dbaron)
(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.
Maybe this is the right fix (the if not grow part).
Attachment #282689 - Attachment is obsolete: true
Attachment #283337 - Flags: review?(dbaron)
Attachment #282689 - Flags: review?(dbaron)
Whiteboard: [dbaron-1.9:RuCr]
Whiteboard: [dbaron-1.9:RuCr] → [dbaron-1.9:RwCr]
Comment on attachment 283337 [details] [diff] [review]
possibly good fix

stealing review
Attachment #283337 - Flags: superreview?(roc)
Attachment #283337 - Flags: review?(roc)
Attachment #283337 - Flags: review?(dbaron)
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.
I think it's best if someone else takes the bug. Reasigning to nobody.
Assignee: asmith15 → nobody
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.
Attachment #283337 - Flags: superreview?(roc)
Attachment #283337 - Flags: superreview+
Attachment #283337 - Flags: review?(roc)
Attachment #283337 - Flags: review+
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.
Attachment #297484 - Flags: superreview?(dbaron)
Attachment #297484 - Flags: review?(dbaron)
Whiteboard: [dbaron-1.9:RwCr] → [dbaron-1.9:RwCr][needs review]
Flags: wanted-next+
Flags: blocking1.9-
Flags: tracking1.9+
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.
Attachment #297484 - Flags: superreview?(dbaron)
Attachment #297484 - Flags: superreview-
Attachment #297484 - Flags: review?(dbaron)
Attachment #297484 - Flags: review-
Whiteboard: [dbaron-1.9:RwCr][needs review] → [dbaron-1.9:RwCr]
Kairo tried this in the current nightly and the testcase doesn't crash anymore. Resolving as works for me.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
The testcase never crashed for me, it did hang for me. Is the hang also gone?
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
I think the underlying bug is still here even if a particular testcase doesn't trigger it.
(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.
Status: REOPENED → RESOLVED
Closed: 9 years ago3 years ago
Resolution: --- → WORKSFORME
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
You need to log in before you can comment on or make changes to this bug.