93015 - onLoad event does not work properly in mozilla and netscape 6.1

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P1)

Description

[dsirnapalli]

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.

Comment 1

[Phil Schwartau]

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

Comment 2

[basic]

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

Comment 3

[dsirnapalli]

Attachment: simple test case

Comment 4

[dsirnapalli]

Attachment: gif file

Comment 5

[dsirnapalli]

Attachment: mozilla image

Comment 6

[dsirnapalli]

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.

Comment 7

[basic]

Attachment: online testcase

Comment 8

[basic]

Attachment: testcase single image

Comment 9

[basic]

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!!".

Comment 10

[dsirnapalli]

-- 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.

Comment 11

[WD]

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

Comment 12

[saari (gone)]

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

Comment 13

[Asa Dotzler [:asa]]

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)

Comment 14

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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.

Comment 15

[Bob Clary [:bc] (inactive)]

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.

Comment 16

[Judson Valeski]

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

Comment 17

[Judson Valeski]

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.

Comment 18

[dsirnapalli]

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. 

Comment 19

[rpotts (gone)]

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

Comment 20

[rpotts (gone)]

oops.  that should have been bug #122150.

Comment 21

[rpotts (gone)]

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

Comment 22

[rpotts (gone)]

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.

Comment 23

[rpotts (gone)]

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*.

Comment 24

[rpotts (gone)]

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

Comment 25

[rpotts (gone)]

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

Comment 26

[rpotts (gone)]

Attachment: This patch prevents expired images from being evicted from the imgCache during history reloads...

Comment 27

[rpotts (gone)]

Attachment: This patch causes cached image requests to get added to the loadgroup too.

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)

Comment 28

[rpotts (gone)]

Attachment: v 1.2 of the patch dealing with expired cache entry eviction...

Comment 29

[rpotts (gone)]

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!

Comment 30

[Darin Fisher]

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.

Comment 31

[Winnie Lam]

Need review and super-review.

Comment 32

[Darin Fisher]

winnie: iirc, rpotts has a revised patch forthcoming.

Comment 33

[Eugene Savitsky]

Did all forgot this bug?

Patch attached 18.04.2002...

Comment 34

[rpotts (gone)]

Attachment: v 1.3 of patch

Comment 35

[Darin Fisher]

oh yeah!!

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

Comment 36

[rpotts (gone)]

Attachment: v 1.4 (updated to the tip...)

Comment 37

[Bob Clary [:bc] (inactive)]

"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 38

[Darin Fisher]

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

Comment 39

[rpotts (gone)]

Attachment: v 1.5 (addresses darins comments)

Comment 40

[rpotts (gone)]

>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 41

[Johnny Stenback (:jst)]

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

Comment 42

[rpotts (gone)]

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

-- rick

Comment 43

[rpotts (gone)]

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

Comment 44

[rpotts (gone)]

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

Comment 45

[Johnny Stenback (:jst)]

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

Comment 46

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 47

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch against 1.0 branch

Merged against the 1.0 branch.

Also includes patch from bug 150142 (imgLoader::GetMimeTypeFromContent doesn't
check for malloc failure)

ready for review

Comment 48

[Darin Fisher]

Comment on attachment 102332 [details] [diff] [review]
Patch against 1.0 branch

r/sr=darin

Comment 49

[rpotts (gone)]

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

Comment 50

[Randell Jesup [:jesup] (needinfo me)]

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).

Comment 51

[Randell Jesup [:jesup] (needinfo me)]

Attachment: previous patch plus patch from bug 129795

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

Comment 52

[Darin Fisher]

Comment on attachment 102350 [details] [diff] [review]
previous patch plus patch from bug 129795

r/sr=darin

Comment 53

[rpotts (gone)]

Comment on attachment 102350 [details] [diff] [review]
previous patch plus patch from bug 129795

r=[e-mail]

Comment 54

[Brendan Eich [:brendan]]

Comment on attachment 102350 [details] [diff] [review]
previous patch plus patch from bug 129795

a=[e-mail] -- 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

Comment 55

[Randell Jesup [:jesup] (needinfo me)]

Fix checked into branch along with the fix for bug 171053

Comment 56

[Moied]

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,

Comment 57

[rpotts (gone)]

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

Comment 58

[Moied]

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

Comment 59

[John Dodrill]

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>

Comment 60

[Mike Kaply [:mkaply]]

Is this bug ever going to be fixed?

The testcase here:

http://www.kaply.com/work/bugzilla/93015

still fails.

Comment 61

[Brendan Eich [:brendan]]

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

Comment 62

[Brian Ryner (not reading)]

This might just work once bug 83774 is fixed.

Comment 63

[Boris Zbarsky [:bzbarsky]]

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.

Comment 64

[Mike Kaply [:mkaply]]

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.

Comment 65

[Johnny Stenback (:jst)]

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).

Comment 66

[Boris Zbarsky [:bzbarsky]]

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....

Comment 67

[Darin Fisher]

(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.

Comment 68

[Boris Zbarsky [:bzbarsky]]

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).

Comment 69

[Johnny Stenback (:jst)]

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.

Comment 70

[Michael Buckland]

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

Comment 71

[Johnny Stenback (:jst)]

Attachment: Possible fix (untested)

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?

Comment 72

[Michael Buckland]

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

Comment 73

[Johnny Stenback (:jst)]

Attachment: Merge the above to the trunk, and "fix" imglib

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...

Comment 74

[Stuart Parmenter]

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 75

[Darin Fisher]

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.

Comment 76

[jag (Peter Annema)]

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.

Comment 77

[Johnny Stenback (:jst)]

Attachment: Same as above with nits fixed, and slight change to setting mLoadGroup in imgRequestProxy::Init().

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.

Comment 78

[Johnny Stenback (:jst)]

Attachment: Missing Makefile.in change

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

Comment 79

[Darin Fisher]

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 80

[Johnny Stenback (:jst)]

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.

Comment 81

[David G King]

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.

Comment 82

[Boris Zbarsky [:bzbarsky]]

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...

Comment 83

[Bradley Baetz (:bbaetz)]

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 84

[(not reading, please use [e-mail] instead)]

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

a=sspitzer

Comment 85

[Johnny Stenback (:jst)]

FIXED.

Comment 86

[Darin Fisher]

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

Comment 87

[(not reading, please use [e-mail] instead)]

this change caused an odd regression, see bug #205236

Comment 88

[Johnny Stenback (:jst)]

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

Comment 89

[Johnny Stenback (:jst)]

Attachment: Better fix for imglib by Pavlov and myself.

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

Comment 90

[Darin Fisher]

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 91

[jag (Peter Annema)]

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 92

[Asa Dotzler [:asa]]

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

Comment 93

[Johnny Stenback (:jst)]

Fix checked in. FIXED.