Crash on load if <symbol> includes <text> and <svg> lacks 'viewBox' (pango >= 1.37.1)

[su_v]

Inkscape trunk compiled and linked against pango 1.38 (this enables support for opentype features) crashes when loading a drawing with a <symbol> definition that includes <text>, but only if SVG root <svg> does not have a viewBox attribute.

Steps to reproduce:
Open attached test case 'foo19-crash.svg' in current trunk (with opentype features enabled).

Reproduced with Inkscape 0.91+devel r14390, pango 1.38.0 on OS X 10.7.5.

Difference between file which loads ok and file which crashes on load:
$ diff -u foo19-crash.svg foo19-ok.svg
--- foo19-crash.svg 2015-10-01 12:31:57.000000000 +0200
+++ foo19-ok.svg 2015-10-01 12:32:13.000000000 +0200
@@ -8,6 +8,7 @@
    xmlns:xlink="http://www.w3.org/1999/xlink"
    id="svg301"
    version="1.1"
+ viewBox="0 0 793.700787402 1122.51968504"
    height="1122.51968504"
    width="793.700787402">
   <metadata

[su_v (suv-lp)]

Full backtrace with r14390 (debug build).

[su_v (suv-lp)]

The crash was initially encountered when opening a DXF file - the input extension returns an SVG file without viewBox definition AFAICT.

The test case is a reduced version of the DXF import of this file:
https://bugs.launchpad.net/inkscape/+bug/1409486/+attachment/4295879/+files/video_mux4_cropped.dxf

[su_v (suv-lp)]

I happened to have a separate trunk build around which has r13938 reverted (also compiled and linked against pango 1.38) and noticed that this custom build does not crash on load of 'foo19-crash.svg'.

The reverted revision:
https://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/13938

Note that the follow-up commit 13955 (which addressed concerns discussed after r13938 in bug #1239682) modifying src/namedview.cpp was left unchanged.

[Tavmjong Bah (tavmjong-free)]

The crash is a result of font_feature_settings.value having a NULL value in the comparison in SPStyle::getFontFeatureString().

This is called in Layout-TNG-Compute.cpp by Layout::Calculator::_buildPangoItemizationForPara(ParagraphInfo *para) but only if PANGO_VERSION_CHECK(1,37,1) is true.

Why this would have anything to do with the SVG 'viewBox' is missing is really strange.

Looking at the traceback SPText::rebuildLayout() is called in SPText::write(). There should be no reason to call this function here. A write should not require rebuilding the text layout. Commenting out this line gets rid of the crash. A comment says that this was put in as a fix for LP Bug 1339305 but there is no explanation of why this would fix that problem... seems like a classic case of "Cargo Cult Programming".

Going back further, the whole "write" chain is started in sp_file_open() when a document without a 'viewBox' attribute is given one (this is probably not necessary... we probably should not be adding 'viewBox' where one doesn't already exist... especially basing it off of the display unit). So this explains why a document without a 'viewBox' is crashing on load.

Now, why would text in a symbol cause trouble? It is different from normal text in that symbols are not rendered directly. Styling may not be fully resolved at this point (just a guess). Needs further investigation.

[Tavmjong Bah (tavmjong-free)]

I went ahead and protected the string compare in SPStyle::getFontFeatureString() against a NULL value for font_feature_settings.value. This should probably be done anyway but the above questions about why this function is being called remain. "Fixed" in r14395.

[su_v (suv-lp)]

On 2015-10-04 10:06 (+0200), Tavmjong Bah wrote:
> Going back further, the whole "write" chain is started in sp_file_open()
> when a document without a 'viewBox' attribute is given one (this is
> probably not necessary... we probably should not be adding 'viewBox'
> where one doesn't already exist... especially basing it off of the
> display unit).

See also the lengthy discussion in bug #1239682. That report is not closed yet (I moved the milestone to 0.92 for now, comment #30).

> Now, why would text in a symbol cause trouble? It is different from
> normal text in that symbols are not rendered directly. Styling may not
> be fully resolved at this point (just a guess). Needs further
> investigation.

Should we track remaining issues with styling in a separate, more specific report and close this one as fixed; or keep this one open for now (it might provide some context to whatever needs to be further investigated)?

On 2015-10-04 10:30 (+0200), Tavmjong Bah wrote:
> I went ahead and protected the string compare in
> SPStyle::getFontFeatureString() against a NULL value for
> font_feature_settings.value. This should probably be done anyway but the
> above questions about why this function is being called remain. "Fixed"
> in r14395.

"Fix" confirmed with r14395 on OS X 10.7.5 - both with the reduced test case, as well as with the original DXF import. Thanks a lot!