715075 - Page Info reads image width and height synchronously after setting src, leads to 0px x 0px if the image isn't in your cache (e.g. after clearing cache)

Status: RESOLVED WORKSFORME

(Firefox :: Page Info Window, defect)

Description

[dindog]

View image Info of a Base64 image won't its naturalWidth and naturalHeight, but 0px * 0px

Comment 1

[Boris Zbarsky [:bzbarsky]]

Fixed in Firefox 11.  I have to assume you're using an older build than that, since you didn't specify...

Do please reopen if you see it in current nightly or Aurora builds.

Comment 2

[dindog]

(In reply to Boris Zbarsky (:bz) from comment #1)
> Fixed in Firefox 11.  I have to assume you're using an older build than
> that, since you didn't specify...
> 
> Do please reopen if you see it in current nightly or Aurora builds.
> 
> *** This bug has been marked as a duplicate of bug 632119 ***

No, I am using nightly, and I knew that bug too, it fixed the display, but not the info.
Here is what I got now:
0px × 0px (scaled to 483px × 271px)

Comment 3

[Daniel Holbert [:dholbert]]

I can't reproduce, with Linux Nightly.

I tried both with the original (non-base64) data URI in bug 632119 comment 0, as well as the base64 variant in bug 632119 comment 24.  In both cases, I see this in View Image Info:
>  Location:   data:image/png[etc]
>  Type:       PNG Image
>  Size:       Unknown (not cached)
>  Dimensions: 100px x 100px
>  Media Preview: [the actual image]
(whereas in earlier versions of Firefox, "Dimensions" shows 0px x 0px and Media Preview has a broken-image icon)

dindog: can you reproduce this bug in an up-to-date Nightly using the base64 data URI that I provided in bug 632119 comment 24?

If so, can you post your User Agent string from Help | Troubleshooting Information?  (For future reference, this is generally a useful thing to include whenever filing a bug, just so we're clear on which builds are reported to be affected.)

Comment 4

[Daniel Holbert [:dholbert]]

Aha! I was able to reproduce the bug at the URL you provided.
 ( http://www.cnblogs.com/dindog/articles/2312074.html )

I loaded that URL it in a fresh profile, did "View Image Info" on the cat photo, and I got:
> Dimensions: 0px × 0px (scaled to 434px × 244px)

If I repeat the STR a few times, it ends up showing the correct size (1,600px × 900px) instead of 0px x 0px.
Mozilla/5.0 (X11; Linux i686; rv:12.0a1) Gecko/20120103 Firefox/12.0a1

Comment 5

[Daniel Holbert [:dholbert]]

OK -- turns out data URI isn't needed.

Here are simple STR for this bug:
 1. View any image (e.g. attachment 510334 [details])
 2. Clear your cache (Tools | Clear Recent History, make sure "Cache" is ticked)
 3. Right-click the image, choose View Image Info.

EXPECTED RESULTS: should report "Dimensions: 100px x 100px"
ACTUAL RESULTS:   it reports:   "Dimensions: 0px x 0px"

The key requirement seems to be that the image *isn't* in some cache when you choose View Image Info, even though it's being displayed onscreen. (I'm guessing this happens for dindog's original URL because it's a huge image and takes some time to be loaded into cache.)

Comment 6

[dindog]

Thanks, Daniel, to put the issue more accurately

Comment 7

[Boris Zbarsky [:bzbarsky]]

Is this really an imagelib bug?  Is the page info code assuming that sizes are available sync after starting an image load, perhaps?

Comment 8

[Joe Drew (not getting mail)]

Yep - in pageInfo.js, there's a new Image that gets src set to some url, and then reads the width and height synchronously afterwards.

Comment 9

[Dario Bertero]

I'd like to work on this bug. Please assign it to me. Do you have any suggestion?

Comment 10

[Philip Chee]

Hi!

Have you set up a build environment?
https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

Have you written patches for Mozilla before?
https://developer.mozilla.org/en-US/docs/Creating_a_patch
https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch

> Do you have any suggestion?
You might want to make the code that reads the width and height asynchronous. Our async load tests might give you some ideas.
http://mxr.mozilla.org/mozilla-central/source/image/test/unit/async_load_tests.js#49

Comment 11

[Dario Bertero]

With the change I am now posting the issue seems solved. I don't know if I operated the right way.

PS: It's my first time, I came here after one of your public events where you suggested to take a bug like this one to start.

Comment 12

[Dario Bertero]

Attachment: First attempt of a patch

See comment #11

Comment 13

[Joe Drew (not getting mail)]

Comment on attachment 673710 [details] [diff] [review]
First attempt of a patch

Dario, once you've posted a patch, you need to ask for review or feedback of an appropriate person. In this case, looks like Dão is an appropriate person, so I've asked it of him!

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 673710 [details] [diff] [review]
First attempt of a patch

It looks like this might create a race condition when selecting a second image before the first image loaded. Or is the first image's load event not going to fire in that case?

Comment 15

[Marco Bonardo [:mak]]

(In reply to Dão Gottwald [:dao] from comment #14)
> It looks like this might create a race condition when selecting a second
> image before the first image loaded.

yeah, if the second content is not using onload (svg or video) it may update synchronously and then we'd get the onload from the first image. I think it's possible to happen until the image object is garbage collected and we can't predict that. maybe the callback could just check if the content being updated is still the currently previewed one, bail out otherwise.

Comment 16

[Dario Bertero]

Attachment: Second attempt of a patch

(In reply to Marco Bonardo [:mak] from comment #15)
> (In reply to Dão Gottwald [:dao] from comment #14)
> > It looks like this might create a race condition when selecting a second
> > image before the first image loaded.
> 
> yeah, if the second content is not using onload (svg or video) it may update
> synchronously and then we'd get the onload from the first image. I think
> it's possible to happen until the image object is garbage collected and we
> can't predict that. maybe the callback could just check if the content being
> updated is still the currently previewed one, bail out otherwise.

You mean this way?

Comment 17

[Marco Bonardo [:mak]]

something like that should work, yes. I was thinking of a way to test it works locally (while was not before), maybe with a test page containing 1 video, 1 image, 1 video, and moving down fast by keeping the down key pressed, should show the image size in the second video.
I'll start looking at the coding style part later, there's some of it to be fixed.

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 674008 [details] [diff] [review]
Second attempt of a patch

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

The approach looks fine, coding style needs some fixes

::: browser/base/content/pageinfo/pageInfo.js
@@ +978,5 @@
>      var oldImage = document.getElementById("thepreviewimage");
>  
>      var isProtocolAllowed = checkProtocol(gImageView.data[row]);
>  
> +    var updateImageDimensions = function(physWidth, physHeight, width, height) {

This updates not just images, so maybe updateMediaDimensions would be better.
please add a space between function and the parenthesis, we usually use:
"function name()" or "function ()" for anonymous ones

@@ +1001,2 @@
>      var newImage = new Image;
> +    

There are various trailing whitespaces in the patch, you should remove those
also this added newline after newImage doesn't look needed. Usually try to  reduce changes to the minimum needed, unless there's a good readability reason for the change.

@@ +1003,5 @@
>      newImage.id = "thepreviewimage";
>      var physWidth = 0, physHeight = 0;
>      var width = 0, height = 0;
>  
> +    var readImageDimensions = function() {

ditto on space after function

@@ +1004,5 @@
>      var physWidth = 0, physHeight = 0;
>      var width = 0, height = 0;
>  
> +    var readImageDimensions = function() {
> +               

trailing spaces and this newline is not needed

@@ +1006,5 @@
>  
> +    var readImageDimensions = function() {
> +               
> +      //Checks if another image had been loaded asyncronously
> +      //before this callback

Always add a whitespace after "//" (optionally end comments with proper punctuation)

The comment is actually a bit wrong, cause what we care about is if "another media has been loaded in the meanwhile" (both synchronously or asynchronously may end up firing before this afaict).

@@ +1007,5 @@
> +    var readImageDimensions = function() {
> +               
> +      //Checks if another image had been loaded asyncronously
> +      //before this callback
> +      if(newImage.getAttribute("src") != url)

add space between if and the parenthesis

@@ +1014,3 @@
>        physWidth = newImage.width || 0;
>        physHeight = newImage.height || 0;
> +      

trailing spaces

@@ +1026,5 @@
>          // (for example, "table" can have "width" or "height" attributes)
>          newImage.width = newImage.naturalWidth;
>          newImage.height = newImage.naturalHeight;
>        }
> +       

trailing spaces

@@ +1040,4 @@
>        document.getElementById("brokenimagecontainer").collapsed = true;
> +
> +      
> +      updateImageDimensions(physWidth, physHeight, width, height);

trailing spaces and too many newlines

@@ +1048,5 @@
> +         item instanceof SVGImageElement ||
> +         (item instanceof HTMLObjectElement && mimeType.startsWith("image/")) || isBG) && isProtocolAllowed) {
> +      newImage.setAttribute("src", url);
> +
> +      newImage.onload = readImageDimensions; //Sets image dimensions asyncronously

move the comment on the line above and expand it a bit, something like:
// Wait for the image to be loaded before checking its
// dimensions, cause it may not be cached yet.

Comment 19

[Dario Bertero]

Attachment: Third attempt of a patch

I tried to solve the issues reported. Please check if it is ok now.

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 675010 [details] [diff] [review]
Third attempt of a patch

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

yes, now it's better, though I think we can still do a small cleanup

::: browser/base/content/pageinfo/pageInfo.js
@@ +1056,5 @@
>        newImage.id = "thepreviewimage";
>        newImage.mozLoadFrom(item);
>        newImage.controls = true;
>        width = physWidth = item.videoWidth;
>        height = physHeight = item.videoHeight;

this looks a bit pointless now, I'd say to throw away the global width, height, physWidth and pysHeight, in this case just pass item.videoWidth and item.videoHeight to updateMediaDimensions.

instead, define those vars inside readImageDimensions, that is the only one that actually cares about those.

@@ +1078,5 @@
>      else {
>        // fallback image for protocols not allowed (e.g., javascript:)
>        // or elements not [yet] handled (e.g., object, embed).
>        document.getElementById("brokenimagecontainer").collapsed = false;
>        document.getElementById("theimagecontainer").collapsed = true;

previously in this case we were still updating the text, now looks like it will retain the value of the previously selected valid item, that is wrong
The same is valid for the audio case above

I'd say to just do
setItemValue("imagedimensiontext", ""); in these 2 cases, and remove the isAudio check from updateMediaDimensions that becomes useless since we don't invoke the update function there.

Comment 21

[Dario Bertero]

Attachment: 4th attempt

Comment 22

[Marco Bonardo [:mak]]

Comment on attachment 675975 [details] [diff] [review]
4th attempt

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

Tested this against various pages with datauris, audio and video

::: browser/base/content/pageinfo/pageInfo.js
@@ +1047,5 @@
> +      newImage.setAttribute("src", url);
> +
> +      // Waits for the image to be loaded before checking its
> +      // dimensions, because it may not have been cached yet.
> +      newImage.onload = readImageDimensions;

While testing this I figured there is still some cases here we don't handle properly.
- if the load fails .onerror is unhandled
- HTMLLinkElement, HTMLInputElement etc likely don't have an url, so onload/onerror will never fire.
- if the first image in the list is large we show: "Dimensions: " and then just the value appears or the whole row disappears, depending on whether it was able to load the preview.

At this point, to handle all of these cases, would be better to initially hide the row and make it visible when we set a valid value to it. So, my suggestion is to call
setItemValue("imagedimensiontext", "");
just before setting the onload handler, thus the row is initially hidden and appears only when the content is loaded, or stays hidden if there's any problem with the preview image. This should fix all of the 3 cases above.

This also means the if (!url) check in updateMediaDimensions is now useless, since the only 2 cases we handle there are in the onload handler and HTMLVideoElement, both always have an url. So you can remove the condition.

Sorry for this further iteration, though I think with this we should be fine.

Comment 23

[Dario Bertero]

Attachment: 5th attempt

Comment 24

[Marco Bonardo [:mak]]

Comment on attachment 677471 [details] [diff] [review]
5th attempt

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

Functionally it looks safe now, there is still some code polish to do, minor things though.
Thanks for keeping up the good work here.

::: browser/base/content/pageinfo/pageInfo.js
@@ +994,5 @@
> +                                               [formatNumber(width),
> +                                                formatNumber(height)]);
> +      }
> +
> +      setItemValue("imagedimensiontext", imageSize);

nit: you may remove the empty newlines after the var declaration and before this set, they don't seem to add any readability value

@@ +1004,5 @@
> +    function readImageDimensions() {
> +      var physWidth = 0, physHeight = 0;
> +      var width = 0, height = 0;
> +
> +      // Checks if another media has been loaded in the meanwhile.

nit: "Check" instead of "Checks" (we don't usually use third person comments)

@@ +1012,2 @@
>        physWidth = newImage.width || 0;
>        physHeight = newImage.height || 0;

you may declare var physWidth and var physHeight directly here, saving one line of code.

@@ +1031,5 @@
>          newImage.height = item.height.baseVal.value;
>        }
>  
>        width = newImage.width;
>        height = newImage.height;

and declare var width and var height directly here, saving another line of code

@@ +1044,5 @@
> +         item instanceof HTMLImageElement ||
> +         item instanceof SVGImageElement ||
> +         (item instanceof HTMLObjectElement && mimeType.startsWith("image/")) || isBG) && isProtocolAllowed) {
> +      newImage.setAttribute("src", url);
> +      setItemValue("imagedimensiontext", "");

add a comment above the empty string setter saying
"// Hide the field by default, so it's shown only when available."
since I'd like to clarify why we do this here.

@@ +1046,5 @@
> +         (item instanceof HTMLObjectElement && mimeType.startsWith("image/")) || isBG) && isProtocolAllowed) {
> +      newImage.setAttribute("src", url);
> +      setItemValue("imagedimensiontext", "");
> +
> +      // Waits for the image to be loaded before checking its

nit: Replace "Waits" with "Wait" (same reason as above)

@@ +1054,1 @@
>      }

useless newline before the brace

@@ +1074,5 @@
>        document.getElementById("theimagecontainer").collapsed = false;
>        document.getElementById("brokenimagecontainer").collapsed = true;
> +
> +      setItemValue("imagedimensiontext", "");
> +      }

This brace has been wrongly indented, please move it back

Comment 25

[Dario Bertero]

Attachment: 6th attempt

Comment 26

[Marco Bonardo [:mak]]

Comment on attachment 680340 [details] [diff] [review]
6th attempt

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

YES, you did it!

Now let's go on with the landing process, you don't have landing rights, so you need someone to do that for you (after some patches you can start the process to get that access).
This is commonly achieved just by adding checkin-needed keyword to the bug, then a tree sheriff will land it for you.
To help this process, you can follow the suggestions pointed out here: https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in
You should thus attach a patch that specifies both the author (you) and the commit message (bug # - description. r=reviewer) and finally set the checkin-needed keyword.
You don't need a further review on that patch, this one stays valid.

Thank you very much for you contribution, hope to see you soon on some other bug.

Comment 27

[Dario Bertero]

Attachment: Formatted patch for check-in.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Phil Ringnalda (:philor)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/22f24157e6ea - the browser-chrome test browser_bug517902.js has some assumptions about image height and width, assumptions you made no longer true.

Comment 30

[Tanner M. Young [:tmyoung]]

Looking at the errors in the build and hunting through my old patches, the patch for this bug breaks the patches for Bug 526721 and/or Bug 539068.  I'm not sure how exactly it is breaking those, but I am happy to explain anything (that I can remember) about those patches/tests in case it will help.

Comment 31

[Marco Bonardo [:mak]]

could just be the test is making assumption the values are set synchronously.

Comment 32

[Tanner M. Young [:tmyoung]]

(In reply to Marco Bonardo [:mak] from comment #31)
> could just be the test is making assumption the values are set synchronously.

I agree, that is very, very likely, but we should at least check the other possibility, as the test was returning results that are correct for other elements on the test page rather than returning no results at all.  The elements it may be detecting were included to make sure that page info gets the right image info from the right element as Page Info collapses duplicate images so the list is easy to read, but it doesn't always collapse it in a way that works nicely with View Image Info, hence Bug 526721 and Bug 539068.  What's odd is that this code doesn't change anything in the code that fixed those bugs.  So I would say that checking if the test needs some sort of better hook for this patches new "onimagedimensionscollected" (or whatever we should think of it as) is definitely the first place to start, and otherwise, those two other bugs are the next place to look.

Regardless of which problem is the cause, I would suggest that we refactor the test slightly to make it easier to definitively test which element page info thinks it's getting its information from, so that this ambiguity doesn't impact future patches and makes diagnosis of issues with View Image Info faster and easier for everyone.  (So in the test fix, I would ensure that each image element on the test has a different set of dimensions and perhaps a check that id of the element found was indeed the id and element we were looking for as View Image Info should pull out the exact element we want even if it is from a list that Page Info has collapsed in its table for readability, and indicating through comments in the js test source exactly why each row of image elements is there as this tests multiple bugs (both those listed and those we found while still writing Bug 517902) with the same function call, in way that isn't explicitly clear and someone new to the code must go through the entire history of the test and research a lot of the whys and wherefores of it--which through comments in the test could be made a whole lot simpler.)

Comment 34

[Daniel Holbert [:dholbert]]

The test doesn't assume we set the values synchronously -- it assumes they're set by the time "onImagePreviewShown" fires.

This bug's patch might (?) want to delay that firing, until the image has actually finished loading.

(Right now, the patch does newImage.onload = readImageDimensions. It might want to instead do e.g. "newImage.onload = readImageDimensionsAndFireImagePreviewShown" (horrible name, just for the purpose of illustration))

Comment 35

[Tanner M. Young [:tmyoung]]

Dario: Are you still working on this patch?  If you are, could you please check if this patch fixes 0px x 0px images for redirected content like Bug 880415 deals with, in case your patch fixes both.

Comment 36

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:florian, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 37

[Daniel Holbert [:dholbert]]

I retried my STR from comment 5, and discovered that "View Image Info" is no longer a thing that's available in an image's context menu.

I tried some modified STR, though, using the Page Info dialog (and otherwise based on comment #5):

1. Start with a fresh Firefox profile.
2. View attachment 510334 [details]
3. Clear your cache (Ctrl+shift+del, I chose "Time Range: Everything" just to be extra-sure)
4. View the page-info dialog (Ctrl+I or Tools | Page Info)
5. Click the "Media" tab
6. Click the 3rd entry in the list at the top (for the image)

The old "actual results" implied that we would've shown "0x0" here.

But now instead, I briefly see 128x128 (which I think is just left-behind from chrome://global/skin/media/imagedoc-darknoise.png, the initially-focused thing, and it stays there until we decode the newly-focused image); and then in less than a second, the image is decoded and painted, and the size updates to show the correct size (100x100).

So I think this is WORKSFORME. Let's close it as-such, unless anyone can still find a way to reproduce.