Last Comment Bug 725918 - "ASSERTION: No pattern returned from paint server"
: "ASSERTION: No pattern returned from paint server"
Status: RESOLVED FIXED
: assertion, regression, testcase
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla13
Assigned To: Edwin Flores [:eflores] [:edwin]
:
:
Mentors:
Depends on: 719288
Blocks: stirdom
  Show dependency treegraph
 
Reported: 2012-02-09 19:49 PST by Jesse Ruderman
Modified: 2012-02-22 13:09 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (95 bytes, image/svg+xml)
2012-02-09 19:49 PST, Jesse Ruderman
no flags Details
stack trace (10.12 KB, text/plain)
2012-02-09 19:49 PST, Jesse Ruderman
no flags Details
Patch (3.12 KB, patch)
2012-02-10 13:27 PST, Edwin Flores [:eflores] [:edwin]
no flags Details | Diff | Splinter Review
Patch (2.50 KB, patch)
2012-02-11 14:02 PST, Edwin Flores [:eflores] [:edwin]
no flags Details | Diff | Splinter Review
Patch (2.70 KB, patch)
2012-02-11 14:40 PST, Edwin Flores [:eflores] [:edwin]
longsonr: review+
Details | Diff | Splinter Review
Part 1 - Style fix (3.26 KB, patch)
2012-02-11 14:41 PST, Edwin Flores [:eflores] [:edwin]
longsonr: review+
Details | Diff | Splinter Review
additional tests (4.15 KB, patch)
2012-02-12 01:37 PST, Robert Longson
no flags Details | Diff | Splinter Review
Part 2 - Actual fix (6.86 KB, patch)
2012-02-13 14:50 PST, Edwin Flores [:eflores] [:edwin]
edwin: review+
Details | Diff | Splinter Review
Part 3: Pass handle to nsRefPtr outparam using getter_AddRefs (2.77 KB, patch)
2012-02-16 14:54 PST, Daniel Holbert [:dholbert]
jwatt: review+
Details | Diff | Splinter Review
updated tests (3.35 KB, patch)
2012-02-17 08:41 PST, Robert Longson
no flags Details | Diff | Splinter Review
Part 4: forget() (955 bytes, patch)
2012-02-20 14:42 PST, :Ms2ger (⌚ UTC+1/+2)
dholbert: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-02-09 19:49:11 PST
Created attachment 595942 [details]
testcase

###!!! ASSERTION: No pattern returned from paint server: '*strokePattern', file layout/svg/base/src/nsSVGGlyphFrame.cpp, line 900
Comment 1 Jesse Ruderman 2012-02-09 19:49:44 PST
Created attachment 595943 [details]
stack trace
Comment 2 Robert Longson 2012-02-10 04:52:15 PST
It's certainly possible for GetPaintServer to return null in which case we need to use the fallback colour.
Comment 3 Edwin Flores [:eflores] [:edwin] 2012-02-10 13:27:30 PST
Created attachment 596158 [details] [diff] [review]
Patch
Comment 4 Edwin Flores [:eflores] [:edwin] 2012-02-11 14:02:50 PST
Created attachment 596385 [details] [diff] [review]
Patch
Comment 5 Edwin Flores [:eflores] [:edwin] 2012-02-11 14:40:16 PST
Created attachment 596389 [details] [diff] [review]
Patch

Now without the dependency on the rest of my patch queue!
Comment 6 Edwin Flores [:eflores] [:edwin] 2012-02-11 14:41:04 PST
Created attachment 596390 [details] [diff] [review]
Part 1 - Style fix
Comment 7 Robert Longson 2012-02-12 01:35:43 PST
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 2012-02-12 01:37:06 PST
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.
Comment 9 Robert Longson 2012-02-12 02:16:18 PST
> 
> 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 [:eflores] [:edwin] 2012-02-12 14:23:57 PST
(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 2012-02-12 23:45:04 PST
(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 [:eflores] [:edwin] 2012-02-13 14:50:49 PST
Created attachment 596800 [details] [diff] [review]
Part 2 - Actual fix

Carrying over r=longsonr
Comment 13 Daniel Holbert [:dholbert] 2012-02-16 14:54:07 PST
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 )
Comment 14 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-02-16 15:46:11 PST
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] (back in October - email directly if necessary) 2012-02-16 15:53:52 PST
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] 2012-02-16 15:57:20 PST
Thanks! I've got these currently going through TryServer, and then I'll push all 3 patches together.
Comment 17 Daniel Holbert [:dholbert] 2012-02-16 16:44:21 PST
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] 2012-02-16 16:45:48 PST
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 2012-02-17 07:41:19 PST
Best just drop that test. I'll fix it later.
Comment 20 Robert Longson 2012-02-17 08:40:25 PST
Trying with stroke-width="4" in the final test: http://hg.mozilla.org/try/pushloghtml?changeset=829db36753af
Comment 21 Robert Longson 2012-02-17 08:41:14 PST
Created attachment 598253 [details] [diff] [review]
updated tests
Comment 22 Robert Longson 2012-02-17 12:37:38 PST
Updated tests passed on Try
Comment 23 Daniel Holbert [:dholbert] 2012-02-17 14:11:54 PST
(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] 2012-02-17 16:23:44 PST
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 26 Robert Longson 2012-02-18 08:06:27 PST
and the missing test... https://hg.mozilla.org/integration/mozilla-inbound/rev/161d2a62a563
Comment 28 Ed Morley [:emorley] 2012-02-20 10:25:46 PST
https://hg.mozilla.org/mozilla-central/rev/161d2a62a563
Comment 29 :Ms2ger (⌚ UTC+1/+2) 2012-02-20 14:42:08 PST
Created attachment 598969 [details] [diff] [review]
Part 4: forget()
Comment 30 :Ms2ger (⌚ UTC+1/+2) 2012-02-22 13:09:59 PST
https://hg.mozilla.org/mozilla-central/rev/82da956a911c

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