1179608 - SVG position different from 39 and 40.

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

[Mantaroh Yoshinaga[:mantaroh]]

I guess that SVG behavior of Firefox 41 is different from 40.

Step of reproduce:
1) Open the Foxkeh wallpaper site[1] on Nightly(42.0a1 2015-07-01)
2) Click the 'Preview' button.

Actually Result:
 -  can't display the background.
 -  if repeat the reproduce step, position x of background is misaligned to the left.

Expected Result:
 -  can display the background full.

Additional Information:
 - Firefox 39 can display as good.
 - Foxkeh wallpaper site used transform attribute(translate).
 - Foxkeh wallpaper site used the getBBox.

[1] http://wallpapers.foxkeh.com/en/creator/pc.xhtml?size=851x315&x=142&y=21

Comment 1

[Daniel Holbert [:dholbert]]

Does the bug go away in Nightly if you disable the pref "svg.transform-origin.enabled"?

If so, caused by bug 923193 (which flipped that pref for Firefox 41).

(I briefly tried to answer my own question, but I'm not 100% sure I'm actually seeing the bug; hence, needinfo=reporter)

Comment 2

[Mantaroh Yoshinaga[:mantaroh]]

dholbert,

I tried to disable the "svg.transform-origin.enabled".
But phenomenon is not changed.
(can't display the SVG background pics)

BTW, This application use the Viewbox.
So I think that this bug caused by calculating the background svg position.

Comment 3

[Alice0775 White]

[Tracking Requested - why for this release]:


https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=6deee758b43c&tochange=605d1ea1ff14

Suspect: 605d1ea1ff14	Jonathan Watt — Bug 1159053 - Cache SVG getBBox and objectBoundingBox calculations for better performance. r=heycam

Comment 4

[Kate Glazko]

Tracking because regression and affected in 40, 41, 42.

Comment 5

[Lawrence Mandel [:lmandel] (use needinfo)]

Jet - We're shipping this regression in Firefox 40. Do you have someone who can address this regression in 41?

Comment 6

[Jet Villegas (inactive)]

To Jonathan for a look...

Comment 7

[Daniel Holbert [:dholbert]]

If someone can come up with a reduced testcase here, that would greatly help with figuring out where the bug is.

I looked at this bug for a bit earlier today. Based on the regressing bug, it seems like there must be a fateful getBBox() call that's being made by JS at some point, and we're returning the wrong value to that call.  The page is a bit complex, though, and there seem to be tons of calls to GetBBox based on a quick GDB session that I did; and the ones that I saw go by seemed to be valid. (The cached value from Bug 1159053 was the same as the value that we'd return if there weren't anything cached.)  So, a smaller testcase would definitely help.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Firefox 40 will ship with this regression.

Comment 9

[Jonathan Watt [:jwatt]]

I spent a good long time unsuccessfully trying to just get a local version of the site that reproduces the issue. Brian is going to try and get hold of the maintainer to help with that. Brian, can you needinfo me once that has happened? Thanks!

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I made a locally runnable version of the site you can download here:

  http://people.mozilla.org/~bbirtles/bugs/1179608/creator.zip

The source is actually available here:

  https://github.com/foxkeh/wallpapers.foxkeh.com

But it does a bunch of XHR requests for JSON resources which won't work when you load the site as a local file (we don't allow parsing of JSON file: resources loaded using XHR). I've rewritten the JS to include the needed JSON resources locally so you should be able to open this up as a file and run it. It won't work in Chrome, however, since Chrome won't load the local SVG resources. If you need to test with Chrome you'll let to set up a local web server to get it going.

I've also stripped out a lot of unneeded stuff and simplified the folder structure but I'll leave it to you to further simplify from there.

Comment 11

[Jonathan Watt [:jwatt]]

Attachment: reduced test

Thanks, Brian. That was really helpful.

I've further reduced the zip from 10 MB to about 100 KB. Still some way to go though.

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: reduced test

There are still a lot of moving parts, but this is now sufficiently reduced to be able to make some more progress on figuring out what's going wrong.

Comment 13

[Jonathan Watt [:jwatt]]

Attachment: add debugging assertions

Comment 14

[Jonathan Watt [:jwatt]]

Maybe the |!(aFlags & nsSVGUtils::eBBoxBypassCache)| should just be a comment.

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8646962 [details] [diff] [review]
add debugging assertions

Review of attachment 8646962 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/nsSVGUtils.cpp
@@ +922,5 @@
>  
>      FrameProperties props = aFrame->Properties();
>  
> +    if (aFlags == eBBoxIncludeFillGeometry &&
> +        !(aFlags & nsSVGUtils::eBBoxBypassCache)) {

The second aFlags check here doesn't make sense, because directly above it we're already ensuring that aFlags is exactly eBBoxIncludeFillGeometry (no other bits are set).

So, I suspect one of these checks is wrong... (either the first one wants to use "&" to allow for other bits being set, or the second one is trivially satisfied & hence useless. Maybe this is what you're alluding to in comment 14?)

It probably is worth mentioning somewhere in a comment that the eBBoxBypassCache flag is *purely* to get us past this equality check, if that's indeed what you're going for... (otherwise it's a bit confusing because we don't actually check for it in the logic anywhere)

Comment 16

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #15)
> Maybe this is what you're alluding to in comment 14?)

Indeed.

Comment 17

[Jonathan Watt [:jwatt]]

Attachment: add debugging assertions

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8649352 [details] [diff] [review]
add debugging assertions

Looks good!

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Jonathan, do you want to check this into mozilla-central? I believe you need to set the checkin-needed keyword. Also do you want to request patch for uplift to Beta and Aurora after that?

Comment 20

[Jonathan Watt [:jwatt]]

No, not yet. There's not much point in making people using debug builds crash when we have a testcase that reproduces the problem already.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Seems too late to take a fix in 41 for this.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

We shipped two releases with this bug, nothing new for a month, I don't see the point of tracking it.

Comment 23

[Ciprian Muresan [:cmuresan], Ecosystem QA]

Removing the testcase-wanted keyword considering the requested testcase was provided in Comment 12.

Comment 24

[Alice0775 White]

I can reproduce the issue on Firefox40.
And It works for me on Nightly62.0a1, 52ESR. The bug was fixed on Firefox48.

Comment 25

[Jonathan Watt [:jwatt]]

Thanks, Alice!