Hang with CSS filter property (infinite loop of restyling)

RESOLVED FIXED in mozilla2.0b8



9 years ago
9 months ago


(Reporter: jruderman, Assigned: dholbert)


(Depends on 1 bug, Blocks 1 bug, {hang, regression, testcase})

Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)



(5 attachments, 1 obsolete attachment)



9 years ago
No description provided.

Comment 1

9 years ago
Posted file hang sample
This sample suggests that the hang consists of an infinite loop of restyling:

1557 mozilla::css::RestyleTracker::ProcessRestyles
  1483 mozilla::css::RestyleTracker::ProcessOneRestyle


9 years ago
blocking2.0: ? → final+
Sounds like the other one
Assignee: nobody → dholbert

Comment 3

9 years ago
Yup, probably the same thing as bug 601999 -- has the same situation of an element being used as [part of] its own filter.
Depends on: 601999

Comment 4

9 years ago
I guess this is slightly different from bug 601999 in that it's a hang instead of a stack-exhaustion crash.  But it's got the same kind of ping-pong effect going on.

Here's a stack to demonstrate what happens here.

At the top of the stack (level 17), we're inside of RestyleTracker::ProcessRestyles, which basically says:  "while (have pending restyles) { shift pending restyles to a temporary array, and service them }"

As it turns out, we have 1 restyle queued up.  In the process of handling it, we invalidate our <span> frames, and nsIFrame::InvalidateInternal causes us to invalidate SVG rendering observers of those frames, which ultimately makes us post a restyle event (stack level 0).

So, when this stack all finishes and we get back up to stack level 17, we restart the "while" loop with a new entry in our restyle queue.  So we continue looping so we can service that entry -- but that makes us add another new entry, etc -- and we loop forever.

Comment 5

9 years ago
Note that stack level 7 --> 6 is the call to "nsSVGEffects::InvalidateDirectRenderingObservers" that I highlighted in bug 601999 comment 6.  Without that line, we don't post a new restyle event, and we don't infinite-loop.  That line was added in bug 506826 --> marking as regression from that bug.
Blocks: 506826


9 years ago
OS: Mac OS X → All
Hardware: x86 → All


9 years ago
Keywords: regression

Comment 6

9 years ago
Here's a slightly-simplified testcase, with just 1 span element, which is dynamically restyled to use its parent (the <body>) as a filter.
What's supposed to prevent recursion here is that nsSVGRenderingObserverList::InvalidateAll clears out the observer list before firing any observers. So something must be re-adding to the observer list to get infinite recursion here.
I'm guessing it's the call to nsSVGEffects::GetEffectProperties from nsSVGIntegrationUtils::GetInvalidAreaForChangedSource.

How about, if there is no observer relationship established, don't let GetEffectProperties add one, just bail out of here without changing aInvalidRect. That should be fine, since the invalidation that killed the observer relationship should have invalidated everything covered by the filter already.

Comment 9

9 years ago
(In reply to comment #7)
> So something must be re-adding to the observer list to
> get infinite recursion here.

(Just to be clear for posterity -- there's actually no infinite recursion in this bug's testcase, though there is in bug 601999's testcase. See comment 4)

RE comment 8: Aha, I think you're on to something! I don't think it's GetEffectProperties, though -- it appears to be GetFilterFrame that's at fault.

nsSVGIntegrationUtils::GetInvalidAreaForChangedSource has a call to 
nsSVGEffects::GetFilterFrame, which ends up adding a rendering observer via this stack:
> #0  nsSVGEffects::AddRenderingObserver (aElement=0x7f7e89a40040, aObserver=0x7f7e89a12300) at layout/svg/base/src/nsSVGEffects.cpp:553
> #1  0x00007f7eb2c4945d in nsSVGRenderingObserver::GetReferencedElement (this=0x7f7e89a12300) at layout/svg/base/src/nsSVGEffects.cpp:144
> #2  0x00007f7eb2c49481 in nsSVGRenderingObserver::GetReferencedFrame (this=0x7f7e89a12300) at layout/svg/base/src/nsSVGEffects.cpp:153
> #3  0x00007f7eb2c494c7 in nsSVGRenderingObserver::GetReferencedFrame (this=0x7f7e89a12300, aFrameType=0x7f7ea1f84370, aOK=0x0) at layout/svg/base/src/nsSVGEffects.cpp:160
> #4  0x00007f7eb2c49597 in nsSVGFilterProperty::GetFilterFrame (this=0x7f7e89a12300) at layout/svg/base/src/nsSVGEffects.cpp:254
> #5  0x00007f7eb2c60daa in nsSVGEffects::GetFilterFrame (aFrame=0x7f7e89a54020) at layout/svg/base/src/nsSVGEffects.h:341
> #6  0x00007f7eb2c5f60b in nsSVGIntegrationUtils::GetInvalidAreaForChangedSource (aFrame=0x7f7e89a54020, aInvalidRect=...) at layout/svg/base/src/nsSVGIntegrationUtils.cpp:150

Comment 10

9 years ago
Posted patch fix? (obsolete) — Splinter Review
This mostly does what roc suggested in comment 8, except it modifies GetFilterFrame instead of GetEffectProperties (since GetFilterFrame is where the observer gets added to the observer-list, as noted in previous comment).

This fixes both testcases here.  Running it through TryServer to sanity-check.
Attachment #486783 - Attachment is patch: false

Comment 11

9 years ago
Posted patch fix v2Splinter Review
The previous fix was a little bit too big of a stick & broke some reftests, since other callers of nsSVGFilterProperty::GetFilterFrame depend on it setting up the observer relationship.

So, this version is more targeted -- it makes nsSVGIntegrationUtils::GetInvalidAreaForChangedSource() perform the check to see whether we've got an observer relationship yet.  This means we have to expose the "mInObserverList" flag via an accessor method, but that's not a big deal.
Attachment #488392 - Attachment is obsolete: true
Attachment #489366 - Flags: review?(roc)

Comment 12

9 years ago
(I've confirmed that fix v2 passes TryServer, btw)

Comment 13

9 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/c7726ff56cbf
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8


9 years ago
Depends on: 610954
Depends on: 613819
(Reverted in bug 1494092 as no longer necessary now that we use DLBI.)
You need to log in before you can comment on or make changes to this bug.