Hang with CSS filter property (infinite loop of restyling)

RESOLVED FIXED in mozilla2.0b8

Status

()

Core
CSS Parsing and Computation
--
critical
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: Jesse Ruderman, Assigned: dholbert)

Tracking

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

Trunk
mozilla2.0b8
hang, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
Created attachment 466003 [details]
testcase (hangs Firefox when loaded)
(Reporter)

Comment 1

8 years ago
Created attachment 466004 [details]
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
blocking2.0: --- → ?

Updated

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

Comment 3

8 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.
Status: NEW → ASSIGNED
Depends on: 601999
(Assignee)

Comment 4

8 years ago
Created attachment 486783 [details]
stack demonstrating failure

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.
(Assignee)

Comment 5

8 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
(Assignee)

Updated

8 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Updated

8 years ago
Keywords: regression
(Assignee)

Comment 6

8 years ago
Created attachment 487939 [details]
testcase 2 (hangs Firefox when loaded) (simplified)

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.
(Assignee)

Comment 9

8 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
(Assignee)

Comment 10

8 years ago
Created attachment 488392 [details] [diff] [review]
fix?

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
(Assignee)

Comment 11

8 years ago
Created attachment 489366 [details] [diff] [review]
fix v2

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)
(Assignee)

Comment 12

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

Comment 13

8 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/c7726ff56cbf
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
(Assignee)

Updated

8 years ago
Depends on: 610954
Depends on: 613819
You need to log in before you can comment on or make changes to this bug.