353 bytes, text/html
347 bytes, text/html
375 bytes, text/html
351 bytes, text/html
5.26 KB, patch
|Details | Diff | Splinter Review|
Created attachment 8662730 [details] 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
Created attachment 8662732 [details] 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.
Created attachment 8662733 [details] Example 4 - get for displayed SVG Always return 1333 x 1333 for me, in Firefox and Google Chrome.
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/
(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.)
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.
(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.)
Created attachment 8662752 [details] [diff] [review] 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.
Attachment #8662752 - Flags: review?(seth)
(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 on attachment 8662752 [details] [diff] [review] fix v1 Review of attachment 8662752 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me!
Attachment #8662752 - Flags: review?(seth) → review+
Created attachment 8663172 [details] [diff] [review] fix v2 (w/ crashtest) [r=seth] Thanks! Here's an updated version, with a crashtest now. Carrying forward r+.
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.
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.)
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.
Created attachment 8663760 [details] [diff] [review] 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.
(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 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...
(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.
(The assertion triggered by the crashtest is in comment 6, BTW.)
Created attachment 8665210 [details] [diff] [review] 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 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!
Attachment #8665210 - Flags: review?(seth) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.