By default, Bugzilla does not search the list of RESOLVED bugs.
You can force it to do so by putting the upper-case word ALL in front of your search query, e.g.: ALL tdelibs
We recommend searching for bugs this way, as you may discover that your bug has already been resolved and fixed in a later release.

Bug 554

Summary: art_render_gradient.c makes kicker and konqueror crash
Product: TDE Reporter: Laurent Dard <f.couperin>
Component: tdebaseAssignee: Timothy Pearson <kb9vqf>
Status: RESOLVED FIXED    
Severity: blocker CC: bugwatch, darrella, inukaze.otaku, james, lucmove, petriai, slavek.banko
Priority: P5    
Version: 3.5.12 [Trinity]   
Hardware: amd64   
OS: Linux   
Compiler Version: TDE Version String:
Application Version: Application Name:
Bug Depends on:    
Bug Blocks: 706    
Attachments: Removes assert() statements
Potential fix for segfault
kicker SIGSEGV backtrace
Potential fix for segfault
art_render_gradient.hc.diff

Description Laurent Dard 2011-10-26 09:35:14 CDT
Hello,

It seems there's something wrong in art_render_gradient.c.
I suffer from the same bug in kicker and konqueror.

* konqueror:
If I rightclick on a deb file in a folder, it crashes with:

$ konqueror: art_render_gradient.c:337: art_render_gradient_linear_render_8: Assertion `(stops[ix-1].offset <= offset_fraction + 1e-6) || ((stops[ix].offset > (1.0 - 1e-6)) && (offset_fraction < 1e-6 ))' failed.
X Error: BadWindow (invalid Window parameter) 3
  Major opcode:  19
  Minor opcode:  0
  Resource id:  0x1a00008
KCrash: Application 'konqueror' crashing...
X Error: BadWindow (invalid Window parameter) 3
  Major opcode:  20
  Minor opcode:  0
  Resource id:  0x1a00008
X Error: BadWindow (invalid Window parameter) 3
  Major opcode:  19
  Minor opcode:  0
  Resource id:  0x1a00008


* kicker:
If I try to open the System menu, it crashes with:

$ QWidget::setMinimumSize: The smallest allowed size is (0,0)
kicker: art_render_gradient.c:337: art_render_gradient_linear_render_8: Assertion `(stops[ix-1].offset <= offset_fraction + 1e-6) || ((stops[ix].offset > (1.0 - 1e-6)) && (offset_fraction < 1e-6 ))' failed.
kicker: crashHandler called
KCrash: Application 'kicker' crashing...
QWidget::setMinimumSize: The smallest allowed size is (0,0)
X Error: BadWindow (invalid Window parameter) 3
  Major opcode:  20
  Minor opcode:  0
  Resource id:  0x1a00008
X Error: BadWindow (invalid Window parameter) 3
  Major opcode:  19
  Minor opcode:  0
  Resource id:  0x1a00008


(No problem on Debian testing/unstable.)

System Info:
Linux 3.0.0-12-generic x86_64 GNU/Linux
Ubuntu 11.10
kdebase:
	konqueror-kde3  4:3.5.12-0ubuntu7+r1181781
	kicker-kde3     4:3.5.12-0ubuntu7+r1181781

-- 
Laurent
Comment 1 Laurent Dard 2011-10-26 10:14:27 CDT
Maybe related to this old discussion:
http://bugs.kde.org/show_bug.cgi?id=142214
 (bug not solved in kde3)

Maybe a patch here:
http://lists.freebsd.org/pipermail/freebsd-ports-bugs/2003-August/010101.html
Comment 2 Timothy Pearson 2011-10-26 11:37:06 CDT
We are currently in a hard code freeze pending the 3.5.13 release, and therefore cannot incorporate such a large patch into the Trinity sources at this time.

This bug report will be revisited once the repository thaws for 3.5.14 development.
Comment 3 Laurent Dard 2011-10-26 11:57:34 CDT
Thanks.
I'll try the nightly builds in the meantime.
The bug involves libart-lgpl and svg files.
I'll take a look, if I can.
Comment 4 Laurent Dard 2011-10-27 11:45:16 CDT
Some more informations.

