866767 - Too-much-recursion crash with filter, abs pos

Status: RESOLVED DUPLICATE of bug 1125750

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

The repeating part of the stack:

PresShell::DidDoReflow
PresShell::ProcessReflowCommands
PresShell::FlushPendingNotifications
PresShell::FlushPendingNotifications
PresShell::HandlePostedReflowCallbacks

Comment 1

[Jesse Ruderman]

Attachment: stack (gdb)

Comment 2

[Scoobidiver (away)]

On Windows: bp-8611c9a4-a5d4-43a4-b4a5-0ac9c2130429, bp-76460018-c121-41c0-be13-a29642130429, bp-d0003569-fa62-484e-b85b-02d6e2130429.

Comment 3

[Mats Palmgren (inactive)]

Attachment: stacks + frame tree

So what happens here is:
1. we reflow all normal flow frames, including the scroll frame for
   <div style="overflow-y: scroll; filter: url(#b);">
2. nsViewportFrame reflows its fixed pos frames, in this case the
   frame for #b.  Since the element in 1 in observing it it gets
   a nsChangeHint_UpdateOverflow request (see Stack A).
3. UpdateOverflow is called on the scroll frame which turns that into
   a reflow request in this case (Stack B).  Since the closest reflow
   root is the viewport frame we start the new reflow from there.
4. we reflow the scroll frame again since it's dirty per the
   FrameNeedsReflow request
5. the viewport frame calls GetAbsoluteContainingBlock()->Reflow again:
   http://hg.mozilla.org/mozilla-central/annotate/f2d7d694aae5/layout/generic/nsViewportFrame.cpp#l256
   this time though, the fixed frame isn't dirty, but note the
   "true true" on line 259 above, and:
   http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsAbsoluteContainingBlock.cpp#125
   and FrameDependsOnContainer:
   http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsAbsoluteContainingBlock.cpp#201
   In this case we return true in the very first statement that checks
   top/bottom == auto.

Note also that in 4, we return true from the reflow callback function
ScrollFrameHelper::ReflowFinished:
http://hg.mozilla.org/mozilla-central/annotate/f2d7d694aae5/layout/generic/nsGfxScrollFrame.cpp#l4336
(meaning that the pres shell will do a flush here)
This is what drives the buildup of the stack.
Changing it to return false avoids that, making the browser responsive,
but we're still reflowing the frames per above in eternity.

To fix this we need to:
1. track to placeholder position and only do the first statement in
   FrameDependsOnContainer if it has changed since the last reflow
2. pass real values to aCBWidth/HeightChanged instead of "true".
   Otherwise, it will cause us to return true in the remainder of
   FrameDependsOnContainer when it's not necessary.

I'm hoping that the things that can cause 
nsSVGRenderingObserver::InvalidateViaReferencedElement
doesn't ever affect the layout of the observer in any way, because
then it could still drive reflow in eternity.

Comment 4

[Mats Palmgren (inactive)]

Just as an experiment, I pushed a change to return 'false' in
ScrollFrameHelper::ReflowFinished to see if that's really needed:
https://tbpl.mozilla.org/?tree=Try&rev=9f7e78653721
It caused one test to fail:
http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/bugs/test_sizetocontent_clamp.html?force=1&mark=46-48,55-56

Now we just get one resize event there, which seems like an improvement
to me.  If I remove the early return there, and change is "2" to "1" in
the is(), then it passes locally.

roc, do you know why we explicitly request a flush in
ScrollFrameHelper::ReflowFinished ?

Comment 5

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

(In reply to Mats Palmgren (:mats) from comment #4)
> roc, do you know why we explicitly request a flush in
> ScrollFrameHelper::ReflowFinished ?

We need to flush reflows of scrollbars and any fixed frame we did FrameNeedsReflow for, right?

Comment 6

[Daniel Holbert [:dholbert]]

The attached testcase doesn't crash at this point, FWIW. Tracking down a fix range...

Comment 7

[Daniel Holbert [:dholbert]]

Fix range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c5b90c003be8&tochange=993eb76a8bd6

Likely fixed by bug 1125750 (whose commit messages mention avoiding unnecessary reflow w/ overflow styles), given that the testcase had overflow-y:scroll and given that the bad-result was a recursive reflow spiral.

I'll file a new bug for the crash signature that was associated with this bug (which is still getting a low level of crashes), and I think we can call this one "fixed" by bug 1125750, as-filed. (And I'll land a crashtest.)

Comment 8

[Daniel Holbert [:dholbert]]

(RE the crash signatures: nsFont::nsFont and nsFontCache::GetMetricsFor are the two associated signatures that have a nonzero number of crash reports from the past 6 months, where we have unexpired crash data. It's not obvious to me that those signatures are actually connected to each other, so I'll file two different bugs for them.)

Comment 9

[Daniel Holbert [:dholbert]]

Spun off bug 1794695 on nsFont::nsFont and bug 1794699 on nsFontCache::GetMetricsFor.

Closing this bug as dupe of bug 1125750 per comment 7.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: Bug 866767: Add crashtest for this no-longer-reproducible bug. (no review, crashtest-only)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46b6b6bcf392
Add crashtest for this no-longer-reproducible bug. (no review, crashtest-only)

Comment 12

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/46b6b6bcf392

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)