Closed Bug 1326910 Opened 3 years ago Closed 3 years ago

"Page Info" displays wrong image dimensions the 1st time (0px ? 0px)

Categories

(Firefox :: Page Info Window, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: arni2033, Assigned: junderx)

Details

Attachments

(2 files, 7 obsolete files)

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
STR_1:
1. Open bug 1257815
2. Right-click on the 1st user avatar on the page, click "View Image Info"
3. Close "Page Info" window, repeat Step 2

AR:
 Step 2 - "Page Info" window says image dimensions are  "0px ? 0px (scaled to 32px ? 32px)"
 Step 3 - "Page Info" window says image dimensions are  "64px ? 64px (scaled to 32px ? 32px)"
ER:  Either X or Y
 X) Step 2 - "Page Info" should display the correct data, i.e.  "64px ? 64px (scaled to 32px ? 32px)"
 Y) Firefox UI should underline that it's not reliable, and ask user to switch to repeat Step 2
No longer blocks: 1277113
Component: Untriaged → Page Info Window
The root cause of the bug was that the width and height of the newImage instance were being used before the instance was finished loading from the url. The proposed fix sets up a loadend event which calls a new function after the event that accesses the width and height of the loaded image.
Attachment #8832379 - Flags: review?(florian)
Comment on attachment 8832379 [details] [diff] [review]
1326910.patch - Proposed fix for Bug 1326910

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

Thanks for looking into this. The approach in the patch seems good to me, I agree we need to wait for the load to finish before getting the size.

::: browser/base/content/pageinfo/pageInfo.js
@@ +843,5 @@
>      newImage.id = "thepreviewimage";
>      var physWidth = 0, physHeight = 0;
>      var width = 0, height = 0;
>  
> +    // The item instance is already loaded before we get here

I don't understand the meaning of this comment, could you please explain or rephrase?

