My test case consists of a web page which displays two images. I have onLoad event on both the images. Also i have onLoad on body tag also. onLoad on body tag gets called before onLoad on images is called. It works fine in netscape 4.7. source code for test case can be found at: http://bubblegum/ngdriver/suites/dharma/EmbedSmokeTest.html Steps to reproduce: 1. go to My test case consists of a web page which displays two images. I have onLoad event on both the images. Also i have onLoad on body tag also. onLoad on body tag gets called before onLoad on images is called. It works fine in netscape 4.7. source code for test case can be found at: http://bubblegum/ngdriver/suites/dharma/EmbedSmokeTest.html Steps to reproduce: open mozilla. 1. go to http://bubblegum/ngdriver/tester.html 2. replace suites/suite1 with suites/dharma and click on Find Testcases in: 3. Specify a name for the test run. select EmbedSmokeTest.html and click RunTestcase. 4. click on resultfile*.html 5. you see image loads are failing. sometime only one image is loaded othertime both images are not loaded. But if you run the same testcase from netscape 4.7 it passes.

Browser, not engine. Reassigning to Event Handling for further triage -

I can't access the testcase but I think I might have a simple testcase. http://www.geocities.com/_basic/test.html the text below the image is suppost to be: img body but when you click on reload it is: body img and when you do shift reload: img body tested on build 2001073003 win98

Your comments were little confusing to me. Attached abovee is a simple test case two images are also attached.when i open this testcase from IE or Netscape 4.7 the results are mozload: true gogload: true pageload: true but when i run it from mozilla different time it gives different results. somtimes , for the first time only the result is gogload: false mozload: false pageload: true but sometime it is ok, doing repeated reload or shift reload for sometime the results will fail. it will show gogload: false mozload:false pageload: true. shouldn't it behave same as netscape 4.7.

In "testcase single image", when I load the page for the very first time (after clearing cache, or using shift reload), "image loaded!!" popsup before "page loaded!!". After that, if I do a reload or just visit the page again I'll get "page loaded!!" before "image loaded!!".

-- So it this not a bug. why does this not happen in 4.7. any number of times you do reload in 4.7 the "image loaded" appears first and then the "page loaded" appears next. why is it different in mozilla now. so how do i know when i load or reload a page what is the last one to fire. I was trying a test case which contains some images. i set some imagevariables to "false" initially. I was trying to know whether the images are loaded properly or not. i am firing events when the images are loaded. i set imagevariables to true when the images are loaded. and i wrote onload event on body. in that function i check the imagevariables and return the results. so since first time the onload on body fires after images are loaded the imagevariables are true and test case passes. but when i do reload since pageload occurs first the test case fails.

I get Page Loaded before Image Loaded with Linux and also Winme / Win98 OS-> ALL

