Closed Bug 587336 Opened 14 years ago Closed 14 years ago

Hang with CSS filter property (infinite loop of restyling)

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla2.0b8
Tracking Flags:
Tracking Status
blocking2.0 --- final+

People

(Reporter: jruderman, Assigned: dholbert)

References

Details

(Keywords: hang, regression, testcase)

Attachments

(5 files, 1 obsolete file)

testcase (hangs Firefox when loaded)
(deleted), text/html
Details
hang sample
(deleted), text/plain
Details
stack demonstrating failure
(deleted), text/plain
Details
testcase 2 (hangs Firefox when loaded) (simplified)
(deleted), text/html
Details
fix v2
(deleted), patch
roc
: review+
Details | Diff | Splinter Review
Attached file testcase (hangs Firefox when loaded) (deleted) — 
      No description provided.
Attached file hang sample (deleted) — 
This sample suggests that the hang consists of an infinite loop of restyling:

1557 mozilla::css::RestyleTracker::ProcessRestyles
  1483 mozilla::css::RestyleTracker::ProcessOneRestyle
blocking2.0: --- → ?
blocking2.0: ? → final+
Sounds like the other one
Assignee: nobody → dholbert
Yup, probably the same thing as bug 601999 -- has the same situation of an element being used as [part of] its own filter.
Status: NEW → ASSIGNED
Depends on: 601999
Attached file stack demonstrating failure (deleted) — 
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.
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
OS: Mac OS X → All
Hardware: x86 → All
Keywords: regression
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.
(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
Attached patch fix? (obsolete) (deleted) — — 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
Attached patch fix v2 (deleted) — — Splinter 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)
(I've confirmed that fix v2 passes TryServer, btw)
Landed: http://hg.mozilla.org/mozilla-central/rev/c7726ff56cbf
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
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.

Attachment

General

Creator:
Created:
Updated:
Size: