1205923 - width & height property obtained 4294967295 for non-fixed-width and non-shown SVG image

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[YF (Yang)]

Attachment: Example 1

STR:
Open the chrome://browser/content/1 get the Not Found page, View Page Info, Media tab, select the svg file row, the size shown: 4,294,967,295px × 4,294,967,295px (scaled to 0px × 0px).

Alternative, see the Example 1.


Google Chrome 45 always displayed as 100 x 100 for the Example 1.


PageInfo codebase: https://hg.mozilla.org/mozilla-central/annotate/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/browser/base/content/pageinfo/pageInfo.js#l952

Comment 1

[YF (Yang)]

Attachment: Example 2 - link to fixed-width SVG

30 x 30 works fine.

Comment 2

[YF (Yang)]

Attachment: Example 3 - get property immediately

Firefox 41b9:
0 x 0 for first load and Ctrl+F5.
4294967295 x 4294967295 for cached loaded.

Google Chrome 45:
Always return 0 x 0.

Comment 3

[YF (Yang)]

Attachment: Example 4 - get for displayed SVG

Always return 1333 x 1333 for me, in Firefox and Google Chrome.

Comment 4

[Daniel Holbert [:dholbert]]

So, two things are semi-broken here:
 (1) nsGenericHTMLElement::GetWidthHeightForImage doesn't check for success when it makes this call:
     image->GetWidth(&size.width);

As it happens, the call fails (and has bogus value -1 in the outparam). The caller, GetWidthHeightForImage, proceeds to use the returned value, at at some stage it's cast to be unsigned and ends up being the huge value that's reported here.

 (2) VectorImage::GetWidth / GetHeight are touching their outparams in a failure condition (setting the outparam to -1 in tihs case), which violates an xpcom guideline -- functions with outparams & the ability to fail should only touch their outparams on success conditions.  (I'm not sure where this guideline is documented, but I found it at least on an old blog post from dmandelin*, which satisfied my fear that I might've just made it up.)

Easiest/cleanest solution here is to fix GetWidth & GetHeight in VectorImage to leave their outparams untouched on failure, I think.  Then, the call-site here (GetWidthHeightForImage) doesn't need to bother checking for failure, because it can be confident that the size will be left at 0 its initial value of 0, which is probably the fallback behavior we want here anyway.

* https://blog.mozilla.org/dmandelin/2008/03/10/i-need-a-theorem-prover/

Comment 5

[Daniel Holbert [:dholbert]]