@@ +850,5 @@
>           (item.HTMLObjectElement && mimeType && mimeType.startsWith("image/")) ||
>           isBG) && isProtocolAllowed) {
>        newImage.setAttribute("src", url);
> +      // We need to wait for the image to finish loading before using width & height
> +      newImage.addEventListener("loadend", imgSize, false);

- It would be safer to set the listener before setting the "src" attribute.
- is a removeEventListener call missing? Or maybe you want to use the {once: true} option?
- nit: please omit the optional third parameter of addEventListener when it's 'false'.

@@ +889,5 @@
> +        document.getElementById("theimagecontainer").collapsed = false
> +        document.getElementById("brokenimagecontainer").collapsed = true;
> +
> +        let imageSize = "";
> +        if (url && !isAudio) {

I know you just moved this code, but we don't need this !isAudio check here, right?

@@ +907,5 @@
> +      }
> +    }
> +
> +    // Handle the case where newImage is not used for width & height
> +    if (!((item.HTMLLinkElement || item.HTMLInputElement ||

Isn't this just "else if (..." like before? I don't see the reason why you need to change this.
Attachment #8832379 - Flags: review?(florian) → feedback+
Patch with all the nice change recommendations implemented
Attachment #8832379 - Attachment is obsolete: true
Attachment #8832784 - Flags: review?(florian)
Comment on attachment 8832784 [details] [diff] [review]
Patch that includes recommended changes

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

Thanks for updating the patch! I tried the patch and it worked fine for me. I will likely r+ the next version.

::: browser/base/content/pageinfo/pageInfo.js
@@ +848,5 @@
>           item.HTMLImageElement || item.SVGImageElement ||
>           (item.HTMLObjectElement && mimeType && mimeType.startsWith("image/")) ||
>           isBG) && isProtocolAllowed) {
> +      // We need to wait for the image to finish loading before using width & height
> +      newImage.addEventListener("loadend", imgSize, {once: true});

I'm not really comfortable with this using a function that's only declared a few lines later. Given that this function is used only once, I would inline it:

newImage.addEventListener("loadend", function() {
  ...
}, {once: true});
newImage.setAttribute("src", ...

I should have mentioned this during the previous review, sorry :-/.
Attachment #8832784 - Flags: review?(florian) → feedback+
Changed the imgSize() function to inline since it is only called one place.
Attachment #8832784 - Attachment is obsolete: true
Attachment #8833193 - Flags: review?(florian)
Comment on attachment 8833193 [details] [diff] [review]
Use inline function this time.

Looks good to me, thanks!

Your patch doesn't include a header with the author and commit message, which name would you like us to use to credit you when checking in the patch?
Flags: needinfo?(junderx)
Attachment #8833193 - Flags: review?(florian) → review+
Hi Florian, Please use my full name, John Underwood.  You could also mention junderx.

Thanks,
John U
https://hg.mozilla.org/integration/mozilla-inbound/rev/8475919c50b5e7874af3856ae8a8cc4778939102
Bug 1326910 - wait for a loadend event before using the width and height of the Page Info media preview, r=florian.
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40ea52619427
Backed out changeset 8475919c50b5 for test failures in browser_bug517902.js on a CLOSED TREE
Usually I would have pushed to the try server a patch changing the browser UI, to verify that it passes all the tests before checking it in, but I thought this code wasn't covered by tests. Turns out I was wrong and there's http://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_bug517902.js

The next step here will be to figure out if the test failure is caused by a bug in this test, or is showing a real problem with the patch.
I'll take a look to sort it out.
The http://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_bug517902.js test is checking the size of the about:logo image.  It says that the width is expected to be 150 and the height to be 100. I reverted the pageInfo.js source file and ran mach build again.

Then I loaded the about:logo page after starting the build result and selected View Image Info while stopping at break points in the debugger to check width and height variables.  No variations of the width and height variables ever had anything except 0 or 300 for width and 0 or 236 for height.

So my conclusion is that the browser_bug517902.js test should fail even with this bug fix reverted.
(In reply to junderx from comment #13)
> The
> http://searchfox.org/mozilla-central/source/browser/base/content/test/
> general/browser_bug517902.js test is checking the size of the about:logo
> image.

It's checking the width and height of the preview image, which previews <img src='about:logo?b' height=100 width=150 alt=2 id='test-image'>. So expecting 100 and 150 is reasonable.

What's happening here is that when your patch is applied, the width and height attributes are set on the preview image only after it has finished loading.

The test needs to be changed to wait for the loadend event on the preview image before checking the width and height (and I think we can remove the 'executeSoon' call currently in the test).
Changing the test to use a callback for testImg loaded event did not work.  The callback was never called.  This may have been due to the fact that the pageinfo.js code was already using that event and callback.  This patch just waits 1 second for the pageinfo.js code to finish before testing the result.  I tried 300ms and that also works, but 30ms did not work.
Attachment #8839261 - Flags: review?(florian)
Comment on attachment 8839261 [details] [diff] [review]
Patch for the test that was failing

(In reply to junderx from comment #15)

> Changing the test to use a callback for testImg loaded event did not work. 
> The callback was never called.

We'll need to figure this out. Can you show me the code you tried and that didn't work?

> This patch just
> waits 1 second for the pageinfo.js code to finish before testing the result.
> I tried 300ms and that also works, but 30ms did not work.

We can't just use a timer here, or it'll fail intermittently on all machines that are slower than yours (or under heavy load).
Attachment #8839261 - Flags: review?(florian) → review-
Attachment #8843637 - Flags: review?(florian)
I have attached 4 versions of the test that never reach the callback.

Since this is test code and not the browser itself, I'm not sure we will ever be running on a slow or heavily loaded machine, but I suppose that is still a possibility.
Comment on attachment 8843637 [details]
First version of the test that fails to reach the callback

Just replace the executeSoon call by:
pageInfoImg.addEventListener("loadend", function() {

(and that means you need to move the "var pageInfoImg = ..." line up a little bit of course)
Attachment #8843637 - Flags: review?(florian) → review-
(In reply to junderx from comment #21)

> Since this is test code and not the browser itself, I'm not sure we will
> ever be running on a slow or heavily loaded machine, but I suppose that is
> still a possibility.

You would be surprised; it's common, eg. when tests run on virtual machines.
Assignee: nobody → junderx
Looks like this completes this bug fix.  The test passes now. If there is anything else that needs to be done for this, anyone please feel free to do it.  I'm giving up my cloud server tonight on AWS.
Attachment #8839261 - Attachment is obsolete: true
Attachment #8843637 - Attachment is obsolete: true
Attachment #8843638 - Attachment is obsolete: true
Attachment #8843639 - Attachment is obsolete: true
Attachment #8843640 - Attachment is obsolete: true
Flags: needinfo?(junderx)
Attachment #8865316 - Flags: review?(florian)
Comment on attachment 8865316 [details] [diff] [review]
Patch for the test that was failing

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

Thanks! (and sorry for the long delay) The code change to the test looks good, but the indent shouldn't use tabs.

Note: this test has been renamed to browser/base/content/test/pageinfo/browser_pageinfo_image_info.js
Attachment #8865316 - Flags: review?(florian) → review+
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7aacc0f1690
wait for a loadend event before using the width and height of the Page Info media preview, r=florian.
https://hg.mozilla.org/mozilla-central/rev/e7aacc0f1690
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
I have reproduced this Bug on Nightly 53.0a1 (2017-01-01) on Windows 10, 64 Bit!

The bug's fix is now verified on latest  Nightly 55.0a1

Build ID 	20170604030205
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170607]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.