788226 - Onload does not fire for image loads started from the page load event handler if the same image was loaded earlier on the page via a cache validator

Status: NEW

(Core :: Graphics: ImageLib, defect)

Description

[sombra2eternity]

Attachment: test.html

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:15.0) Gecko/20100101 Firefox/15.0
Build ID: 20120825192214

Steps to reproduce:

The problem here is that firefox in ver 15 send new headers (if-modified-since ...) that active some server features that was not activated in early versions. The code against those features seems a bit untested. If you load the image via URL with tamper data opened you will see a 304 header, the image loads as expected, it only fails when an image is loaded over javascript, you gets a 304 response and there is a CSS border-image property involving the image. Its a bit weird but I have a test for it.

If you remove the border-image code it works as expected. If you append a random tail to the image url preventing cache (like ajax petitions) it works as expected.

It works on chrome and opera.


Actual results:

The image object never fire the onload event.


Expected results:

Image onload event should have fired.

Comment 1

[(mostly gone) XtC4UaLL [:xtc4uall]]

Either I don't understand the Issue or the Issue is present on older Firefox Versions as well using provided Testcase since it behaves the same from Firefox 3.6. upto a recent Nightly - at least on Windows 7.

Comment 2

[sombra2eternity]

(In reply to XtC4UaLL [:xtc4uall] from comment #1)
> Either I don't understand the Issue or the Issue is present on older Firefox
> Versions as well using provided Testcase since it behaves the same from
> Firefox 3.6. upto a recent Nightly - at least on Windows 7.

The problem is simple:

var img = new Image();
img.onload = function(){alert('The image loads - Reload the page to confirm');};
img.src = src;

With the previous code, you can load an image object into img variable using javascript. Please, note that a "onload" event is set, that means whenever the browser finish the image load, the function you attach should be fired. If the image is cached by the browser it should be pretty fast, if not it can take a bit more. A common mistake is to attach "onload" event after the img.src assignation, then if the image is cached, the src will load instantly and when you attach the event, the image is entirely loaded and onload was fired a few microseconds before. Please note that this is not the case, the onload attachment is done before the src assignation which starts the image loading.

The problem is that I have proven (or I think so) with a test case, that the second time you reload this code, the event "onload" is not fired and as a consecuence you only see the message "The image loads - Reload the page to confirm" the first time. You could check it, the rest of browsers shows this message ALL the times you reload.

This happens under certain conditions, like I said before, a CSS border-image rule should be present. Surely there will be more sutiations where the bug can be triggered, but I only found that so far (not testing too harder). This is a problem since the border-image property got unprefixed in last (15) version, so Firefox is potentially breaking lots of image-load codes that worked previously if the maintainers starts adding border-image rules.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Confirmed.  Steps to reproduce:

1)  Load testcase.
2)  Hit reload button.

focusing the URL bar and hitting enter does NOT reproduce the bug, oddly.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Looks like nsImageLoadingContent::OnStopDecode is never called (?).

Comment 5

[Boris Zbarsky [:bzbarsky]]

I never even see imgStatusTracker::SyncNotify called for that image.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So in this case, we validate the image cache entry, get back an imgRequestProxy... but we seem to send no notifications for it???

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yeah, I get _no_ notifications of any sort in the nsImageLoadingContent code.  

What happens is that we call imgLoader::ValidateRequestWithNewChannel which does this:

      // We will send notifications from imgCacheValidator::OnStartRequest().
      // In the mean time, we must defer notifications because we are added to
      // the imgRequest's proxy list, and we can get extra notifications
      // resulting from methods such as RequestDecode(). See bug 579122.
      proxy->SetNotificationsDeferred(true);

But after that point, imgCacheValidator::OnStartRequest never gets called.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Oh, this is lovely.

We load the page.  We kick off an image cache validator for the border image.  Eventually, that comes back into imgCacheValidator::OnStartRequest.  This goes ahead and calls imgRequestProxy::SyncNotifyListener which calls, via the imgStatusTracker, imgRequestProxy::RemoveFromLoadGroup, which triggers the onload event.  That ends up running the JS in the testcase and entering imgLoader::LoadImage, which lands us in 
imgLoader::ValidateRequestWithNewChannel.

At this point, the imgRequest still has a non-null mValidator, because we don't null out mValidator on the imgRequest until _after_ we've done all the SyncNotifyListener bits.  So we skip notifying, because we think that imgCacheValidator::OnStartRequest will notify, but of course it won't at this point, because it cached mProxies.Count() on the stack before entering the loop.