(Looks like RasterImage GetWidth/GetHeight modify their outparams on failure, too, so they're also violating this xpcom api guideline. Though at least RasterImage sets the outparam to 0, which probably causes fewer surprises than -1 does.)

Comment 6

[Daniel Holbert [:dholbert]]

Note: the attached testcase ("example 3") triggers these assertion-failures, too:
###!!! ASSERTION: negative width: 'size.width >= 0', file dom/html/nsGenericHTMLElement.cpp, line 3228
###!!! ASSERTION: negative height: 'size.height >= 0', file dom/html/nsGenericHTMLElement.cpp, line 3229

That's just indicating that the nsGenericHTMLElement method I mentioned above is surprised to receive a negative return value from imgIContainer::GetWidth/GetHeight.

Comment 7

[Daniel Holbert [:dholbert]]

(And the negative return value in question comes from the SVGSVGElement::GetIntrinsicWidth() early "return -1" when the root <svg> element has a percent width:
http://mxr.mozilla.org/mozilla-central/source/dom/svg/SVGSVGElement.cpp?rev=3f2f4df587af&mark=1243-1243,1257-1257#1239

This -1 gets passed up to VectorImage which happily puts it in the outparam, and returns a failure code which the caller ignores as noted above.)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This still needs an automated test (pretty sure we can make a crashtest based on example 3 which exercises the assertion noted above), but here's the fix at least.

Comment 9

[Daniel Holbert [:dholbert]]

(If it's not clear, the point of the patch is just to restructure the logic so that both of the existing failure cases happen as early-returns, before we've touched the outparam.)

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=67648234715d

Comment 10

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8662752 [details] [diff] [review]
fix v1

Review of attachment 8662752 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: fix v2 (w/ crashtest) [r=seth]

Thanks! Here's an updated version, with a crashtest now. Carrying forward r+.

Comment 12

[Daniel Holbert [:dholbert]]

I verified that (in local crashtest runs) the crashtest reliably causes assertion failures like comment 6 if I don't have the fix, and it passes (no assertion failures) if I do have the fix.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/757560ab8de1

Comment 14

[Daniel Holbert [:dholbert]]

Huh, apparently this breaks some XUL object-fit tests, only on a few platforms (Linux x64 opt/ASAN [not debug], and WinXP [not sure if opt-only; debug hasn't finished yet]):
  https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=757560ab8de1

Some code must be accidentally depending on GetWidth/GetHeight setting the outparam even on failure, or something. (Not too surprising.)

Comment 15

[Daniel Holbert [:dholbert]]

Backed out per comment 14: https://hg.mozilla.org/integration/mozilla-inbound/rev/3e4168e0a457

Comment 16

[Daniel Holbert [:dholbert]]

I can't reproduce the failures locally (on Linux64, on an enable-optimize enable-debug build, which isn't quite the platform that failed but is close [opt at least]).

So I don't know exactly what's going on, but I think I know roughly what's happening -- when I first wrote VectorImage.cpp, I recall seeing lots of GetWidth/GetHeight callers that just (incorrectly) assume these accessors are infallible, which was a fine assumption for the traditional codepath with RasterImage. For callers that don't initialize GetWidth/GetHeight's outparam themselves, an "xpcom-correct" strategy (leaving outparams unset on failure) turns all these callsites into uninitialized-memory-reads & causes odd random behavior.  For SVG, this will happen for any SVG image that has no explicitly-specified width & height (or any percent-valued width & height). So, when initially writing VectorImage.cpp, I followed RasterImage's pattern and always initialized the value to *something* to avoid causing these sorts of uninitialized-memory-reads.

Arguably, we should clean up the callers to check for GetWidth/GetHeight failure, & make no assumptions about the outparam in the case of failure.  But for now, I think that might be too clumsy, and it's probably easier to just keep following RasterImage's lead & decide to always set the outparam from these methods, to avoid accidental uninitialized-memory-reads.

So, I'm now thinking the only change we should make here (from the current codebase) is to avoid letting the outparam get set to -1, and instead set it to 0 on failure (just like RasterImage does). That way, callers that blindly attempt to use the outparam (without checking for failure) will at least get marginally-sane behavior (e.g. 0-sized things) instead of bizarre infinity-sized-things from misinterpreting -1.

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: fix v3

This version makes us set the outparam to 0 on failure (instead of -1 & instead of leaving it unassigned), per comment 16.

This matches what RasterImage does - source reference:
http://mxr.mozilla.org/mozilla-central/source/image/RasterImage.cpp?rev=07d6fcc70e7c&mark=210-210#204

Pretty sure this won't hit the reftest failures (which probably are from uninitialized-memory-reads) that the previous version hit.

Comment 18

[Daniel Holbert [:dholbert]]

(I also fixed an incorrect type in the previous patch version -- I had one 'nscoord' that really wanted to be int32_t.)

Interdiff view, comparing new patch vs. the patch that you reviewed:
https://bugzilla.mozilla.org/attachment.cgi?oldid=8662752&action=interdiff&newid=8663760&headers=1

Comment 19

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8663760 [details] [diff] [review]
fix v3

Review of attachment 8663760 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks good, but I'm a little confused by the crashtest.

::: image/test/crashtests/1205923-1.html
@@ +7,5 @@
> +    var newImage = new Image;
> +    newImage.id = "thepreviewimage";
> +    newImage.setAttribute("src", "unsized-svg.svg");
> +    physWidth = newImage.width || 0;
> +    physHeight = newImage.height || 0;

Did we crash from running these getters, or are these variables just unused?

@@ +8,5 @@
> +    newImage.id = "thepreviewimage";
> +    newImage.setAttribute("src", "unsized-svg.svg");
> +    physWidth = newImage.width || 0;
> +    physHeight = newImage.height || 0;
> +    document.documentElement.innerHTML += newImage.width + " x " + newImage.height + "<br>";

This is just debugging output, right?

Also, technically speaking, you should wait for the |load| event to check |width| and |height|. Though obviously we shouldn't crash regardless...

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #19)
> The patch looks good, but I'm a little confused by the crashtest.
> 
> > +    physWidth = newImage.width || 0;
> > +    physHeight = newImage.height || 0;
> 
> Did we crash from running these getters, or are these variables just unused?

In current trunk, we assert (non-fatally, which is treated as a crashtest failure) inside of these getters, when we hit them the second time.

(This code is partly cribbed from the attached "Example 3" testcase, BTW.)

> @@ +8,5 @@
> > +    newImage.id = "thepreviewimage";
> > +    newImage.setAttribute("src", "unsized-svg.svg");
> > +    physWidth = newImage.width || 0;
> > +    physHeight = newImage.height || 0;
> > +    document.documentElement.innerHTML += newImage.width + " x " + newImage.height + "<br>";
> 
> This is just debugging output, right?

Yup, not strictly necessary for triggering the assertion-failure.  (Useful to make sure we don't somehow optimize away the width/height lookup, too, via some hypothetical future optimization.)
 
> Also, technically speaking, you should wait for the |load| event to check
> |width| and |height|. Though obviously we shouldn't crash regardless...

True; I can make that tweak if you like, though the current version reliably asserts.

Comment 21

[Daniel Holbert [:dholbert]]

(The assertion triggered by the crashtest is in comment 6, BTW.)

Comment 22

[Daniel Holbert [:dholbert]]

Attachment: fix v4 (crashtest clarified)

Here's the same patch, with an improved crashtest. (Hopefully a bit easier to follow, and with the second image-load chained off of the first image-load).

I verified that the width/height queries after the second image-load still cause assertions in current trunk (making the crashtest fail), and those assertions go away with the fix applied.

Comment 23

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8665210 [details] [diff] [review]
fix v4 (crashtest clarified)

Review of attachment 8665210 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the test tweaks; it's easier to follow now!

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d57165a9b336

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d57165a9b336