*** Bug 96926 has been marked as a duplicate of this bug. ***

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 (you can query for this string to delete spam or retrieve the list of bugs I've moved)

Major corporations depend on eapp bugs, and need them to be fixed before they can recommend Mozilla-based products to their customers. Adding nsbeta1+ keyword and making sure the bugs get re-evaluated if they are targeted beyond 1.0.

eapp was incorrectly used to change this to nsbeta1+. Resetting to nsbeta1 to nominate. This is an important issue and deserves to be nsbeta1+ by the ADT.

-> Rick, joki and I think this is a loadgroup/uriloader OnStop() firing issue.

dharma, Can you retest this? It looks like your test case is broken. I can't get basic's test case in #7 to fail in today's win2k trunk build.

hi jud, My test case is not working because it is looking for images in local directory which it does not have. You have to put the source of my test case(#3) locally and also the images(#4 and #5) locally in the same folder as test case. However, there is not much difference b/n my test case and basic's. basic modified my test case to point to the attached images like shown below so that you can run directly. <img src="http://bugzilla.mozilla.org/showattachment.cgi?attach_id=44583" height=58 width=600 onLoad = "mozillaimageLoaded();"> However i can still reproduce the bug using basic's test case(#7). i am testing it on winNT. I am reopening the file again and again. sometimes its working fine but sometimes its failing.

i believe that this bug 'might' be related to bug #120150... i'm seeing the testcase run correctly when run by clicking on the 'online testcase' link. however, when i run the testcase via the 'Edit' link and then go BACK and FORWARD the image onLoad events do *not* fire. i believe that the difference in behavior is due to the IFRAME which contains the testcase in the 'edit' page... this is the only abnormaility that i'm seeing in these tests. -- rick

oops. that should have been bug #122150.

ok... so it appears that the *real* problem is that there is a race condition when images are loaded from the cache... when we load images from the cache the onLoad handlers 'sometimes' fire after the document onLoad handler... -- rick

The race condition is due to the fact that images which are loaded from the cache are *not* added into the loadgroup. See: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/modules/libpr0n/src/imgRequestProxy.cpp#103 It appears that this is a 'feature' that hyatt implemented ;-) See: http://bugzilla.mozilla.org/show_bug.cgi?id=77002#c44. As it turns out, adding the requests is *not* a waste of time because it allows the various notifications to be correctly synchronized. Currently, when a document is loaded from the cache, there is a race between to document finishing and EACH of the images... So, in general, the onLoad events for the images will come in AFTER the onLoad event for the document.

Unfortunately, this is not the only problem :-( Another related problem is that the onLoad event for an image can be fired multiple times !! This is because image loading is performed by the image frame... Unfortunately, when the frame model is torn down due to style changes, when the frame is recreated, the image is reloaded... and once the image has finished 'reloading' the image frame fires the onLoad event *again*. In order to fix this situation, the HTMLImageElement not the ImageFrame should be responsible for loading (reloading) and firing the onLoad events... Until this is fixed, each time style changes (which require reframing) occur EVERY image on the page will fire its onLoad event *again*.

Alas, there are not merely two problem with image loading :-( A third problem is that there seems to be a fundamental disconnect about the LoadFlags that are used to load images... This means that when images are fetched from the cache as part of history loads (ie. back/forward) they are *validated* !! So, any expired images end up getting evicted from the memory cache during history loads... It turns out that we don't really 'notice' most of the time because the image is also stored in the disk cache... So the image is simply streamed in from the disk cache and the network is not hit. However, this is quite a bit of extra useless work! And humorously, the fact that we hit the disk cache exagerates the race condition between images and the document being loaded... This is because image loading from the memory cache is essentially synchronous. However, loading from the disk cache is asynchronous :-( So, if we fix this cache eviction problem we will also eliminate the race condition that is causing the attached test cases to have their image onLoad handlers fire *after* the document onLoad. Of course, the onLoad event will *still* have the race condition if the image is *not* in the memory cache, but *is* in the disk cache... And the onLoad handler will *still* fire each time stylistic changes cause a reframing of the document... But it's a start ;-) -- rick

oh yeah... i forgot to mention one other problem that affects this bug... it turns out that the nsImageFrame uses a PLEvent to fire the onLoad handler. this means that if image *are* in the loadgroup and an image is the last request in the loadgroup, then PLEvent to fire the onLoad handler can end up in the event queue *after* the OnStop notification for the document (and its onLoad handler)... The code in nsImageFrame to defer firing the image onLoad event was added to fix bug #66022. Unfortunately, i had to *reopen* this bug because somewhere along the line mozilla has regressed no longer runs the testcase :-( This introduces the possibility of removing this code ;-) -- rick

In addition to applying this patch, the patch attached to http://bugzilla.mozilla.org/show_bug.cgi?id=66022 must be BACKED OUT. These two things will cause onLoad notifications from images in the disk cache to be fired before teh documents onLoad notification... These are BIG changes!! 1. image onLoad notifications will fire synchronously. (see bug #66022) 2. cached images (from the disk cache) get placed in the loadgroup. (see bug #77002)

The above patch tries to deal with the following issues: 1. Prevent expired cache entries from being evicted from the cache during history loads. 2. Rationalize the use of LoadFlags !! 3. Centralize the creation of the *real* channel used to fetch the image data. This ensures that ALL image channels are consistantly initialized... Currently, if cache validatation fails and we need to fall-back to the network for the data, the following attributes are *not* set on the channel: * notification callbacks * referrer * document URI This could cause cause problems !! 4. Added NS_RELEASE(request) to several early return points to prevent leaks. 5. Do *not* always pass VALIDATE_ALWAYS into the 'cache validataion' channel... This can cause unnecessary protocol validatation !! 6. nsIRequest::Cancel(...) is now called on the 'cache validation' channel if the cache entry is still valid. This is because bug #113959 has been fixed!

i read through the patch and it looks good... when i get the chance i want to read through it again more thoroughly... maybe monday.

Need review and super-review.

winnie: iirc, rpotts has a revised patch forthcoming.

Did all forgot this bug? Patch attached 18.04.2002...

oh yeah!! pavlov: this is the patch i mentioned to you that rpotts was working on :)

"online testcase" http://bugzilla.mozilla.org/attachment.cgi?id=44669&action=view appears to wfm (pagload fires last) in mozilla 1.0.1 and 1.1b on win2k from 20020815. "testcase single image" http://bugzilla.mozilla.org/attachment.cgi?id=44670&action=view appears to still be broken in mozilla 1.0.1 and 1.1b on win2k from 20020815 Since we have missed the 1.0.1 train, can we get this for 1.2?

Comment on attachment 92627 [details] [diff] [review] v 1.4 (updated to the tip...) >Index: modules/libpr0n/src/imgLoader.cpp >+static PRBool RevalidateEntry(nsICacheEntryDescriptor *aEntry, >+ aEntry->GetMetaDataElement("MustValidateIfExpired", >+ getter_Copies(value)); >+ if (PL_strcasestr(value, "true")) { >+ bValidateEntry = PR_TRUE; >+ } nit: since you own this metadata field, how about just trusting that you will always store lowercase? then you could just use PL_strcmp instead, right? >+/// NS_ASSERTION(request->mCacheEntry == entry, >+/// "imgRequest has wrong cache entry"); nit: how about just deleting this? >+ entry->GetMetaDataElement("MustValidateIfExpired", >+ getter_Copies(value)); >+ if (PL_strcasestr(value, "true")) { same thing again... >+ bValidateRequest = PR_TRUE; >+ } >+ } >+ // >+ // LOAD_FROM_CACHE allows a stale cache entry to be used... Otherwise, >+ // the entry must be revalidated. >+ // >+ else if (!(requestFlags & nsIRequest::LOAD_FROM_CACHE)) { >+ bValidateRequest = PR_TRUE; >+ } looks like some of this is duplicate logic... can it be lumped together into one subroutine? >Index: modules/libpr0n/src/imgRequestProxy.cpp >+ // XXX: This does not deal with the situation where cached content >+ // is being revalidated. In this case, the request needs to >+ // be added in case the cache entry is doomed. do we need to worry about this now? sr=darin

>Index: modules/libpr0n/src/imgRequestProxy.cpp >+ // XXX: This does not deal with the situation where cached content >+ // is being revalidated. In this case, the request needs to >+ // be added in case the cache entry is doomed. Right now, i think we should just leave the comment... If this situation becomes problematic, then we can fix it later... -- rick

Comment on attachment 97828 [details] [diff] [review] v 1.5 (addresses darins comments) +PRBool imgCache::Get(nsIURI *aKey, PRBool *aHasExpired, <...>) { LOG_STATIC_FUNC(gImgLog, "imgCache::Get"); @@ -222,12 +222,13 @@ if (NS_FAILED(rv) || !entry) return PR_FALSE; - if (aDoomIfExpired) { + if (aHasExpired) { This is against the XPCOM rules, out parameters can't be null, return an error code if it is. Or is this case "special" for some reason? - In imgLoader.cpp (RevalidateEntry()): + aEntry->GetMetaDataElement("MustValidateIfExpired", + getter_Copies(value)); Indentation off-by-one. - In NewImageChannel(): + rv = NS_NewChannel(aResult, + aURI, // URI + nsnull, // Cached IOService + nsnull, // LoadGroup + callbacks, // Notification Callbacks + aLoadFlags); You don't want to pass the load group to NS_NewChannel() here? Or would we not want to cancel the actual channel that's pulling down the data if the load group is canceled, and so on...? - Further down in the same file: + requestFlags = (requestFlags & ~(LOAD_FLAGS_CACHE_MASK)) | + (aLoadFlags & LOAD_FLAGS_CACHE_MASK); No need for the parens around LOAD_FLAG_CACHE_MASK, the macro does have the needed parentesis in it (as it should). Either way works though... (there's a few places with this same "problem"). ... - return asyncOpenResult; + return openRes; } else { // If it isn't caching channel, use the cached version. // XXX we should probably do something more intelligent for local files. ... Remove the else-after-return here. Other than that, r/sr=jst

ok... i've updated my patch to include jst's comments. -- rick

ok... I've checked the fist patch (v1.5) into the trunk. this cleans up imglib abuses of the networking layer... next, up is forcing the onLoad() notification for images to be fired synchronously (not through a PLEvent indirection...) -- rick

adding bug #66022 as a blocker... as long as we fire image onLoad handlers async (to prevent the stack overrun) we cannot ensure that they will fire before the document's onLoad() -- rick

Well, we could if we'd pull one of those dummy request tricks again in the image code :-)

Pushing milestone (1.0.1 is past), nominating for 1.0.2 We need to get the patch onto the branch. This has a fair bit of impact on embedders who don't use a disk cache due to this: http://bugzilla.mozilla.org/show_bug.cgi?id=93015#c24 A third problem is that there seems to be a fundamental disconnect about the LoadFlags that are used to load images... This means that when images are fetched from the cache as part of history loads (ie. back/forward) they are *validated* !! So, any expired images end up getting evicted from the memory cache during history loads... It turns out that we don't really 'notice' most of the time because the image is also stored in the disk cache... So the image is simply streamed in from the disk cache and the network is not hit. Also, rpotts (Rick?), a more serious issue: Please, please post that actual patch as applied. After trying to apply this patch to a 1.0 branch, I found that there problems due to no final patch having been uploaded after you addressed jst's comments. As best I can tell, there were changes to files that weren't even mentioned in the last uploaded patch, like imgILoader.idl. Actually, I think that some of those were part of the bug 145579 patch, and the two patches got checked in together. Normally this wouldn't be a problem, but when trying to merge the patch to an older branch it can cause problems. If I hadn't hit serious problems trying to get it working after hand-applying it, I might have missed some critical changes. I _think_ I have them all, but even now I'm not 100% sure. It was also very painful trying to apply your '1.5' patch by hand as well, since you didn't use -u option for cvs diff. (Patch failed to apply any of the hunks for most files, so I had to merge by hand.) If I can get this working, I'll upload a branch patch. Right now I have it compiling and running, but I get a lot of "Overwriting an existing document channel!" assertions and it doesn't actually load URLs.

Merged against the 1.0 branch. Also includes patch from bug 150142 (imgLoader::GetMimeTypeFromContent doesn't check for malloc failure) ready for review

Comment on attachment 102332 [details] [diff] [review] Patch against 1.0 branch r/sr=darin

are you *sure* you want to land this patch on the 1.0 branch? it's unclear that it has gotten sufficient testing to verify that no regressions were introduced. also, if this patch is move over then the patch for bug #129795 should probably also be moved onto the branch. That's the one about 'wrong document channel' assertions. -- rick

You're correct, bug 129795 is needed as well (I have it in my local tree, and forgot to throw it into the branch patch - I got mixed up as to whether it was already in the branch or not). In fact, that one tripped me up for a while until I realized it was needed, and Darin and I were emailing back and forth until I traced it down. New patch forthcoming. This (the lost images on Back/Forward) is a significant problem for anyone who turns off the disk cache (or sets it to 0 size), and this would apply to all embedders for devices without mass storage, or with only read-only storage (CD, etc). It's important to us because our mozillas run on diskless client machines, or machines with a disk used only for the kernel and for swap space. Obviously I won't be the person giving branch approval on this, but I'd like to see it in 1.0.2 if the other 1.0 drivers agree. If not, we'll need to continue taking the patch in our local tree (and other embedders could do so as well, so long as they can find this patch, which might not be too easy).

addition to the previous patch: patch from 129795 adds masking of the default loadgroup request flags to the low 16 bits.

Comment on attachment 102350 [details] [diff] [review] previous patch plus patch from bug 129795 r/sr=darin

Comment on attachment 102350 [details] [diff] [review] previous patch plus patch from bug 129795 r=rpotts@netscape.com

Comment on attachment 102350 [details] [diff] [review] previous patch plus patch from bug 129795 a=brendan@mozilla.org -- looks like newChannel in NewImageChannel is unused (variables should be moved down to their first assignment so they can be initialized, to avoid orphans like this). There are other nits I could pick, but they'd make for branch/trunk deviation. Maybe later. One fix to pick up from the trunk: rev 1.40 of imgRequestProxy.cpp reorders member initializers to avoid a warning. Please include this to match the trunk and avoid a branch warning. Another possible pick-up: bug 171053's patch is trivial, confined to NewImageChannel in imgLoader.cpp, and may be worth including -- rpotts, what do you think? We can do this as a separate step, for sure. Anyway, with that member initializer warning fix, Asa, blizzard, tor, and I believe this patch is good to go for the 1.0 branch. I personally went through trunk vs. patched branch diffs for all the files, and think I see the right bits. But we're more than a little nervous; this code is dragon-scary, and it has felled more than few heroes in living memory. Separate issue for nsPresContext.cpp -- do we want to put the uninitialized variable crash fix, rev 3.200, for bug 160656, into the 1.0 branch? /be

Fix checked into branch along with the fix for bug 171053

Randell: while trying to verify this bug I did few tests with the attached testcases in this bug, But as per the comment #6 (mozload: pass, gogload: pass, pageload: pass). Senario 1: After clearing the disk cache and performing several shift reloads we can reproduce this bug as (mozload: pass, gogload: fail, pageload: pass). Senario 2. But when we clear the disk cache and restart the browser again we dont see the bug anymore means (mozload: pass, gogload: pass, pageload: pass). if you feel this is right behavior then can you please mark it fixed, so that I can verify this bug. Thanks,

hey Moied, The patch that has been checked in on both the trunk and the branch is only the 'first' part of the fix :-) Please see comment #43. There is more work to be done to completely fix the problem... But this first patch goes lays the groundwork and fixes many of the issues... Unfortunately, the attached test cases will *still* fail as you indicated... I'm planning on leaving this bug open until the rest of the issues around this bug get resolved. -- rick

verified fix checked in and changing the keyword "fixed1.0.2" to "verified1.0.2".

Redefining src of non-gif image (gif images too) doesn't happen with the onLoad event handler but will happen with another event handler such as onMouseOver. Below, a simple function (thecode) trys to change the src of an image called "thepics". An image called doesitwork.jpg should be replaced by another called itdoeswork after all the objects are loaded. Don't wait for that to happen if you're already up in years. onMouseOver uses the same code to change "thepics" successfully. The code is functional 12/29/02. I'll leave the images there for a few months. <HTML> <HEAD> <SCRIPT> function thecode() { thepics.src="http://john.dodrill.name/itdoeswork.jpg"; }; </SCRIPT> </HEAD> <BODY bgColor="#ffffff" onLoad="thecode();"> <a href="http://john.dodrill.name"><IMG NAME="thepics" SRC="http://john.dodrill.name/doesitwork.jpg" border=0 onMouseOver="thepics.src='http://john.dodrill.name/itdoeswork.jpg';"></a> </BODY> </HTML>

Is this bug ever going to be fixed? The testcase here: http://www.kaply.com/work/bugzilla/93015 still fails.

mkaply, this bug needs a new owner (rpotts isn't working on Mozilla right now). Can you help find one? Maybe jst has some thoughts. /be

This might just work once bug 83774 is fixed.

So what is this bug about at this point? The testcase mentioned in comment 60 fails the "testcase must clearly state or show what the expected behavior is, otherwise it is useless" test.... Is it basically about the problem mentioned in comment 25 and comment 43? If so, it will NOT be fixed by bug 83774. Fixing it is nearly impossible unless libpr0n actually fires its notifications asynchronously, and the module owner of libpr0n has stated that it will absolutely not do that, so.... the only other option is to have the PLEvent that fires the real DOM event insert itself into the loadgroup as a fake nsIRequest to keep the document's onload from firing till after the image's onload fires.

Sorry, my apologies. The testcase should show three message boxes, with 2, 2, and then a message about appointments. This is how it worked in 4.x and IE. In Mozilla it only shows two message boxes with 0's. The testcase is from a customer. I will clean it up to make it more obvious what is happening.

Bz, I remember talking about doing exactly that with someone not too long ago (i.e. putting dummy requests in the loadgroup while the content/layout code posts image onload events).

OK. Perhaps we just need a little class that can be reused, inherits from PLEvent and nsIRequest, and will add/remove itself from the loadgroup on construction and destruction (it will need to take a loadgroup in the constructor). We'd need that for image loads and style sheet loads, at least....

(this bug needs a new owner!) bz: jst mentioned this to me before... he was saying that it's unfortunate that the loadgroups can't support the notion of a dummy request because for the loadgroup it could just be implemented as a simple counter. but, given all the machinery surrounding loadgroups and the fact that nsILoadGroup is a frozen API, there might be less code bloat if we just reused the current API with a dummy nsIRequest impl as you suggest.

Or we could make an nsILoadGroup2 for this.... Whichever leads to less code, confusion, and memory churn (and I'm not quite sure how to evaluate those criteria here).

Right, now I remember talking about this counter thing in nsILoadGroup, IIRC it was suggested by rpotts before he left Netscape. I'd say go that route and add an nsILoadGroup2 and remove all our silly dummy requests and replace them with this new counter scheme.

jst, can you work on this, or suggest an ownwer if you are unable?

This isn't really the ideal fix for this, it just reuses the presshell's dummy request code and adds one of those to the loadgroup while firing events for images. I don't have the time to invent nsILoadGroup2 at the moment, so maybe we should go with something like this for now?

Discussedd in bug triage. Are we going to take current fix or continue working on it? Please update target milestone.

The previous patch wasn't enough. There was one more problem and that was that imglib wasn't adding image requests to the load group when loading images from the cache. That caused the load of the cached image to happen asynchronously w/o it putting a request into the loadgroup, and thus the notifications to the image observers happened *after* the page was done loading, and the onload handlers fired in the wrong order. Now, I'm no imglib hacker, so I dunno what this means in the grand scheme of things. Things seem to be working just fine, but I have no idea what unknown consequences this might have...

I'm certainly scared of that patch. Dealing with load groups has always been very fragile and is going to require a lot of testing...

Comment on attachment 122721 [details] [diff] [review] Merge the above to the trunk, and "fix" imglib >Index: content/base/src/nsImageLoadingContent.cpp >+ nsCOMPtr<nsILoadGroup> loadGroup; >+ document->GetDocumentLoadGroup(getter_AddRefs(loadGroup)); >+ NS_ENSURE_TRUE(loadGroup, NS_ERROR_UNEXPECTED); >+ >+ nsCOMPtr<nsIRequest> dummyRequest; >+ rv = nsDummyLayoutRequest::Create(getter_AddRefs(dummyRequest), nsnull); >+ if (!dummyRequest) { >+ return NS_ERROR_OUT_OF_MEMORY; >+ } footprint nit: each of these early returns adds code for all stack based destructors. GCC (at least) does not optimize this code very well and will generate duplicate code for all early returns :-( ... can we just check for null loadgroup and null dummyRequest before using them down below? not as clean, i know, but maybe something to consider given how bad compilers are with all of our early return code. >+ // Add the dummy request to the load group only after all the early >+ // returns here! >+ loadGroup->AddRequest(dummyRequest, nsnull); > > PL_InitEvent(evt, this, ::HandleImagePLEvent, ::DestroyImagePLEvent); > > return eventQ->PostEvent(evt); so the comment is not exactly true given that PostEvent could fail. consider the case where we are shutting down. could we end up with a reference cycle if RemoveRequest is not called? maybe capture the return value of PostEvent and only if NS_SUCCEEDED(rv) call AddRequest. >Index: modules/libpr0n/src/imgRequestProxy.cpp >- if (!(imageStatus & imgIRequest::STATUS_LOAD_COMPLETE) && >- !(imageStatus & imgIRequest::STATUS_ERROR)) { >+ if (!(imageStatus & imgIRequest::STATUS_ERROR)) { you've verified that we call RemoveRequest elsewhere in the STATUS_LOAD_COMPLETE case? sr=darin with nits addressed.

This patch doesn't compile as-is. The #include "nsDummyLayoutRequest.h" requires layout/html/base/src to be added to Makefile.in as an include directory. This seems to fix bug 185547 though.

This is basically the same as the above, with Darin's nits fixed, and I talked this over with Pavlov and we came up with a minor tweak to what I'd done to imgRequestProxy::Init(), but no major changes.

Duh, I forgot to include the missing Makefile.in change that jag pointed out in the previous patches...

Comment on attachment 122804 [details] [diff] [review] Same as above with nits fixed, and slight change to setting mLoadGroup in imgRequestProxy::Init(). >Index: content/base/src/nsImageLoadingContent.cpp >+ PL_DestroyEvent(evt); i was about to say "just call |delete evt;|" but then i realized that PL_DestroyEvent actually cleans up some stuff allocated in PL_InitEvent. so, you are totally correct in calling PL_DestroyEvent here, but then i realized that in necko-land we have lot's of error cases just like this, but with a straight call to delete instead of PL_DestroyEvent! sure, just a shutdown leak, but good to know ;-) sr=darin

Comment on attachment 122804 [details] [diff] [review] Same as above with nits fixed, and slight change to setting mLoadGroup in imgRequestProxy::Init(). Given the keywords in this bug, I think we want this for 1.4 final.

As per comment #80, asking for 1.4 final consideration. I think it should, especially as one of the bugs it blocks (bug #185547) is already a 1.4 blocker.

Actually, iirc gcc 3.2 optimizes early returns correctly with -O2. bbaetz has more info. Optimizing code based on the fact that we compile with almost no optimization on gcc 2.9x is sort of silly...

Re comment 75, what bz said. Make the code readable, and let the compiler deal with it. We're moving to 3.2 RSN, hopefully, and then we can turn on -O2.

Comment on attachment 122804 [details] [diff] [review] Same as above with nits fixed, and slight change to setting mLoadGroup in imgRequestProxy::Init(). a=sspitzer

FIXED.

looks like the premature onLoad was buying us ~30ms on Tp. oh well... now i guess the numbers on btek are more accurate :-/

this change caused an odd regression, see bug #205236

Reopening since this caused regression bug 205236. The imgRequestProxy part of this diff was backed out.

This changes imglib to be more consistent in how it deals with loadgroups, and this fixes the remaining part of this bug.

Comment on attachment 123205 [details] [diff] [review] Better fix for imglib by Pavlov and myself. >+void imgRequestProxy::RemoveFromLoadGroup() ... >+ /* calling RemoveFromLoadGroup may cause the document to finish >+ loading, which could result in our death. We need to make sure >+ that we stay alive long enough to fight another battle... at >+ least until we exit this function. >+ */ >+ nsCOMPtr<imgIRequest> kungFuDeathGrip(this); >+ >+ mLoadGroup->RemoveRequest(this, NS_OK, nsnull); ... >+} this is scary... under what cases is this grip needed? i see that it's just copy-paste, but does anyone know why the caller wouldn't have an owning reference? anyhow, this patch looks fine to me. sr=darin

Comment on attachment 123205 [details] [diff] [review] Better fix for imglib by Pavlov and myself. Too bad patchmanager doesn't notify you of conflicts. Restoring r=pavlov annotation and requesting approval for 1.4.

Comment on attachment 123205 [details] [diff] [review] Better fix for imglib by Pavlov and myself. a=asa (on behalf of drivers) for checkin to 1.4

Fix checked in. FIXED.