853889 - SVG path with more than one subpath can render incorrectly

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Eugenio Marchiori]

Attachment: test-svg.html

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.31 (KHTML, like Gecko) Chrome/26.0.1410.40 Safari/537.31

Steps to reproduce:

Tried to render a path with two subpaths (i.e. d=".... Z ... Z"). This subpath was generated by code, from the conversion of a Flash ad.


Actual results:

When one of the subpaths is outside the viewBox of the SVG, the whole path (i.e. the part that is inside the viewBox and visible) is rendered incorrectly.


Expected results:

In the attached example, it should render a green bar (one of the subpaths) but instead it renders a red bar that should be covered by it.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Works for me on Windows using Firefox nightly. Would you try a build from https://nightly.mozilla.org

Comment 2

[Eugenio Marchiori]

Tested in both current and nighties for Linux and it does not work.

I had missed that it does work in Mac OS (and apparently in Windows as well).

Comment 3

[Tom Klein]

Attachment: testcase2.svg

This looks to be a cairo bug; the attached slightly modified testcase shows the broken behavior a little more clearly:

when drawing the green path, in _cairo_path_fixed_fill_rectilinear_to_boxes we take the else leg where _cairo_path_fixed_iter_is_fill_box returns a box for the first subpath that has its p1 to the right of its p2.  _cairo_traps_tessellate_rectangle adds that box to |traps| as a trap with its left side to the right of its right side, and then sends |traps| with one element to _cairo_bentley_ottmann_tessellate_rectangular_traps (the other subpath got clipped out).
_cairo_bentley_ottmann_tessellate_rectangular_traps doesn't do anything if there's only one trap, so |traps| gets returned unadjusted.  Eventually it makes its way to _cairo_xlib_surface_fill_rectangles which calls XRenderFillRectangle on a rectangle with a negative width (because the sides of the trap were switched), only XRenderFillRectangle takes width as unsigned, so we get the very wide green rectangle going in the wrong direction.

Comment 4

[Tom Klein]

Attachment: bug853889.diff

I submitted a patch that fixes this to cairo (the same as the attached patch):
https://bugs.freedesktop.org/show_bug.cgi?id=90984

but note that this bug isn't actually reproducible in the current cairo code (though the patch still seems relevant), i.e. this works upstream, but it seems to me the original bug/hole still exists.  I'll update once I hear back from them.

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

Milan would you review or suggest a reviewer?

Comment 6

[Tom Klein]

Attachment: bug853889.diff

Added a reftest.

Comment 7

[Tom Klein]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e51a0f3b4d0

Getting some intermittent failures.

M-bc2 and M-bc3 failed on the first run, but I ran them again and they passed.  Plus the failures appear to be known issues, though one failure report is marked as WORKSFORME...

M-dt8 failed twice on the try, with different tests failing on each run, but all the failures seem to be known issues, though one of the failure reports has been marked as FIXED...

So a) am I supposed to do anything with the failures that have been marked as WORKSFORME and FIXED?
b) Does the fact that these are all known issues mean that effectively I can ignore them?  (Especially since all of them work for me locally on my linux debug build (pulled to the most recent code)...)

Thanks

Comment 8

[Tom Klein]

Trying a new linux debug mochitest try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a4c7a7e637c

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/264d12cfb073

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android 853889-1.html reftest failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=10889094&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c47e1ef6c58

Comment 11

[Ryan VanderMeulen [:RyanVM]]

And B2G

Comment 12

[Tom Klein]

Attachment: bug853889.diff

Apparently android and b2g were failing on a similar bug in _cairo_bentley_ottmann_tessellate_boxes that has already been fixed in upstream cairo, so I added their fix to this patch - do I need to indicate that in some other way? i.e. that that's not my code - and the try runs look good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c962c38e93f0
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a2b369391e2 (missed Android 2.3 on the first try)

Comment 13

[Jeff Muizelaar [:jrmuizel]]

(In reply to Tom Klein from comment #12)
> Created attachment 8624177 [details] [diff] [review]
> bug853889.diff
> 
> Apparently android and b2g were failing on a similar bug in
> _cairo_bentley_ottmann_tessellate_boxes that has already been fixed in
> upstream cairo, so I added their fix to this patch - do I need to indicate
> that in some other way? i.e. that that's not my code - and the try runs look
> good:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c962c38e93f0
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a2b369391e2 (missed
> Android 2.3 on the first try)

If you can reference the upstream commit hash in the commit message that would be great.

Comment 14

[Tom Klein]

Attachment: bug853889.diff

Updated the commit message to include the cairo commit hash for the _cairo_bentley_ottmann_tessellate_boxes part of this patch.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8624446 [details] [diff] [review]
bug853889.diff

In the future, please just use the checkin-needed bug keyword. It works better with the various automated bug marking tools.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e64c57bd478f

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e64c57bd478f