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

Status: VERIFIED FIXED

(Firefox :: Page Info Window, defect)

Description

[arni2033]

>>>   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

Comment 1

[junderx]

Attachment: 1326910.patch - Proposed fix for Bug 1326910

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.

Comment 2

[Florian Quèze [: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.

Comment 3

[junderx]

Attachment: Patch that includes recommended changes

Patch with all the nice change recommendations implemented

Comment 4

[Florian Quèze [: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 :-/.

Comment 5

[junderx]

Attachment: Use inline function this time.

Changed the imgSize() function to inline since it is only called one place.

Comment 6

[Florian Quèze [: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?

Comment 7

[junderx]

Hi Florian, Please use my full name, John Underwood.  You could also mention junderx.

Thanks,
John U

Comment 8

[Florian Quèze [:florian]]

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.

Comment 9

[Carsten Book [:Tomcat]]

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=74764752&repo=mozilla-inbound

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40ea52619427
Backed out changeset 8475919c50b5 for test failures in browser_bug517902.js on a CLOSED TREE

Comment 11

[Florian Quèze [:florian]]

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.

Comment 12

[junderx]

I'll take a look to sort it out.

Comment 13

[junderx]

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.

Comment 14

[Florian Quèze [:florian]]

(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).

Comment 15

[junderx]

Attachment: Patch for the test that was failing

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.

Comment 16

[Florian Quèze [: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).

Comment 17

[junderx]

Attachment: First version of the test that fails to reach the callback

Comment 18

[junderx]

Attachment: Second version of the test that fails to reach the callback

Comment 19

[junderx]

Attachment: Third version of the test that fails to reach the callback

Comment 20

[junderx]

Attachment: Fourth version of the test that fails to reach the callback

Comment 21

[junderx]

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 22

[Florian Quèze [:florian]]

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)

Comment 23

[Florian Quèze [:florian]]

(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.

Comment 24

[junderx]

Attachment: Patch for the test that was failing

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.

Comment 25

[Florian Quèze [: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

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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.

Comment 27

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

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

Comment 28

[Saddam Hossain]

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]