The patch mentioned above is useless (already applied). The bug was solved in libart itself and in Debian, years ago (since libart-2.0-2 2.3.16-1). With Google, I found a lot of old (2003-2004) bug reports involving librsvg and "art_render_gradient.c:337" from libart_lgpl.

This bug should be dead and it comes back like a ghost in TDE.

I think that there is something, somewhere, that is linked against a static library from an outdated libart_lgpl. I'm looking for it but I didn't find it for the moment.


(Some history:
http://bugzilla.gnome.org/121850
http://bugzilla.gnome.org/155472)
Comment 5 Laurent Dard 2011-10-27 15:04:46 CDT
Created attachment 111 [details]
Removes assert() statements
Comment 6 Laurent Dard 2011-10-27 15:08:03 CDT
I found a solution.
It's here: https://bugzilla.gnome.org/show_bug.cgi?id=155472#c1

I applied the above attached patch, recompiled and installed libart-2.0-2, and
the bug is gone. There is only harmless messages on the console:
'art_render_gradient.c:359: Old assert() failed!'

It's a patch for libart_lgpl that is no more actively maintained by the Gnome
project. I'll forward it to Debian.

In the meanwhile, when you'll get time, do you think it's possible to include a
patched libart_lgpl in the dependencies of TDE?
Comment 7 Timothy Pearson 2011-10-27 15:36:32 CDT
(In reply to comment #6)
> I found a solution.
> It's here: https://bugzilla.gnome.org/show_bug.cgi?id=155472#c1
> 
> I applied the above attached patch, recompiled and installed libart-2.0-2, and
> the bug is gone. There is only harmless messages on the console:
> 'art_render_gradient.c:359: Old assert() failed!'
> 
> It's a patch for libart_lgpl that is no more actively maintained by the Gnome
> project. I'll forward it to Debian.
> 
> In the meanwhile, when you'll get time, do you think it's possible to include a
> patched libart_lgpl in the dependencies of TDE?

If the upstream project is truly abandoned then yes, we might as well absorb this tiny little library.  However, this will not happen for 3.5.13 due to the close timing of the release--you can compile a patched version for your distribution for 3.5.13 if you so desire.

Tim
Comment 8 Timothy Pearson 2011-11-07 15:52:31 CST
*** Bug 584 has been marked as a duplicate of this bug. ***
Comment 9 Laurent Dard 2011-11-23 13:55:23 CST
marked as patchavail but the package is not yet in trinity

original sources are here:
http://ftp.gnome.org/pub/gnome/sources/libart_lgpl/2.3/libart_lgpl-2.3.21.tar.bz2

Upstream is here: http://www.levien.com/libart/

I'm sending a mail to the developer, Raph Levien.
Comment 10 Timothy Pearson 2011-12-07 19:34:28 CST
(In reply to comment #9)
> marked as patchavail but the package is not yet in trinity
> 
> original sources are here:
> http://ftp.gnome.org/pub/gnome/sources/libart_lgpl/2.3/libart_lgpl-2.3.21.tar.bz2
> 
> Upstream is here: http://www.levien.com/libart/
> 
> I'm sending a mail to the developer, Raph Levien.

libart has been officially abandoned as of the middle of the year 2011 by its last maintainer, the Gnome project.  In light of this I have added libart-lgpl to our dependencies tree, and applied a slew of ignored patches to it (including the one linked to above) in order to resolve a number of problems.

The major complaint about libart is something about numerical instability.  Whether that affects TDE I don't know, but I have not personally seen any problems that could be clearly attributed to libart outside of this crash.
Comment 11 Timothy Pearson 2011-12-19 13:39:47 CST
*** Bug 429 has been marked as a duplicate of this bug. ***
Comment 12 Timothy Pearson 2012-01-10 17:11:21 CST
*** Bug 591 has been marked as a duplicate of this bug. ***
Comment 13 James 2012-03-10 02:25:32 CST
Trinity Version 3.5.13
In Debian Unstable, which is using libart-2.0-2, version 2.3.21-1, which has "Apr 26  2010 /usr/lib/libart_lgpl_2.so.2.3.21"

This bug is now causing the KMenu applet in Kicker to crash Kicker when mousing-over certain menu items, specifically "Settings" or "System".  Some image file may have been changed, triggering this bug now, though I don't know.  Of course, with this, using KMenu means crashing the entire Kicker panel.

...
X Error: BadWindow (invalid Window parameter) 3
  Major opcode:  20
  Minor opcode:  0
  Resource id:  0x2200023
X Error: BadWindow (invalid Window parameter) 3
  Major opcode:  20
  Minor opcode:  0
  Resource id:  0x2200023
X Error: BadWindow (invalid Window parameter) 3
  Major opcode:  20
  Minor opcode:  0
  Resource id:  0x2200023
X Error: BadWindow (invalid Window parameter) 3
  Major opcode:  19
  Minor opcode:  0
  Resource id:  0x2200023
*** attempt to put segment in horiz list twice
*** attempt to put segment in horiz list twice
kicker: art_render_gradient.c:337: art_render_gradient_linear_render_8: Assertion `(stops[ix-1].offset <= offset_fraction + 1e-6) || ((stops[ix].offset > (1.0 - 1e-6)) && (offset_fraction < 1e-6 ))' failed.
kicker: crashHandler called
...

For the curious, recounting the history since 2004 and beyond, here is another reference:

 http://www.mail-archive.com/blfs-dev@linuxfromscratch.org/msg11430.html


I don't suppose that this bug should be called "fixed" until the fix has been incorporated into the current release.

It is important to note that, in typical Debian fashion, despite this bug-fix having been available since 2004 October, over 7 years ago now,

 https://bugzilla.gnome.org/show_bug.cgi?id=155472

and despite Laurent Dard's bug report to the Debian Maintainers last 2011 October, more than 4 months ago now,

 http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=271111

there has been absolutely no action taken by Debian to upgrade the package.  Where even the Debian "Unstable" repository, preceding "Testing", preceding "Stable", can be _years_ and even two major releases behind a "current" package, as it recently was, posting only "KDE SC 4.6" when "KDE SC 4.8" had already been released, it may be appropriate for Trinity to make use of its own "libart_lgpl_2.so.2".


James
Comment 14 James 2012-03-10 05:06:45 CST
Hmm - I tried installing today's

 libart-2.0-2_14.0.0-0debian6+r2+pr6~wheezy_i386.deb

and this causes Kicker from 3.5.13 to crash on start-up, getting only as far as creating a window for the panel.

The KDE Crash Handler only has

[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/i386-linux-gnu/i686 cmov/libthread_db.so.1".
[KCrash handler]
#6  __memcpy_ia32 () at ../sysdeps/i386/i686/multiarch/../memcpy.S:75
#7  0x101fd060 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)

Recompiling the source locally produces the same result.

However, starting with Laurent's libart_lgpl-2.3.21.tar.bz2, applying the patch libart-remove-asserts.diff, and compiling normally produces a libart_lgpl_2.so.2.3.21 which seems to work, with no KMenu crash, instead producing a list of errors, only once when first running KMenu and mousing-over the "Settings" and "System" items, but also first popping-up an "Error - KDE Panel" window when clicking on the KMenu icon, again only once when first selecting KMenu,

 Could not start process Unable to create io-slave:
 klauncher said: Error loading 'kio_system'.

then

 X Error: BadWindow (invalid Window parameter) 3
  Major opcode:  20
  Minor opcode:  0
  Resource id:  0x4800966
X Error: BadWindow (invalid Window parameter) 3
  Major opcode:  20
  Minor opcode:  0
  Resource id:  0x4800966
X Error: BadWindow (invalid Window parameter) 3
  Major opcode:  20
  Minor opcode:  0
  Resource id:  0x4800966
X Error: BadWindow (invalid Window parameter) 3
  Major opcode:  19
  Minor opcode:  0
  Resource id:  0x4800966
X Error: BadWindow (invalid Window parameter) 3
  Major opcode:  20
  Minor opcode:  0
  Resource id:  0x4800966
X Error: BadWindow (invalid Window parameter) 3
  Major opcode:  20
  Minor opcode:  0
  Resource id:  0x4800966
X Error: BadWindow (invalid Window parameter) 3
  Major opcode:  20
  Minor opcode:  0
  Resource id:  0x4800966


and then, for the mouse-over,


*** attempt to put segment in horiz list twice
*** attempt to put segment in horiz list twice
art_render_gradient.c:359: Old assert() failed!
art_render_gradient.c:359: Old assert() failed!
art_render_gradient.c:359: Old assert() failed!
art_render_gradient.c:359: Old assert() failed!
art_render_gradient.c:359: Old assert() failed!
art_render_gradient.c:359: Old assert() failed!
art_render_gradient.c:359: Old assert() failed!
art_render_gradient.c:359: Old assert() failed!
art_render_gradient.c:359: Old assert() failed!
art_render_gradient.c:359: Old assert() failed!
art_render_gradient.c:359: Old assert() failed!
art_render_gradient.c:359: Old assert() failed!
art_render_gradient.c:359: Old assert() failed!
art_render_gradient.c:359: Old assert() failed!
art_render_gradient.c:359: Old assert() failed!
art_render_gradient.c:359: Old assert() failed!
*** attempt to put segment in horiz list twice
*** attempt to put segment in horiz list twice
QImage::smoothScale: Image is a null image

So, presumably, some of the other patches that have been applied to produce  libart-2.0-2_14.0.0-0debian6+r2+pr6~wheezy_i386.deb
are causing some additional problem.  I suppose, Tim, if you want to break-out each of the patches, I could try bisecting to find the one causing the problem, assuming that the original source is the same, or at least not the problem.

Also, does anyone have an ideas about the io-slave/kio_system error with KMenu?


James
Comment 15 James 2012-03-10 05:53:05 CST
Oops - the problem with the io-slave/kio_system and the "Error - KDE Panel" window popping-up has nothing to do with the libart, and went away when I restarted kdeinit.  So please ignore that bit.

I am going to suggest that this bug be reopened, until this other problem with libart is resolved.


James
Comment 16 Timothy Pearson 2012-04-17 01:31:20 CDT
For reference this is the patch that was applied to the stock libart 2.3.21 sources in GIT: http://git.trinitydesktop.org/cgit/libart-lgpl/commit/?id=150562b89b645c402f1bb837a09f8b84bf6e49ec

The patch incorporates all tested patches from the Gnome bugtracker, however I suppose it is possible that one or more of them caused this new segfault to happen.  I would suspect that it is related to the changes to the "art_vpath_render_bez" function, but have never been able to replicate any libart crash on my systems.

James, if you are still willing, can you grab the current TDE libart sources, try to back out the change mentioned above by applying the attached patch in reverse and see if libart still crashes?
Comment 17 Timothy Pearson 2012-04-17 01:32:28 CDT
Created attachment 541 [details]
Potential fix for segfault
Comment 18 James 2012-04-17 15:33:44 CDT
Created attachment 544 [details]
kicker SIGSEGV backtrace

Tim, I rebuilt the library with the art_vpath_bpath.c patch reversed, as suggested.  I get the same SIGSEGV kicker crash.  I did get a backtrace, though, which ends with "#6  art_render_gradient_radial (render=0x9e010e0, gradient=0x9ce7a28, level=ART_FILTER_HYPER) at art_render_gradient.c:731".  I'll attach the backtrace.

James
Comment 19 Timothy Pearson 2012-04-17 15:57:23 CDT
Created attachment 545 [details]
Potential fix for segfault

The backtrace helped some.

Try reversing this patch to see if it cures the crash.

Thanks!
Comment 20 James 2012-04-19 02:07:03 CDT
Alas, no.  I get basically the same crash and backtrace, ending similarly, this time with,

 #6  art_render_gradient_radial (render=0x9f031b8, gradient=0x9f35a18, level=ART_FILTER_HYPER) at art_render_gradient.c:710

With "strace -f", there is,

 ...
 access("/usr/share/icons/hicolor/scalable/animations/kdict.svgz", R_OK) = -1 ENOENT (No such file or directory)
 access("/opt/trinity/share/icons/hicolor/scalable/apps/kdict.svgz", R_OK) = 0
 open("/opt/trinity/share/icons/hicolor/scalable/apps/kdict.svgz", O_RDONLY|O_LARGEFILE) = 11
 _llseek(11, 0, [0], SEEK_CUR)           = 0
 read(11, "\37\213\10\0\0\0\0\0\0\3\355\\\373o\334F\222\376\331\371+\270\23\340\260\6H\252\337\17\237\235"..., 8192) = 5220
 read(11, "", 2972)                      = 0
 close(11)                               = 0
 mmap2(NULL, 2366828544, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x28160000
 --- SIGSEGV (Segmentation fault) @ 0 (0) ---

which, I don't know if that tells you anything useful, except that that "length" value does not seem reasonable, "2366828544".  Still, with the "working" version of libart, at this point in the trace, the "access" calls simply continue, with an eventual "mmap2" call as,

 mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xb7719000

What would call "mmap2" in art_render_gradient_radial()?  What about,

 memcpy (image_source->gradient.stops, gradient->stops, sizeof (ArtGradientStop) * gradient->n_stops);

I can only see a single call to art_render_gradient_radial(), in testart.c, from test_render_rad_gradient(), which defines,

 gradient.n_stops = sizeof(stops) / sizeof(stops[0]);

and where

 ArtGradientStop stops[3] = {
    { 0.0, { 0x7fff, 0x0000, 0x0000, 0x7fff }},
    { 0.5, { 0x0000, 0x0000, 0x0000, 0x1000 }},
    { 1.0, { 0x0000, 0x7fff, 0x0000, 0x7fff }}
  };

with, from art_render_gradient.h,

 typedef struct _ArtGradientStop ArtGradientStop;
 ...
 struct _ArtGradientStop {
  double offset;
  ArtPixMaxDepth color[ART_MAX_CHAN + 1];
 };

and, from art_render.h,

 #define ART_MAX_CHAN 16


Hmm - any patches in testart.c?


James
Comment 21 Timothy Pearson 2012-04-19 02:13:11 CDT
Can you upload your known-working source tarball so that I may examine it?

Thanks!
Comment 22 James 2012-04-19 17:09:29 CDT
Created attachment 554 [details]
art_render_gradient.hc.diff

The patched source which "works" is as described above.  The source is from,
 http://ftp.gnome.org/pub/gnome/sources/libart_lgpl/2.3/libart_lgpl-2.3.21.tar.bz2
and the patch is from,
 https://bugzilla.gnome.org/attachment.cgi?id=32629

I finally went through the diff between this patched "working" source and "the stock libart 2.3.21 sources in GIT", as linked above, and found the bit that matters.  The diff between these two is not particularly large, but the bit that matters is exactly one line, in art_render_gradient.h,

 struct _ArtGradientRadial {
    double affine[6]; /* transforms user coordinates to unit circle */
    double fx, fy;    /* focal point in unit circle coords */
 +  ArtGradientSpread spread;
    int n_stops;
    ArtGradientStop *stops;
  };

Now, the patch for art_render_gradient.c will not work without adding "ArtGradientSpread spread;" to "struct _ArtGradientRadial", but more importantly, the addition of this one line to "struct _ArtGradientRadial" _alone_ is sufficient to trigger the SIGSEGV crash.

Note that the backtrace being shown for this crash is the same as above,

 #6  __memcpy_ia32 () at ../sysdeps/i386/i686/multiarch/../memcpy.S:75

and NOT the above "art_render_gradient_radial()" backtrace.

I have attached the diffs for art_render_gradient.h and art_render_gradient.c.  It is sufficient to simply patch the source with these.  But there is still the issue of why adding "ArtGradientSpread spread;" to "struct _ArtGradientRadial" in art_render_gradient.h is triggering this seg-fault.  Perhaps someone more familiar with C abstractions can find the underlying problem, and then, presumably, the patch to art_render_gradient.c could be applied without any further trouble.

Hmm - looking at the one and only call to "art_render_gradient_radial()" from testart.c, all the struct values are being assigned _except_ for this new "ArtGradientSpread spread" value, leaving it uninitialized.  What should be the value of this new "gradient.spread"?  Taking a wild guess, looking at the function just above in testart.c, "test_render_gradient()", there is

 gradient.spread = ART_GRADIENT_PAD;

where art_render_gradient.h has

  typedef enum {
   ART_GRADIENT_PAD,
   ART_GRADIENT_REFLECT,
   ART_GRADIENT_REPEAT
 } ArtGradientSpread;

Actually, I do not suppose that the specific value of "gradient.spread" matters at all here, simply because "art_render_gradient_radial()", which is being called, never evaluates "gradient.spread".

So, I tried defining "gradient.spread" in "test_render_rad_gradient()" in testart.c.  No joy - it makes no difference at all.  In fact, even after also "commenting-out" "test_render_rad_gradient()" and "art_render_gradient_radial()" so that they are never called, the entire backtrace shown is only

 #6  __memcpy_ia32 () at ../sysdeps/i386/i686/multiarch/../memcpy.S:75
 #7  0x494ac060 in ?? ()
 Backtrace stopped: previous frame inner to this frame (corrupt stack?)

Hmm - so then, there are only two other references to the struct "ArtGradientRadial", which was changed by the addition of "ArtGradientSpread spread".  They are both in art_render_gradient.c, in "art_render_gradient_radial_render()" and in

 typedef struct _ArtImageSourceGradRad ArtImageSourceGradRad;
 ...
 struct _ArtImageSourceGradRad {
   ArtImageSource super;
   ArtGradientRadial gradient;
   double a;
   ArtGradientStop stops[1];
 };

And then, there are only two reference to any struct of type "ArtImageSourceGradRad", both, again, in art_render_gradient.c, in, again, "art_render_gradient_radial_render()", and in the, occasionally problematic, "art_render_gradient_radial()".  Since there can be a crash even when "art_render_gradient_radial()" is never called, it should be sufficient to find the mechanism of the crash in "art_render_gradient_radial_render()", which begins

 art_render_gradient_radial_render (ArtRenderCallback *self, ArtRender *render,
                                   art_u8 *dest, int y)
 {
   ArtImageSourceGradRad *z = (ArtImageSourceGradRad *)self;
   const ArtGradientRadial *gradient = &(z->gradient);
 ...

Hmm - that's as far as I can get for now.  Hopefully there is a clue in there somewhere, maybe some inaccurate assumption about the changed "ArtImageSourceGradRad" types.


James
Comment 23 Timothy Pearson 2012-04-19 17:40:41 CDT
I recognize those symptoms! :-)

The patch introduced an accidental ABI incompatibility.  It should have been:

 struct _ArtGradientRadial {
    double affine[6]; /* transforms user coordinates to unit circle */
    double fx, fy;    /* focal point in unit circle coords */
    int n_stops;
    ArtGradientStop *stops;
 +  ArtGradientSpread spread;
  };

Try that and see if the crash is resolved.

Thanks!

Tim
Comment 24 James 2012-04-19 18:57:43 CDT
Ha!  Yes, that does fix the crash problem!

"accidental ABI incompatibility" - very impressive, Tim!  Maybe you have a link to an explanation with more detail?  I'm curious...

So now, maybe somebody would take a look at those old "assert()" tests in art_render_gradient.c, which still spit-up errors on occasion.  Also, there is the occasional "*** attempt to put segment in horiz list twice\n" from art_svp_intersect.c.  And finally, there is an occasional "QImage::smoothScale: Image is a null image", but that is not generated by libart.

While we're at it, to nit-pick, in the README, where you have

 NOTE: The TDE project has take over maintinance of this library,

that should be "taken" and "maintenance",

 NOTE: The TDE project has taken over maintenance of this library,


Thanks!

James
Comment 25 Timothy Pearson 2012-04-19 20:10:58 CDT
All should be fixed in GIT hash 2e6e0a1.  Thanks for taking the time and effort to debug this!

This is a reasonably good Wiki article on ABI issues in C/C++: http://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++

Specifically, "You can..." "append new enumerators to an existing enum.".  Inserting a new item into the middle of an existing enumerator invalidates pointer offsets, causing a crash in many cases, or at best, silent data corruption.

Tim
Comment 26 James 2012-04-19 22:07:05 CDT
So then, I have to wonder, in art_render_gradient.h, in the similar struct,

 struct _ArtGradientLinear {
   double a;
   double b;
   double c;
   ArtGradientSpread spread;
   int n_stops;
   ArtGradientStop *stops;
 };

where "ArtGradientSpread spread;" is, again, in the middle of the sequence, is there also a possibility for the same kind of corruption?  Should that be changed, similarly, to

 struct _ArtGradientLinear {
   double a;
   double b;
   double c;
   int n_stops;
   ArtGradientStop *stops;
   ArtGradientSpread spread;
 };

with the "ArtGradientStop" enum data-type at the end of the struct?

And, there are other enum data-types in structs:
 ArtPathcode in art_bpath.h
 ArtPathcode in art_vpath.h
 ArtPixFormat in art_pixbuf.h
 ArtAlphaType in art_render.h
 ArtCompositingMode in art_render.h


Are there potential problems in these also?

I still don't get quite how the ordering of the struct elements matters.


James
Comment 27 James 2012-05-02 14:00:11 CDT
After more thought and some conversation, it would seem that this problem with the ordering of the struct members arose specifically because this modified library was being used with applications compiled using the _old_ header file.  And so, the _old_ applications would be calculating or assuming memory offsets that would be incompatible with the _new_ stuct, and an "enum" jammed into the middle, displacing the memory positions of the subsequent struct members.  Presumably, then, if all the applications were recompiled with the _new_ header file, there would be no compatibility problems, and no seg-fault.

We might consider, should the "ArtGradientSpread spread;" in "struct _ArtGradientRadial" be moved backed to the middle of the struct, to match the form of "struct _ArtGradientLinear", and all the applications recompiled?  Of course, that is unnecessary, and also causes backward compatibility problems.

Still, should "struct _ArtGradientLinear" be modified to match the form of "struct _ArtGradientRadial", by moving the "ArtGradientSpread spread;" member to the end of the struct?  Again, that would probably cause incompatibilities with existing applications, compiled with the old header file.

So I suppose that the forms of these structs should just be the way they are now, somewhat an accident of history.

Tim, did you want to go ahead and close this bug, and deal with the "art_render_gradient.c:359: Old assert() failed!"
later, if it should ever actually come up in a more tractable context, after the applications and library are all known to be using the same header files?

James
Comment 28 Timothy Pearson 2012-05-02 14:03:01 CDT
(In reply to comment #27)
> After more thought and some conversation, it would seem that this problem with
> the ordering of the struct members arose specifically because this modified
> library was being used with applications compiled using the _old_ header file. 

Correct.

I poked around the 3.5.13 release PPA and could not find an upgraded libart_lgpl package in that PPA.  How did you originally upgrade libart_lgpl from the stock Debian package to the modified version?
Comment 29 James 2012-05-02 15:47:29 CDT
I did not upgrade libart_lgpl from the stock Debian package.  Instead, I followed Laurent Dard, as I have described above, with the source from,
 http://ftp.gnome.org/pub/gnome/sources/libart_lgpl/2.3/libart_lgpl-2.3.21.tar.bz2
and the patch from,
 https://bugzilla.gnome.org/attachment.cgi?id=32629

Subsequently, I used the diff between that result and your stock libart 2.3.21
sources in GIT:
 http://git.trinitydesktop.org/cgit/libart-lgpl/commit?id=150562b89b645c402f1bb837a09f8b84bf6e49ec
to find the cause of the seg-fault.

I then over-wrote the stock debian "/usr/lib/libart_lgpl_2.so.2.3.21" with my locally compiled version, the patched version from Trinity GIT.

Not, myself, knowing any differently, where you say above "The patch incorporates all tested patches from the Gnome bugtracker", I am supposing that you have assembled the "latest and greatest" version of the libart-lgpl library, with that change to the ordering of "struct _ArtGradientRadial", and any updates to the Changelog that you may have made, being the only additional changes of which I am aware.

The debian unstable source files, "orig" and "diff", from
 http://packages.debian.org/source/unstable/libart-lgpl
appear to be missing many of your patches.  There is a patch to support the new debian multi-arch feature, dated 29 Feb 2012,
 http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=661714
but these are just changes to the debian specific build files in the "debian/" subdirectory.

Perhaps you can persuade the debian maintainers to adopt the presumably more functional and up-to-date version of libart-lgpl from Trinity?  I suppose that they should at least be notified, by creating a new debian bug report, even though Laurent has already made note of this Trinity bug report on the debian bug tracker, but under a bug dating from 11 Sep 2004.


James
Comment 30 Timothy Pearson 2012-05-02 17:08:54 CDT
> Not, myself, knowing any differently, where you say above "The patch
> incorporates all tested patches from the Gnome bugtracker", I am supposing that
> you have assembled the "latest and greatest" version of the libart-lgpl
> library, with that change to the ordering of "struct _ArtGradientRadial", and
> any updates to the Changelog that you may have made, being the only additional
> changes of which I am aware.

Your supposition is correct.  I just wanted to make sure that there wasn't a broken version of the binary libart-lgpl package floating around waiting to pounce on unwary users. ;-)

Those who do want to try the updated binary package may download it from this URL:
https://quickbuild.pearsoncomputing.net/~trinity/+archive/trinity-nightly-builds/+packages?field.name_filter=libart-lgpl&field.series_filter=&field.status_filter=published&start=0&batch=300

Tim
Comment 31 Slávek Banko 2012-05-02 17:25:36 CDT
(Odpověď na komentář #30)
> Your supposition is correct.  I just wanted to make sure that there wasn't a
> broken version of the binary libart-lgpl package floating around waiting to
> pounce on unwary users. ;-)
> 
> Those who do want to try the updated binary package may download it from this
> URL:
> https://quickbuild.pearsoncomputing.net/~trinity/+archive/trinity-nightly-builds/+packages?field.name_filter=libart-lgpl&field.series_filter=&field.status_filter=published&start=0&batch=300
> 
> Tim

I understand correctly, it would be useful to include a new package libart also into the upcoming updates for 3.5.13?
Comment 32 Timothy Pearson 2012-05-02 20:07:00 CDT
(In reply to comment #31)
> (Odpověď na komentář #30)
> > Your supposition is correct.  I just wanted to make sure that there wasn't a
> > broken version of the binary libart-lgpl package floating around waiting to
> > pounce on unwary users. ;-)
> > 
> > Those who do want to try the updated binary package may download it from this
> > URL:
> > https://quickbuild.pearsoncomputing.net/~trinity/+archive/trinity-nightly-builds/+packages?field.name_filter=libart-lgpl&field.series_filter=&field.status_filter=published&start=0&batch=300
> > 
> > Tim
> 
> I understand correctly, it would be useful to include a new package libart also
> into the upcoming updates for 3.5.13?

Yes, this would be a good idea.  You can directly copy the libart-lgpl packages from the trinity-nightly-builds PPA into your updates PPA if you want.
Comment 33 Slávek Banko 2012-05-02 21:09:24 CDT
(Odpověď na komentář #32)
> Yes, this would be a good idea.  You can directly copy the libart-lgpl packages
> from the trinity-nightly-builds PPA into your updates PPA if you want.

Copy the packages is a good idea. The package does not depend on anything from Trinity, so no need to modify it for 3.5.13. Nor need not be recompiled.