The default bug view has changed. See this FAQ.

"ASSERTION: No pattern returned from paint server"

RESOLVED FIXED in mozilla13

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: eflores)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
mozilla13
x86
Mac OS X
assertion, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 595942 [details]
testcase

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

Comment 1

5 years ago
Created attachment 595943 [details]
stack trace

Comment 2

5 years ago
It's certainly possible for GetPaintServer to return null in which case we need to use the fallback colour.
Depends on: 719288
Keywords: regression
Assignee: nobody → eflores
Status: NEW → ASSIGNED
Created attachment 596158 [details] [diff] [review]
Patch
Attachment #596158 - Flags: review?(longsonr)
Created attachment 596385 [details] [diff] [review]
Patch
Attachment #596158 - Attachment is obsolete: true
Attachment #596158 - Flags: review?(longsonr)
Attachment #596385 - Flags: review?(longsonr)
Created attachment 596389 [details] [diff] [review]
Patch

Now without the dependency on the rest of my patch queue!
Attachment #596385 - Attachment is obsolete: true
Attachment #596385 - Flags: review?(longsonr)
Attachment #596389 - Flags: review?(longsonr)
Created attachment 596390 [details] [diff] [review]
Part 1 - Style fix
Attachment #596390 - Flags: review?(longsonr)

Comment 7

5 years ago
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.
Attachment #596389 - Flags: review?(longsonr) → review+

Comment 8

5 years ago
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.

Updated

5 years ago
Attachment #596390 - Flags: review?(longsonr) → review+

Comment 9

5 years ago
> 
> 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).
Attachment #596390 - Attachment description: Style fix. Patch depends on this. → Part 1 - Style fix
(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

5 years ago
(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.
Created attachment 596800 [details] [diff] [review]
Part 2 - Actual fix

Carrying over r=longsonr
Attachment #596389 - Attachment is obsolete: true
Attachment #596438 - Attachment is obsolete: true
Attachment #596800 - Flags: review+
Keywords: checkin-needed
Created attachment 598012 [details] [diff] [review]
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 )
Attachment #598012 - Flags: review?
Attachment #598012 - Flags: review? → review?(longsonr)
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.
Attachment #598012 - Flags: review?(longsonr) → review-
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.
Attachment #598012 - Flags: review- → review+
Thanks! I've got these currently going through TryServer, and then I'll push all 3 patches together.
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?
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.)
Keywords: checkin-needed

Comment 19

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

Comment 20

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

Comment 21

5 years ago
Created attachment 598253 [details] [diff] [review]
updated tests

Comment 22

5 years ago
Updated tests passed on Try
(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)
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.
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
Target Milestone: --- → mozilla13

Comment 26

5 years ago
and the missing test... https://hg.mozilla.org/integration/mozilla-inbound/rev/161d2a62a563
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).
https://hg.mozilla.org/mozilla-central/rev/161d2a62a563
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Created attachment 598969 [details] [diff] [review]
Part 4: forget()
Attachment #598969 - Flags: review?(dholbert)
Attachment #598969 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/82da956a911c
You need to log in before you can comment on or make changes to this bug.