So we should either nul out mValidator earlier, or deal with additions to mProxies from inside the loop, or not use sync notifications from imgCacheValidator::OnStartRequest, so that the notifications happen _after_ we've nulled out mValidator.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Over to Joe, and ccing jlebar in case he's interested.

Comment 10

[Joe Drew (not getting mail)]

Good Lord.

Comment 11

[Konstantin]

Could you give us a status update on this one. I'm running FF ver. 21 on windows 7 pro and still seeing the same unexpected behavior:

    foo.img = new Image();
    foo.img.addEventListener("load", function () { ... });
    foo.img.src = 'image_of_foo.png'

my foo image onload event handler never gets called

i can assure you that my 'foo' lives long enought (in my global scope) and doesn't get garbage collected before event handler should be call.

the problem manifests only when 'image_of_foo.png' was already loaded in browser cache, let's say via css or from a previous page load. However if a given image never got loaded in teh first palce then javascript listed above works just fine.

if it's already a registered and recognized bug is there any temp. workaround in place?
Perhaps the one can detect that images in fact is loaded right after .src attrbute is assigned. And if so then how it can be done?

just fyi: same "onload" code works just fine in ie/8-10, chrome and opera even when given image file was already pre-loaded

Comment 12

[sombra2eternity]

(In reply to Konstantin from comment #11)
> if it's already a registered and recognized bug is there any temp.
> workaround in place?
> Perhaps the one can detect that images in fact is loaded right after .src
> attrbute is assigned. And if so then how it can be done?

Woraround this bug, as I explained before (next time please read), is as simple as adding a tail on the image url to prevent cache, like:

img.src = 'imgurl.jpg?r='+(Math.rand()*1000);

Comment 13

[Konstantin]

i appreciate your response but this isn't a real workaround since it forces browser re-download the same image over and over again. as result recommended approach reduces custom code performance especially when such load happens from let's say  mouse over event. of course the one can create very own cache based on pure url value but it obviously an extrawork i have to do. Perhaps as i suggested in my original post there is a some sort of a flag directly on the element indicating that assigned "src" value in fact already loaded and the image can be consumed right away? Also do you have any estimate on when (if it ever happens) you can resolve this issue for good?

Thanks,
Konstantin

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Perhaps the one can detect that images in fact is loaded right after .src attrbute is
> assigned.

Check img.complete?

Seth, you've been in imagelib recently.  Do you want to take a look?  See comment 8 for details.

Comment 15

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

(In reply to Boris Zbarsky (:bz) from comment #14)
> > Perhaps the one can detect that images in fact is loaded right after .src attrbute is
> > assigned.
> 
> Check img.complete?
> 
> Seth, you've been in imagelib recently.  Do you want to take a look?  See
> comment 8 for details.

Sure, I'll take a look. You seem to have identified the problem, so hopefully the fix is not too far away. I need to discuss with Joe which solution is optimal, though. I'm not sure we can get away with delivering those notifications asynchronously, in particular.

Comment 16

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

Attachment: Catch new proxies added to imgCacheValidator during notification.

OK, after considering the options I feel that catching new proxies added during the notification loop is the lowest-risk change. I've verified that this fixes the testcase locally. This needs a try run before review, though.

Try job: https://tbpl.mozilla.org/?tree=Try&rev=2e2d2e2c3c75

Comment 17

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

Ah, crud. This fell through the cracks. I'm going to go ahead and request review on this.

Comment 18

[Joe Drew (not getting mail)]

Comment on attachment 762959 [details] [diff] [review]
Catch new proxies added to imgCacheValidator during notification.

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

I'd rather us switch to nsTObserverArray, which guarantees that if you're using its iterators and something's appended to the array, it'll be traversed.

Comment 19

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

Attachment: Catch new proxies added to imgCacheValidator during notification.

Didn't know about nsTObserverArray. Good call. This feels a _lot_ cleaner.

Comment 20

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

https://tbpl.mozilla.org/?tree=Try&rev=2633b7ffd768

Comment 21

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

Argh. No idea what happened to the try results there; did try get reset? Anyway, I pushed another try job, and if this looks OK I'll push in the patch later today.

https://tbpl.mozilla.org/?tree=Try&rev=7879ee199fb6

Comment 22

[Matthias Versen [:Matti]]

Seth: Do you forgot this patch ?

Comment 23

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

Nope, didn't forget, but there are oranges and I haven't had a chance to track them down yet.

Comment 24

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

Sigh. Try results are gone again.

https://tbpl.mozilla.org/?tree=Try&rev=66b1d5b58469

Comment 25

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

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.