721659 - Bad renderer of images with Javascript script due to reading width/height (dimensions, sizes) immediately after setting src

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P2)

Description

[ch.esperado]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0.1) Gecko/20100101 Firefox/9.0.1
Build ID: 20111220165912

Steps to reproduce:

Reading this link 
http://www.street-photo.fr/fr/video.html


Actual results:

Images are sometimes not centered and some of them cut.


Expected results:

Images are all centered and not altered in all other browsers, including
IE, Google Chrome, Opera, Safari, and even Andoid default navigator.
It was Ok on old versions of FF. Don't remember in witch one first happens the regression.

Fell free to mail-me @ ch.esperado(at)libertysurf.fr if you need the javascript's source.

Comment 1

[Matthias Versen [:Matti]]

I don't see any images at the URL, only a Flash object.

Comment 2

[ch.esperado]

Oh, so sorry, my mistake with the ling (shame on me):
http://www.street-photo.fr/fr/slideshow-des-membres/38?fold=95&name=Gekos#ja-containerwrapd-fr
Or any link in this page:
http://www.street-photo.fr/fr/slideshow-des-membres

Comment 3

[Thomas Ahlblom]

Last good nightly: 2010-07-28
First bad nightly: 2010-07-29

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=beab39d49de9&tochange=b26489e73818

Comment 4

[Thomas Ahlblom]

WFM:
Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2.25) Gecko/20111212 Firefox/3.6.25

Reproduced:
Mozilla/5.0 (X11; Linux x86_64; rv:9.0.1) Gecko/20100101 Firefox/9.0.1
Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20100101 Firefox/10.0
Mozilla/5.0 (X11; Linux x86_64; rv:11.0a2) Gecko/20120127 Firefox/11.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:12.0a1) Gecko/20120127 Firefox/12.0a1

Comment 5

[ch.esperado]

Thanks for this confirmation Thomas.

Comment 6

[ch.esperado]

I do not know what i have to do in order some dev take this ugly bug in consideration.

I will add it is a pity to see that Firefox, witch was fantastic, the best browser during decades and a great help for web devs like me; is slowly going down and down, and near to be the worse of all, nowadays. 
I do not use-it any more for my work, and near to un-install-it for good !

Comment 7

[ch.esperado]

Well, my site will continue to show an infamous warning against FF, this kind of warning we used against IE( up to 6) during decades !

Comment 8

[Matthias Versen [:Matti]]

I suspect bug 572520

[e-mail]:
>I do not know what i have to do in order some dev take this ugly bug in 
>consideration.

A non critical bug like this one needs usually weeks/months to get fixed.
What could help is a minimized testcase.

Comment 9

[Joe Drew (not getting mail)]

To diagnose this problem, at minimum we'll need a version of this site that doesn't have minified javascript. It's OK if you don't want to post that publicly; you can send it to me privately and I'll figure out what the problem is.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> A non critical bug like this one needs usually weeks/months to get fixed.

It really depends.  Moving it to the right place, as you did, is a good start....

Reporter, what would help the most here is what Joe said: a site version that uses unminified JS.  Also, anything you or someone else can do to reduce the testcase as much as possible would be good.

My guess is that the page is doing measurement of the image before the size of the new image is available, but it's hard to tell with all the indirection in the script.

Comment 11

[ch.esperado]

Emails send as requested. Isuppose too a problem with the style width:auto and heigh:auto. Strange, anyway, as the first images are correctly set.
Will try to find some time to build a page with the script unminified and less images to help bebuging.

Comment 12

[ch.esperado]

Well, as far i'm concerned, i will consider F.F. as DEAD. I un-suscribe from here and un-install. I'm furious and disappointed.
 
Reasons ?
New version each week, (nobody making tests before to publish ?)sometimes each day, and many of them brake something, making some extensions or plugins obsolete, just boring... 
Heavy, complicated and slow, comparing to others, IE included.
The worse development team. Hundreds of uncorrected  bugs for months, and, as you can see here, nobody *really* care ! it's affraying.

By by Mozilla, continue to throw the baby out with the bathwater, if you like, but without us (we are legion and sad) !

Comment 13

[Boris Zbarsky [:bzbarsky]]

The problem here is that the work needed to figure out what's going on is large.  Think days of effort just to figure out which parts of the site are responsible.

It'll happen, but there are other higher-priority things that need to be done (in my case, caring for a sick child, say).

Are you OK with me attaching the unminified script you sent me here so someone else can try to get somewhere with minimizing the testcase if you don't have time to do it yourself?

Comment 14

[ch.esperado]

