1218041 - SVG favicons are not shown in Bookmark/History menu

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[sjw]

STR:
1. Go to a site using SVG favicons (you may use the test cases from bug 366324)
2. Bookmark the site and close tab
3. favicon isn't shown neither in bookmarks list nor in history

Comment 1

[atlanto]

my result of mozregression;
> 6:20.86 LOG: MainThread main INFO Oh noes, no (more) inbound revisions :(
> 6:20.86 LOG: MainThread Bisector INFO Last good revision: 4f0530d695b6898a865e61b0bfaa39761a815f6e
> 6:20.86 LOG: MainThread Bisector INFO First bad revision: d57165a9b3360fb5009184dc90c1cd361782f4bc
> 6:20.86 LOG: MainThread Bisector INFO Pushlog:
>https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4f0530d695b6898a865e61b0bfaa39761a815f6e&tochange=d57165a9b3360fb5009184dc90c1cd361782f4bc

Comment 2

[:Gijs (he/him)]

(In reply to atlanto from comment #1)
> my result of mozregression;
> > 6:20.86 LOG: MainThread main INFO Oh noes, no (more) inbound revisions :(
> > 6:20.86 LOG: MainThread Bisector INFO Last good revision: 4f0530d695b6898a865e61b0bfaa39761a815f6e
> > 6:20.86 LOG: MainThread Bisector INFO First bad revision: d57165a9b3360fb5009184dc90c1cd361782f4bc
> > 6:20.86 LOG: MainThread Bisector INFO Pushlog:
> >https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4f0530d695b6898a865e61b0bfaa39761a815f6e&tochange=d57165a9b3360fb5009184dc90c1cd361782f4bc

dholbert, this points to bug 1205923... any ideas?

Comment 3

[Daniel Holbert [:dholbert]]

That makes some sense.  That bug changed how imgIContainer::GetWidth/GetHeight reports failure, for SVG images that have no explicit width or height.  There's likely some callsite that's involved with SVG favicon creation, which checks for our old failure behavior and needs updating.

I'll take a look. Thanks to all for the bug-filing, regression-range-tracking, & the ping.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to sjw from comment #0)
> STR:
> 1. Go to a site using SVG favicons (you may use the test cases from bug 366324)

I can't actually reproduce this bug with the two live testcases on that bug. But I can using a local testcase that I created by removing the "width" & "height" attributes from the SVG file used in that bug's testcases.

> 3. favicon isn't shown neither in bookmarks list nor in history

Note: the favicon *does* show up for me in the bookmarks & history menus, as well as on the bookmarks toolbar. I haven't been able to reproduce any issues in those areas.

However, I *can* reproduce this issue (I think) in the "Library" window (the Bookmarks/History Organizer, Ctrl+Shift+O).

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 6

[Daniel Holbert [:dholbert]]

I sanity-checked the regression range with nighlties a few days on either side, too. (2015-09-23 & 2015-09-26), and I verified that the favicon *does* show up in the Library window in the earlier nightly, but not in the later nightly.

So, I am seeing this bug.

Comment 7

[Daniel Holbert [:dholbert]]

The issue is in nsTreeBodyFrame::PaintImage, which calls GetWidth/GetHeight on the image and doesn't check whether it succeeded or not, and then passes the result to nsLayoutUtils::GetWholeImageDestination, which now ends up doing 0/0 and producing +infinity, and bogus results ensue.

(Before bug 1205923, we'd use -1 instead of 0 here, which was also bogus, but at least -1/-1 produces reasonable results and gets us drawing something at the right scale.)

I think the right fix is to adjust this nsTreeBodyFrame code to check for GetWidth & GetHeight failure, and skip over its scaling code in that case.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Here's a patch.

This just...
 - Refactors the existing code here to be inside of an "if (image->GetWidth & image->GetHeight() both succeed) {" check
 - Adds some fallback code in an "else" clause, for when that check fails (if we have an SVG image with no explicit width/height).

This chunk of code is for handling image sprites, I think -- and it's trying to compute the full size that the image would be, for the correct sub-region to fit into the destination area.  The fallback code just makes us try to paint the full SVG image into the destination area.

This restores the behavior that we "accidentally" fell into for this case (due to a -1/-1 division) before bug 1205923 regressed this by making GetWidth/GetHeight return 0 in failure cases.  Now we'll be doing that same behavior, a bit more intentionally.

Ideally, it'd be cool to support image sprites here, but given XUL de-prioritization, I'm not going to invest more time into figuring out how to make that work.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> Ideally, it'd be cool to support image sprites here, but given XUL
> de-prioritization, I'm not going to invest more time into figuring out how
> to make that work.

"...to support *SVG* image sprites with no explicit height/width", I mean.

Comment 10

[Daniel Holbert [:dholbert]]

So mechanically, this patch is just:
 - Shifting GetWidth/GetHeight calls inside of a new "if (....)" check.
 - Reindenting the code inside of that check, and rewrapping the newly-indented comments to make them stay within 80-char limit.
 - Reordering code - moving the "GetImageSourceRect" call down to be inside of that "if" check, since that's the only place it's used.
 - Adding an "else" clause.

You may want to use a merge tool like "meld" or "k3b" in review, to assist in highlighting the bits that actually changed.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: WIP reftest patch

Spent some time trying to come up with a reftest, using a XUL tree (the type of rendering object used in the bookmark/history organizer aka "Places Library").

But, I was stymied by the fact that it's apparently impossible to find out when a XUL <treecell> has rendered its image. :-/

The files in this reftest *do* exercise the bug locally -- the "b" variant included here -- which has no size on its SVG image -- renders with no image when I visit it locally, with dom.allow_XUL_XBL_for_file = true in about:config.  But when I load them in the reftest harness, they *all* render with no image, and hence they spuriously pass.

I tried to work around this issue with
  - MozReftestInvalidate + reftest-wait
  - including the image as a data URI (which maybe we'd synchronously load)
...but neither of those helped. We still get reftest snapshots without the image loaded.

The only thing that *does* reliably work is adding a 1-second setTimeout to delay the reftest snapshot. Clearly we don't want to do that though.

I think the real right way to test this might be with a mochitest + WindowSnapshot.js, and just keep cycling & taking snapshots until the files match (and fail if they never do), or something like that.  I'm not really feeling up to writing a test like that at the moment, though, given that XUL is on its way out and given how poorly-tested this stuff is already.  So I'll just drop this reftest WIP here in case I or someone else can improve on it later, and we can rely on manual testing for verification.

Comment 12

[Daniel Holbert [:dholbert]]

[Reclassifying as Core|Layout: Images, since this is really a platform-level XUL image-rendering bug, as shown by the patch.]

Comment 13

[Timothy Nikkel (:tnikkel)]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> I think the real right way to test this might be with a mochitest +
> WindowSnapshot.js, and just keep cycling & taking snapshots until the files
> match (and fail if they never do), or something like that.  I'm not really
> feeling up to writing a test like that at the moment, though, given that XUL
> is on its way out and given how poorly-tested this stuff is already.  So
> I'll just drop this reftest WIP here in case I or someone else can improve
> on it later, and we can rely on manual testing for verification.

If anybody wants to do that there is already a test that does this that could be used as a good starting point:
http://mxr.mozilla.org/mozilla-central/source/image/test/mochitest/test_animSVGImage.html?force=1

Comment 14

[Daniel Holbert [:dholbert]]

Yup, thanks.

In that test, IIRC, we snapshot the reference case, and then keep snapshotting the testcase until it looks like the reference.

Here, we've got a bit of extra complexity -- even the reference case can have an unknown delay before it renders correctly. 

So, to write a test for this bug, I think we'd need to:
 1) create an "anti-reference-case", with a <tree> but no image, and snapshot that.
 2) Load the reference case, and continuously snapshot it (polling with setTimeout) until it looks different from the "anti-reference-case". Assume that this snapshot is our true reference-case-image.
 3) Load the testcase, and continuously snapshot it (polling with setTimeout) until it matches the final snapshot that we got in step 2.

(And if we hit some timeout threshold without completing step 3, declare failure.)

Comment 15

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

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

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

LGTM.

Comment 16

[Daniel Holbert [:dholbert]]

Thanks!

Per IRC discussion w/ seth, RE testing:
 - In a perfect world, the current WIP reftest *should* be effective. (tree cell images should decode synchronously and hence be ready to paint in time for the reftest snapshot)
 - Seth landed a patch recently that he thought should've gotten us to that perfect world, but maybe it didn't after all (needs investigation)
 - Given that we're close to that perfect world, there's no point in writing a crazy mochitest along the lines of comment 13/comment 14; we should just take the attached WIP reftest, with a comment & maybe an annotation saying it's not quite effective yet.

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> But, I was stymied by the fact that it's apparently impossible to find out
> when a XUL <treecell> has rendered its image. :-/

(I filed bug 1218954 on approximately this issue, BTW.)

Here's a Try run for this bug here, with the (slightly-tweaked) reftest patch included:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c41291a65ec

The reftests are marked as "random" right now, w/ reference to bug 1218954.  (I also briefly considered including an additional "do we render an image at all?" sanity-reftest here, but I'm deferring that to the reftest patch on bug 1218954.)

Comment 18

[Daniel Holbert [:dholbert]]

Marking the test "skip" instead of "random", since even loading a XUL page on B2G triggers a test failure like:
  TEST-UNEXPECTED-FAIL | load failed with unknown reason
(despite the "random" annotation)
(This is why all of our XUL tests have B2G-specific skip annotations)

Rather than doing a combo "random skip-if(...)" annotation, I'm just gonna call this "skip" with the XXXdholbert comment, since random is pretty much the same as "skip", enforcement-wise, and I always get compound reftest annotations wrong (e.g. bug 1054376)

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/625650d1493c
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a2aa5e4cde3

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/625650d1493c
https://hg.mozilla.org/mozilla-central/rev/9a2aa5e4cde3

Comment 21

[Mohammad Maruf Islam]

I have reproduced this bug with Firefox Nightly 44.0a1 (Build ID: 20151023030245) on 
windows 8.1 (64-bit) .

Verified as fixed with Firefox Aurora 44.0a2 (Build ID: 20151211004013)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0

Comment 22

[sjw]

Yep, it seems that there is a regression again.
Sadly my mozregression is currently broken, because of an incompatible Python version :|

Comment 23

[:Gijs (he/him)]

(In reply to sjw from comment #22)
> Yep, it seems that there is a regression again.
> Sadly my mozregression is currently broken, because of an incompatible
> Python version :|

Can you file a new bug with more details, please?

Comment 24

[sjw]

I was not able to found a specific regression, but I finally opened bug 1268467.