[Patch] Patch for fontmanager crash

Hi,

In matplotlib 1.2.1, the get_name function is not garding against none
self (unlike other functions); Unfortunately it seems I have a workload
that makes matplotlib call get_name with None (wasn't the case in 1.2.0).
I couldn't isolate the exact trigger, when I reduce the volume of data
processed the problem goes away so I have to simple shareable reproducer.

Anyway, the following patch makes it all work for me, could it (or
something similar) be merged?

diff -uNr matplotlib-1.2.1.orig/lib/matplotlib/font_manager.py
matplotlib-1.2.1/lib/matplotlib/font_manager.py
--- matplotlib-1.2.1.orig/lib/matplotlib/font_manager.py 2013-03-26
14:04:37.000000000 +0100
+++ matplotlib-1.2.1/lib/matplotlib/font_manager.py 2013-07-08
14:49:37.791845661 +0200
@@ -721,6 +721,8 @@
         Return the name of the font that best matches the font
         properties.
         """
+ if self._family is None:
+ return rcParams['font.family']
         return ft2font.FT2Font(str(findfont(self))).family_name

     def get_style(self):

Regards,

···

--
Nicolas Mailhot

Nicolas Mailhot wrote:

Hi,

In matplotlib 1.2.1, the get_name function is not garding against none
self (unlike other functions); Unfortunately it seems I have a workload
that makes matplotlib call get_name with None (wasn't the case in 1.2.0).
I couldn't isolate the exact trigger, when I reduce the volume of data
processed the problem goes away so I have to simple shareable reproducer.

Anyway, the following patch makes it all work for me, could it (or
something similar) be merged?

diff -uNr matplotlib-1.2.1.orig/lib/matplotlib/font_manager.py
matplotlib-1.2.1/lib/matplotlib/font_manager.py
--- matplotlib-1.2.1.orig/lib/matplotlib/font_manager.py 2013-03-26
14:04:37.000000000 +0100
+++ matplotlib-1.2.1/lib/matplotlib/font_manager.py 2013-07-08
14:49:37.791845661 +0200
@@ -721,6 +721,8 @@
         Return the name of the font that best matches the font
         properties.
         """
+ if self._family is None:
+ return rcParams['font.family']
         return ft2font.FT2Font(str(findfont(self))).family_name

     def get_style(self):

Regards,

Could you instead just test for "if not self._family"? Tests for equality
are more expensive (that means self._family == 0 or self._family == False will
also trigger your return).

I can test if it works. However, all the other tests in that file are
already of the if Foo is None form, and I didn't want to change the coding
style

Regards,

···

Le Lun 8 juillet 2013 18:18, Martin Mokrejs a écrit :

Nicolas Mailhot wrote:

Hi,

In matplotlib 1.2.1, the get_name function is not garding against none
self (unlike other functions); Unfortunately it seems I have a workload
that makes matplotlib call get_name with None (wasn't the case in
1.2.0).
I couldn't isolate the exact trigger, when I reduce the volume of data
processed the problem goes away so I have to simple shareable
reproducer.

Anyway, the following patch makes it all work for me, could it (or
something similar) be merged?

diff -uNr matplotlib-1.2.1.orig/lib/matplotlib/font_manager.py
matplotlib-1.2.1/lib/matplotlib/font_manager.py
--- matplotlib-1.2.1.orig/lib/matplotlib/font_manager.py
2013-03-26
14:04:37.000000000 +0100
+++ matplotlib-1.2.1/lib/matplotlib/font_manager.py 2013-07-08
14:49:37.791845661 +0200
@@ -721,6 +721,8 @@
         Return the name of the font that best matches the font
         properties.
         """
+ if self._family is None:
+ return rcParams['font.family']
         return ft2font.FT2Font(str(findfont(self))).family_name

     def get_style(self):

Regards,

Could you instead just test for "if not self._family"? Tests for equality
are more expensive (that means self._family == 0 or self._family == False
will also trigger your return).

--
Nicolas Mailhot

And I can confirm the following patch also fixes my workload

diff -uNr matplotlib-1.2.1.orig/lib/matplotlib/font_manager.py
matplotlib-1.2.1/lib/matplotlib/font_manager.py
--- matplotlib-1.2.1.orig/lib/matplotlib/font_manager.py 2013-03-26
14:04:37.000000000 +0100
+++ matplotlib-1.2.1/lib/matplotlib/font_manager.py 2013-07-08
14:49:37.791845661 +0200
@@ -721,6 +721,8 @@
         Return the name of the font that best matches the font
         properties.
         """
+ if not self._family:
+ return rcParams['font.family'][0]
         return ft2font.FT2Font(str(findfont(self))).family_name

     def get_style(self):

Now could something similar be merged?

Regards,

···

Le Mar 9 juillet 2013 15:14, Nicolas Mailhot a écrit :

Le Lun 8 juillet 2013 18:18, Martin Mokrejs a écrit :

Could you instead just test for "if not self._family"? Tests for
equality
are more expensive (that means self._family == 0 or self._family ==
False
will also trigger your return).

I can test if it works. However, all the other tests in that file are
already of the if Foo is None form, and I didn't want to change the coding
style

--
Nicolas Mailhot

Hi Nicolas,

Nicolas Mailhot wrote:

Could you instead just test for "if not self._family"? Tests for
equality
are more expensive (that means self._family == 0 or self._family ==
False
will also trigger your return).

I can test if it works. However, all the other tests in that file are
already of the if Foo is None form, and I didn't want to change the coding
style

I think it is a bad style then. If in some places a '' or False
is needed to discriminate from None then the code should be changed.

And I can confirm the following patch also fixes my workload

diff -uNr matplotlib-1.2.1.orig/lib/matplotlib/font_manager.py
matplotlib-1.2.1/lib/matplotlib/font_manager.py
--- matplotlib-1.2.1.orig/lib/matplotlib/font_manager.py 2013-03-26
14:04:37.000000000 +0100
+++ matplotlib-1.2.1/lib/matplotlib/font_manager.py 2013-07-08
14:49:37.791845661 +0200
@@ -721,6 +721,8 @@
         Return the name of the font that best matches the font
         properties.
         """
+ if not self._family:
+ return rcParams['font.family'][0]
         return ft2font.FT2Font(str(findfont(self))).family_name

     def get_style(self):

Now could something similar be merged?

Thank you for your efforts on this. I am just a user like you so we have to
wait for an answer of the maintainers. Maybe I should have emphasized that earlier.
:wink: But this is good for the community review anyway. :wink:

Martin

···

Le Mar 9 juillet 2013 15:14, Nicolas Mailhot a écrit :

Le Lun 8 juillet 2013 18:18, Martin Mokrejs a écrit :

This patch doesn't make a whole lot of sense to me. get_name should work just fine if self._family is None, and indeed it does in my own testing:

from matplotlib import font_manager

f = font_manager.FontProperties(None)
print f._family
print f.get_family()
print f.get_name()

So I'd much prefer to get to the bottom of the root cause of this problem than patch it unnecessarily in this way. Any idea what that is?

Mike

···

On 07/08/2013 10:57 AM, Nicolas Mailhot wrote:

Hi,

In matplotlib 1.2.1, the get_name function is not garding against none
self (unlike other functions); Unfortunately it seems I have a workload
that makes matplotlib call get_name with None (wasn't the case in 1.2.0).
I couldn't isolate the exact trigger, when I reduce the volume of data
processed the problem goes away so I have to simple shareable reproducer.

Anyway, the following patch makes it all work for me, could it (or
something similar) be merged?

diff -uNr matplotlib-1.2.1.orig/lib/matplotlib/font_manager.py
matplotlib-1.2.1/lib/matplotlib/font_manager.py
--- matplotlib-1.2.1.orig/lib/matplotlib/font_manager.py 2013-03-26
14:04:37.000000000 +0100
+++ matplotlib-1.2.1/lib/matplotlib/font_manager.py 2013-07-08
14:49:37.791845661 +0200
@@ -721,6 +721,8 @@
          Return the name of the font that best matches the font
          properties.
          """
+ if self._family is None:
+ return rcParams['font.family']
          return ft2font.FT2Font(str(findfont(self))).family_name

      def get_style(self):

Regards,

I haven't the faintest idea. The function seems to be called with a "None"
font, and then it crashes. As it's executed under a Linux system it uses
the fontconfig backend with cairo. The call only occurs when matplotlib is
fed more data than can be isolated in an simple example

Attaching the only patches Fedora/Red Hat applied before the build :

Patch0: %{name}-noagg.patch
Patch1: %{name}-tk.patch
# http://sourceforge.net/mailarchive/message.php?msg_id=30202451
# https://github.com/matplotlib/matplotlib/pull/1666
# https://bugzilla.redhat.com/show_bug.cgi?id=896182
Patch2: %{name}-fontconfig.patch

[…]

# Remove bundled libraries
rm -r agg24 lib/matplotlib/pyparsing_py?.py

# Remove references to bundled libraries
%patch0 -p1 -b .noagg
sed -i -e s/matplotlib\.pyparsing_py./pyparsing/g lib/matplotlib/*.py

# Correct tcl/tk detection
%patch1 -p1 -b .tk
sed -i -e 's|@@libdir@@|%{_libdir}|' setupext.py

# Use fontconfig by default
%patch2 -p1 -b .fontconfig

Regards,

python-matplotlib-fontconfig.patch (1.54 KB)

python-matplotlib-noagg.patch (2.62 KB)

python-matplotlib-tk.patch (1.44 KB)

···

Le Mer 17 juillet 2013 15:04, Michael Droettboom a écrit :

This patch doesn't make a whole lot of sense to me. get_name should
work just fine if self._family is None, and indeed it does in my own
testing:

from matplotlib import font_manager

f = font_manager.FontProperties(None)
print f._family
print f.get_family()
print f.get_name()

So I'd much prefer to get to the bottom of the root cause of this
problem than patch it unnecessarily in this way. Any idea what that is?

--
Nicolas Mailhot

BTW if you have some code to add to the crashing function to identify
what's wrong, I can reproduce the crash at will (though the processing
before crash time is rather long)

Regards,

···

Le Jeu 18 juillet 2013 14:12, Nicolas Mailhot a écrit :

Le Mer 17 juillet 2013 15:04, Michael Droettboom a écrit :

This patch doesn't make a whole lot of sense to me. get_name should
work just fine if self._family is None, and indeed it does in my own
testing:

from matplotlib import font_manager

f = font_manager.FontProperties(None)
print f._family
print f.get_family()
print f.get_name()

So I'd much prefer to get to the bottom of the root cause of this
problem than patch it unnecessarily in this way. Any idea what that is?

I haven't the faintest idea. The function seems to be called with a "None"
font, and then it crashes. As it's executed under a Linux system it uses
the fontconfig backend with cairo. The call only occurs when matplotlib is
fed more data than can be isolated in an simple example

Attaching the only patches Fedora/Red Hat applied before the build :

Patch0: %{name}-noagg.patch
Patch1: %{name}-tk.patch
# http://sourceforge.net/mailarchive/message.php?msg_id=30202451
# https://github.com/matplotlib/matplotlib/pull/1666
# https://bugzilla.redhat.com/show_bug.cgi?id=896182
Patch2: %{name}-fontconfig.patch

[…]

# Remove bundled libraries
rm -r agg24 lib/matplotlib/pyparsing_py?.py

# Remove references to bundled libraries
%patch0 -p1 -b .noagg
sed -i -e s/matplotlib\.pyparsing_py./pyparsing/g lib/matplotlib/*.py

# Correct tcl/tk detection
%patch1 -p1 -b .tk
sed -i -e 's|@@libdir@@|%{_libdir}|' setupext.py

# Use fontconfig by default
%patch2 -p1 -b .fontconfig

--
Nicolas Mailhot