725918 - "ASSERTION: No pattern returned from paint server"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: No pattern returned from paint server: '*strokePattern', file layout/svg/base/src/nsSVGGlyphFrame.cpp, line 900

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Robert Longson [:longsonr]]

It's certainly possible for GetPaintServer to return null in which case we need to use the fallback colour.

Comment 3

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch

Comment 4

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch

Now without the dependency on the rest of my patch queue!

Comment 6

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Part 1 - Style fix

Comment 7

[Robert Longson [:longsonr]]

Comment on attachment 596389 [details] [diff] [review]
Patch

Nit: I would have written the code below as a separate method GetStrokePattern which would have allowed early returns in the method whenever I found a valid pattern, but if you want to keep it inline that's OK too.

> 
>+    nsRefPtr<gfxPattern> strokePattern;
>+
>     if (ps) {
>       // Gradient or Pattern: can get pattern directly from frame
>-      *aStrokePattern = ps->GetPaintServerPattern(this, opacity);

Still doesn't seem to match mozilla-inbound or mozilla-central tip as I've got 

      *strokePattern = ps->GetPaintServerPattern(this, opacity);

i.e. *strokePattern rather than *aStrokePattern so this patch doesn't apply or build for me.

>+
>+    *aStrokePattern = strokePattern;

I think you should use

strokePattern.swap(*aStrokePattern);

here.

Comment 8

[Robert Longson [:longsonr]]

Attachment: additional tests

Can you land these too as there aren't any tests fallback and colour/stroke combination tests.

Comment 9

[Robert Longson [:longsonr]]

> 
> i.e. *strokePattern rather than *aStrokePattern so this patch doesn't apply
> or build for me.

OK, I understand now that I've seen the other patch. The usual convention is to label the patches part 1, part 2 etc (see bug 629200 as an example).

Comment 10

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to Robert Longson from comment #7)
> Comment on attachment 596389 [details] [diff] [review]
> >+
> >+    *aStrokePattern = strokePattern;
> 
> I think you should use
> 
> strokePattern.swap(*aStrokePattern);
> 
> here.

Ah cool; I didn't know that was a thing. There should probably be a wiki page written up at some point with all of the handy little nsRefPtr sorcery.

(In reply to Robert Longson from comment #8)
> Created attachment 596438 [details] [diff] [review]
> additional tests
> 
> Can you land these too as there aren't any tests fallback and colour/stroke
> combination tests.

There's no commit message on this patch; do you want me to subsume it into my one?

(In reply to Robert Longson from comment #9)
> > 
> > i.e. *strokePattern rather than *aStrokePattern so this patch doesn't apply
> > or build for me.
> 
> OK, I understand now that I've seen the other patch. The usual convention is
> to label the patches part 1, part 2 etc (see bug 629200 as an example).

Fair enough. Just thought this patch was a bit small to need to do that.

Comment 11

[Robert Longson [:longsonr]]

(In reply to Edwin Flores from comment #10)
> 
> There's no commit message on this patch; do you want me to subsume it into
> my one?

Sure, that seems simplest.

> 
> Fair enough. Just thought this patch was a bit small to need to do that.

My fault for reviewing it back to front really.

Comment 12

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Part 2 - Actual fix

Carrying over r=longsonr

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: Part 3: Pass handle to nsRefPtr outparam using getter_AddRefs

I was going to land this, but I noticed the function taking a pointer-to-nsRefPtr (in existing code -- not Edwin's fault), and that goes against general nsRefPtr best-practices, IIRC.

As long as we're touching this chunk, we might as well clean that up, too.  This patch switches to use getter_AddRefs.

I cribbed from NS_NewComputedDOMStyle to be sure I was setting the outparam correctly ( http://mxr.mozilla.org/mozilla-central/source/layout/style/nsComputedDOMStyle.cpp?mark=127-128#104 )

Comment 14

[Jonathan Watt [:jwatt]]

Comment on attachment 598012 [details] [diff] [review]
Part 3: Pass handle to nsRefPtr outparam using getter_AddRefs

In SetupCairoState, if we take the |if (ps)| path we'll be fine, but if we take the |else| path, then you're failing to addref.

Comment 15

[Jonathan Watt [:jwatt]]

Comment on attachment 598012 [details] [diff] [review]
Part 3: Pass handle to nsRefPtr outparam using getter_AddRefs

Sorry, I forgot you were patching on top of Edwin's patches, which now makes that an nsRefPtr. r=me.

Comment 16

[Daniel Holbert [:dholbert]]

Thanks! I've got these currently going through TryServer, and then I'll push all 3 patches together.

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 596800 [details] [diff] [review]
Part 2 - Actual fix

>+++ b/layout/reftests/svg/fallback-color-04.svg

Bad news -- this reftest currently fails on TryServer (on Linux, at least).  It's a 1px not-visually-perceptible difference -- the testcase has a single pixel of rgb(1,254,0)in a sea of lime (rgb(0,255,0)), while the reference case is entirely lime.

The test needs to be investigated and fixed (or just removed?) before this can land.

Robert, it looks like this is one of the tests you wrote in comment 8 -- what do you think?

Comment 18

[Daniel Holbert [:dholbert]]

Sorry, forgot the tryserver link: https://tbpl.mozilla.org/?tree=Try&rev=ff4c15293a6f
Failure logs:
 https://tbpl.mozilla.org/php/getParsedLog.php?id=9399778&tree=Try
 https://tbpl.mozilla.org/php/getParsedLog.php?id=9399525&tree=Try
 https://tbpl.mozilla.org/php/getParsedLog.php?id=9399517&tree=Try

(Removing checkin-needed for now, 'cause we don't want this to be checked in with a failing test.)

Comment 19

[Robert Longson [:longsonr]]

Best just drop that test. I'll fix it later.

Comment 20

[Robert Longson [:longsonr]]

Trying with stroke-width="4" in the final test: http://hg.mozilla.org/try/pushloghtml?changeset=829db36753af

Comment 21

[Robert Longson [:longsonr]]

Attachment: updated tests

Comment 22

[Robert Longson [:longsonr]]

Updated tests passed on Try

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson from comment #22)
> Updated tests passed on Try

(on Linux, that is)

Nice!  Since pixel-fuzziness can be a platform-dependent thing, I'll give it an all-platform reftest try run, for good measure.  Assuming that passes, I'll land everything here.  (If it fails, I'll just land w/out that test)

Comment 24

[Daniel Holbert [:dholbert]]

That try-run was:
 https://tbpl.mozilla.org/?tree=Try&rev=1aa9a1d8aae1
Unfortunately, it looks like fallback-color-04.svg still fails on mac, with an 18px difference:
 https://tbpl.mozilla.org/php/getParsedLog.php?id=9427314&tree=Try

I'll just land this without that reftest, per comment 19.

Comment 25

[Daniel Holbert [:dholbert]]

part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/829fa4d266ac
part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/f89c186b8143 (-1 reftest)
part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/4ab82ee636d6

Comment 26

[Robert Longson [:longsonr]]

and the missing test... https://hg.mozilla.org/integration/mozilla-inbound/rev/161d2a62a563

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/829fa4d266ac
https://hg.mozilla.org/mozilla-central/rev/f89c186b8143
https://hg.mozilla.org/mozilla-central/rev/4ab82ee636d6

(Leaving open for the test to merge).

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/161d2a62a563

Comment 29

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part 4: forget()

Comment 30

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/82da956a911c