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
Attachment #658171 - Attachment mime type: text/plain → text/html
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.
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
(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.
(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.
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
Ah, crud. This fell through the cracks. I'm going to go ahead and request review on this.
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 #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 ?
Nope, didn't forget, but there are oranges and I haven't had a chance to track them down yet.
Sigh. Try results are gone again. https://tbpl.mozilla.org/?tree=Try&rev=66b1d5b58469
Assignee: seth.bugzilla → nobody
You need to log in before you can comment on or make changes to this bug.