It is pretty complicated to un-minimize the scripts ( in production). The site use both Mootools and Jquerry, and there is a lot of dynamic scripts to avoid JS conflicts, minimize both JS and css  on the fly (and cache them).
I had tried to make for you a static version, but it does not work, will try to figure out where is the problem. 
The script itself is located here:
http://www.street-photo.fr/fr/modules/mod_rokslideshow/tmpl/rockslideshow.js
In the minimized version, search for /* FILE# 6 
Take care of your sun (indeed more important), and thank-you for your care of this problem.
If, one day this bug is solved, please, can somebody mail-me in order i can remove this infamous F.F. warning from my code ? 

Best regards,
Christophe

Comment 15

[ch.esperado]

Despite my resolution to leave away from FF (like the MOZ devs seems to have done) here a track on this bug.
I have added an alert before all new images with the width and the height  (in pixels)of it (this.img.width). On all other browsers, the values are correct. On FF, it is very strange.
On the left, good values from Chrome or Internet explorer, on the right, wrong values from FF:
1024 681 / 1024 681
1024 681 / 1024 681
1024 681 / 1024 681
1024 681 / 1024 681
 532 800 / 1024 681
 800 800 / 1024 681
 800 800 /  532 800
 531 800 /  800 800
1024 680 /  800 800
 531 800 /  800 800
 800 800 /  531 800

As you can see it is not a minor bug. It seems to be a deep stuttering in the memory management of the javascript engine !!!!!

Comment 16

[ch.esperado]

The bug is confirmed on my side. I had passed the width and height of each images via PHP, and use those values to set each image's styles instead of the img.width and img.height javascript values. Everything works OK that way.
Dev, you had two month to take contact or look at it, i will not wait more with a broken site, so you will not see the bug any more.
I had left a comment in the script to indicate the variables you can change to reproduce-it, but, FF devs don't care, isn't it ?
http://www.street-photo.fr/fr/modules/mod_rokslideshow/tmpl/rockslideshow.js
Have a good life.
Regards.

Comment 17

[Boris Zbarsky [:bzbarsky]]

I'm not sure I follow comment 16...  That code is setting the width and height styles on the <img> tag, right?  Before it was setting them to the width/height properties of the image and this failed?  And now it's setting it to the name and id?  Are you setting the name and id to the image dimensions in your HTML or something?

In any case, img.width and img.height depend on two things: the image's styles and whether the image metadata is available.

Given that this bug appeared when bug 572520 was fixed, it sounds like your site is changing an image src and then reading width and height synchronously.  Per spec, the width and height should become available asynchronously...

Joe, I wonder whether we need to fix that for the case of "preloaded" images (and get the spec changed).  I don't think any other browser has that behavior....

Comment 18

[Boris Zbarsky [:bzbarsky]]

And by the way, I really appreciate your figuring out which exact line of code was triggering the problem!  That was _very_ useful information.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Hmm.  I wonder whether the issue is that with async notifications for precached images we're not switching from mCurrentRequest to mPendingRequest until much later.  Let me see if I can write up a testcase here.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Update the width/height of an image synchronously when src is set to something that was preloaded.

Comment 21

[Boris Zbarsky [:bzbarsky]]

I sent http://lists.w3.org/Archives/Public/public-html/2012Mar/0496.html about the spec being broken here.

I'm going to do a try build with this patch.  Would you be willing to test whether it fixes the original problem on your site?  Basically, switch the site to the old behavior for a few minutes, test with the test build, then switch it back.

Comment 22

[ch.esperado]

Thanks so much, Boris, for your care.
To resume the things and answer your questions in comment 17 , the script is a slideshow and do the followings:

It preload a bunch of images. Echo those images, one after the other (delai), with a fast swipe effect.
In order to center the images and allow different sizes, the width and height properties of each image was read by javacript before to be printed in html in the width and height "style" values.

As you figure out, It seems javascript does not read synchronously the good image sizes in FF. (no issue on all other navigators). It seems the problem occurs when two images follows with the same width and height values: the next one will have this same value and all image's size values are delayed of one step, if four same images, two steps... 

I had fixed my script that way, quick'n dirty:
Now, i read the image sizes in php "getimagesize()" and add in the source the width value in a "widthimg" variable, and height in an "heightimg" variable for each image description.

imgs.push({
file: '1311100855_1011102320_DSC_1783.jpg',
title: 'Sans titre   [4/183]',
desc: '',
widthimg: 681,
heightimg: 681,
url: ''
}); 

When i preload the images, i set the "name" value of the image with this widthimg value and the id value with the heightimg value:

loadImages: function(sources) {
...
name:  self.sources[i].widthimg,
id: self.sources[i].heightimg,
...

Then:

this.img.setProperties({
 src: this.loader.src,
 title: this.loader.title,
 alt: this.loader.alt,
 name: this.loader.name, /**Firefox bug**/
 id: this.loader.id, /** Firefox bug***/
...

