Open Bug 788226 Opened 10 years ago Updated 3 months ago

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

Categories

(Core :: Graphics: ImageLib, defect)

15 Branch
x86
Linux
defect
Not set
normal

Tracking

()

People

(Reporter: sombra2eternity, Unassigned)

Details

Attachments

(2 files, 1 obsolete file)

Attached file 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.
Attachment #658171 - Attachment mime type: text/plain → text/html
Component: Untriaged → DOM
Product: Firefox → Core
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.
(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.
Confirmed.  Steps to reproduce:

1)  Load testcase.
2)  Hit reload button.

focusing the URL bar and hitting enter does NOT reproduce the bug, oddly.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Looks like nsImageLoadingContent::OnStopDecode is never called (?).
Component: DOM → ImageLib
I never even see imgStatusTracker::SyncNotify called for that image.
So in this case, we validate the image cache entry, get back an imgRequestProxy... but we seem to send no notifications for it???
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.
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.
Summary: Firefox unable to load an image with 304 header when border-image → 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
Over to Joe, and ccing jlebar in case he's interested.
Assignee: nobody → joe
Good Lord.
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
(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);
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
> 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.
Flags: needinfo?(seth)
(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.
Flags: needinfo?(seth)
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
Assignee: joe → seth
Ah, crud. This fell through the cracks. I'm going to go ahead and request review on this.
Attachment #762959 - Flags: review?(joe)
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.
Attachment #762959 - Flags: review?(joe) → review-
Didn't know about nsTObserverArray. Good call. This feels a _lot_ cleaner.
Attachment #779405 - Flags: review?(joe)
Attachment #762959 - Attachment is obsolete: true
Attachment #779405 - Flags: review?(joe) → review+
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
Seth: Do you forgot this patch ?
Flags: needinfo?(seth)
Nope, didn't forget, but there are oranges and I haven't had a chance to track them down yet.
Flags: needinfo?(seth)

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

Assignee: seth.bugzilla → nobody
You need to log in before you can comment on or make changes to this bug.