Last Comment Bug 786894 - SVG in an active GFX layer is not invalidating properly
: SVG in an active GFX layer is not invalidating properly
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
:
Mentors:
Depends on: 802628
Blocks: 776054
  Show dependency treegraph
 
Reported: 2012-08-29 18:03 PDT by Daniel Holbert [:dholbert]
Modified: 2013-01-29 07:37 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed
+
fixed
+
verified
fixed


Attachments
testcase 1 (481 bytes, image/svg+xml)
2012-08-29 18:03 PDT, Daniel Holbert [:dholbert]
no flags Details
semi-reference case 1 (377 bytes, image/svg+xml)
2012-08-29 18:05 PDT, Daniel Holbert [:dholbert]
no flags Details
clearer testcase (718 bytes, image/svg+xml)
2012-09-05 03:22 PDT, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
no flags Details
patch for mozilla-beta (17) (2.88 KB, patch)
2012-10-15 03:31 PDT, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
matt.woodrow: review+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review

Description Daniel Holbert [:dholbert] 2012-08-29 18:03:59 PDT
Created attachment 656695 [details]
testcase 1

STR: Load attached testcase.

EXPECTED RESULTS: Animation rapidly switching the rect from blue to yellow, along with subtle opacity animation.

ACTUAL RESULTS: Just the subtle opacity animation is visible, and the color only changes when you force an invalidate by clicking the titlebar or the rect.

I suspect this is related to the off-main-thread animation stuff that dzbarsky was doing, since that was targeted at a few properties including opacity.
Comment 1 Daniel Holbert [:dholbert] 2012-08-29 18:05:26 PDT
Created attachment 656696 [details]
semi-reference case 1

Here's a reference case, with the subtle opacity animation removed. This shows the (rapid) color animation that should be visible in the testcase.
Comment 2 Daniel Holbert [:dholbert] 2012-08-29 18:39:50 PDT
mozregression output (for m-c):
Last good nightly: 2012-08-02
First bad nightly: 2012-08-03
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=588424024294&tochange=89dcadd42ec4

And here's the pushlog for the regression on mozilla-inbound (found by hand using "mozilla-inbound-debug" nightlies):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=59d1993da7e1&tochange=cb017d051029

So -- looks like dzbarsky is innocent after all. :)

New suspect: jwatt, whose checkin for Bug 776054 (enable display-list-based SVG painting) is in both pushlogs
Comment 3 Daniel Holbert [:dholbert] 2012-08-29 18:40:30 PDT
Flagging tracking-firefox17, since this is a functional regression in Firefox 17.
Comment 4 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-09-05 02:51:22 PDT
Yeah, flipping svg.display-lists.painting.enabled on/off breaks/fixes this.
Comment 5 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-09-05 03:22:49 PDT
Created attachment 658433 [details]
clearer testcase

What's happening here is that when an SVG element is being rendered into an active GFX layer, we aren't properly invalidating for changes to that element (or its descendants).

This is almost certainly due to the way that nsSVGUtils::InvalidateBounds works. It transforms the dirty area up the parent chain taking filters into account until it reaches an nsSVGOuterSVGFrame, and invalidates there. Unfortunately that isn't right now that SVG display lists are enabled, since enabling SDL means we can now get active layers for SVG content. With nsSVGUtils::InvalidateBounds as it is, we skip invalidation of any such layers along the parent chain on the way to the nsSVGOuterSVGFrame. :-/
Comment 6 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-03 15:47:16 PDT
Jonathan - can you confirm your ability to prioritize work on this in the coming 5 weeks?  We're about to put 17 on to the Beta channel and so it's time to decide if this is still something to be tracked - SVG bugs have a higher prominance with webapps so please let us know if you can't work on this, who might be a good person to pass it to.
Comment 7 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-10-03 17:08:35 PDT
I'm pretty sure I should be able to have a fix for this for 17.
Comment 8 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-10-15 03:31:48 PDT
Created attachment 671371 [details] [diff] [review]
patch for mozilla-beta (17)

The code on m-c is quite different to branches now that DLBI has landed. It doesn't seem like a good use of time to write a complete fix for branches and a completely different patch for m-c. I think for branches we should simply prevent active layers in SVG.
Comment 9 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-10-15 16:42:58 PDT
Comment on attachment 671371 [details] [diff] [review]
patch for mozilla-beta (17)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 776054
User impact if declined: broken rendering of SVG
Risk to taking this patch (and alternatives if risky): should be low risk
String or UUID changes made by this patch: none

Testing completed (on m-c, etc.): I've not landed it on m-c since the code is different there making any baking on m-c of minimal value. I propose landing this directly on beta.
Comment 10 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-15 22:43:05 PDT
Comment on attachment 671371 [details] [diff] [review]
patch for mozilla-beta (17)

Since it's very early still in Beta, we can take this Beta-only fix as we'll have time to assess and backout if there are any regressions. Marking this tracking for 18 though and we'll need an equivalent patch (or if the same one works on Aurora too, great) so that we're covered for 18 while the code in 19 rides the trains.
Comment 11 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-15 22:43:59 PDT
Also setting tracking on FF19 so we ensure that the proper fix working with DLBI is landed in trunk.
Comment 12 Scoobidiver (away) 2012-10-16 05:11:32 PDT
Pushed to Beta:
http://hg.mozilla.org/releases/mozilla-beta/rev/7ab34cfe4869
Comment 13 Alex Keybl [:akeybl] 2012-11-19 11:12:30 PST
jwatt - do we need this for FF18?
Comment 14 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-12-05 09:09:15 PST
Sorry, missed that question. Yeah, we need to fix this one in my opinion.
Comment 15 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-12-05 09:40:01 PST
The fix involves getting rid of nsSVGUtils::InvalidateBounds and relying on the DLBI mechanisms for SVG invalidation, which is what bug 802628 is about. I have a partially written but buggy patch for this. I need to talk to mattwoodrow about a few things there.
Comment 16 Jet Villegas (:jet) 2012-12-14 10:30:57 PST
(In reply to Jonathan Watt [:jwatt] from comment #15)
> The fix involves getting rid of nsSVGUtils::InvalidateBounds and relying on
> the DLBI mechanisms for SVG invalidation, which is what bug 802628 is about.
> I have a partially written but buggy patch for this. I need to talk to
> mattwoodrow about a few things there.

I was looking for a r? patch here based on our last call. Any update?
Comment 17 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-12-14 11:54:34 PST
I'm having a little trouble with some test failures. I'm giving up for the evening but should have patches up tomorrow.
Comment 18 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-12-17 12:17:37 PST
My first and second patches from bug 802628 are sufficient to fix this. They've landed on beta, and I'll land them on aurora once it reopens.
Comment 19 Alex Keybl [:akeybl] 2013-01-08 10:00:17 PST
(In reply to Jonathan Watt [:jwatt] from comment #18)
> My first and second patches from bug 802628 are sufficient to fix this.
> They've landed on beta, and I'll land them on aurora once it reopens.

Firefox 19 still appears to be affected. Can we move forward with landing to mozilla-beta?
Comment 20 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2013-01-08 10:17:07 PST
Both patches landed on m-a; see bug 802628 comment 11. I just forgot to update the status flags here. Thanks.
Comment 21 Virgil Dicu [:virgil] [QA] 2013-01-29 07:37:08 PST
Verified the issue with the testcases from comment 0, 1 and 5 in 2012-08-29 Nightly
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0
Build ID: 20120829030520

Verified the fix for Firefox 19.0 beta 3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130123083802

Note You need to log in before you can comment on or make changes to this bug.