Just before to print the html in javascript, i use those name and id values to set the style "width" and "height" of each image in the html, instead of the wrong width and height dynamically read by javascript:

this.img.setStyles({
 left: 200,
 top: haut,
 width: this.img.name,
 height: this.img.id,
/* ################################################
 //This is the FF bug: replace this.img.name by this.img.width & this.img.id by this.img.height to reproduce.
################################################ */

But you had figured out everything by yourself. (brilliant ;-)

Of course, i can try a compiled build if you like (i had no more compiling tools on my new pc). If you need, i can put in line an unminified - uncached version of this site on my development server and give-you the rights to edit the sources too (long work).
Or reverse my production site to the old behavior for a while, the time you are working on that. Just wee need to be synchronous, feel free to email-me and ask for your needs.

Thanks and best regards.

Comment 23

[Joe Drew (not getting mail)]

Comment on attachment 606457 [details] [diff] [review]
Update the width/height of an image synchronously when src is set to something that was preloaded.

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

::: content/base/src/nsImageLoadingContent.cpp
@@ +807,5 @@
> +        pusher.PushNull();
> +
> +        // This is a terrible hack to make nsImageFrame pick up the fact that
> +        // it needs to reflow....
> +        LOOP_OVER_OBSERVERS(OnStopDecode(req, NS_OK, nsnull));

What's going to happen when the real OnStopDecode comes through from the image?

Comment 24

[Joe Drew (not getting mail)]

I went trawling through Try. Here are the results: https://tbpl.mozilla.org/?tree=Try&rev=7f8434f967df

And here is the build, presuming you're on Windows: ftp://ftp.mozilla.org/pub/firefox/try-builds/[e-mail]-7f8434f967df/try-win32/firefox-14.0a1.en-US.win32.installer.exe

Comment 25

[Boris Zbarsky [:bzbarsky]]

Yep.  You can get all the builds for that try push from https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-7f8434f967df/

Comment 26

[Boris Zbarsky [:bzbarsky]]

> What's going to happen when the real OnStopDecode comes through from the image?

It'll get called again.  nsImageFrame happens to ignore OnStopDecode for the current request, and it would be the current request at that point, so it would work.

The other option is to add an explicit API to notify the image frame in particular here.  I could do that, I guess..

Comment 27

[ch.esperado]

Tested the nighly-build  with with old javascript on a  XP virtual machine: 
Works perfect.
I let the site unminified for one hour since now with my old javascript code, if you want to test yourself.
As far as i'm concerned, bug solved, thanks a lot.

Best Regards.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Thanks for confirming that!  I don't think we need to test against the site anymore if the build you tried fixes the problem; that just indicates that we diagnosed it correctly.

And thank you again for finding the relevant spot in your code.  That was very very helpful.

Comment 29

[ch.esperado]

Sure, it is easier to debug when we know exactly where the problem is and why it appears.
I was in the same situation to fix the javascript code in order to work around this bug.

Comment 30

[ch.esperado]

Did i have to set the status of this bug to "resolved" , or did Boris Zbarsky (thanks again) have to do-it itself ?
I don't know the habits and rules over there ;-)

Comment 31

[Boris Zbarsky [:bzbarsky]]

I'll mark the bug resolved once a patch actually lands.  That hasn't happened yet.

Comment 32

[Bobby Holley (:bholley)]

Comment on attachment 606457 [details] [diff] [review]
Update the width/height of an image synchronously when src is set to something that was preloaded.

> 
>   /**
>+   * Switch our pending request to be our current request.
>+   * mPendingRequest must be non-null!
>+   */
>+  void SwitchToPendingRequest();

This name confused me at first, because I thought it meant that we were somehow switching to mPendingRequest rather than the value contained therein. Maybe something like MovePendingToCurrent() or something? It's also possible that my confusion was unjustified, so if you think it's a good name I'm fine with keeping it.

(In reply to Boris Zbarsky (:bz) from comment #26)
> The other option is to add an explicit API to notify the image frame in
> particular here.  I could do that, I guess..

Please do that - it will make things saner when I refactor this whole API.

Looks good otherwise. r=bholley with the above.

Comment 33

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #32)

> Please do that - it will make things saner when I refactor this whole API.

Also, I'd very much like if we could call that API function _after_ making the request current. The way it works currently depends on the async-ness of reflows, which is nasty.

Comment 34

[Boris Zbarsky [:bzbarsky]]

> Also, I'd very much like if we could call that API function _after_ making the request
> current.

I'll see what I can do, but right now we use the "request is pending" bit to detect that we have to recompute the intrinsic size...

> The way it works currently depends on the async-ness of reflows, which is nasty.

We depend on that all over.  It's not an unreasonable thing to depend on.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Attachment: With those changes

Comment 36

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f6ef3a02450

Comment 37

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/0f6ef3a02450