74506 - enabling imglib2 has caused some pages to stop firing onload

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect, P1)

Description

[John Morrison]

As libpr0n was enabled on each platform (first win32, then mac, then linux),
the following mock pages used in the page load test began to fail to fire 
onload. These are URLs to a "static" of these pages, without the reloading
javascript that drives the test. 

http://jrgm.mcom.com/perf/loadtime5/base/www.msn.com/index.html
http://jrgm.mcom.com/perf/loadtime5/base/www.spinner.com/index.html
http://jrgm.mcom.com/perf/loadtime5/base/www.zdnet.com_Gamespot.com/index.html
http://jrgm.mcom.com/perf/loadtime5/base/www.tomshardware.com/index.html
http://jrgm.mcom.com/perf/loadtime5/base/www.cnn.com/index.html
http://jrgm.mcom.com/perf/loadtime5/base/www.voodooextreme.com/index.html

Some of the above will fire onload on a second visit, or a reload of the page
(msn.com and spinner.com in my test on win2k), but definitely none will fire
onload on an initial visit. (Or, they may fire onload if left long enough, but
the test cancels a load after waiting 45 seconds for 'load' event to fire). 

This is related to libpr0n, although it may be that it is just causing some 
other code paths in other modules to be exercised differently. At any rate, 
it would be good to sort out what is happening with these pages before a 
beta date.

Comment 1

[Darin Fisher]

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

Comment 2

[Stuart Parmenter]

Attachment: fix for image problem

Comment 3

[Stuart Parmenter]

this fixes gamespot and spinner.  the rest are probably being hurt by the 
textfield bug.

Comment 4

[Stuart Parmenter]

need some hot r= and sr= action

Comment 5

[Brendan Eich [:brendan]]

I can sr= but would like an informed r= from someone who knows
nsHTMLImageElement.cpp.  Who would that guy be?

/be

Comment 6

[Stuart Parmenter]

jst ?

Comment 7

[Johnny Stenback (:jst)]

I don't see how this would fix this bug (I'm not saying it doesn't, but I don't
understand how), is the key to the fix here to hold on to the image request for
the lifetime of images created with img = new Image(); img = 'someurl'? If so,
do we really want that, I don't see where we'd use it after ::OnStopDecode(),
except in ::GetComplete() where checking for it's existance should work, AFAIK.

I don't see how mRequest->Cancel(NS_ERROR_FAILURE); in
nsHTMLImageElement::SetSrcInner() could ever be called, the only place mRequest
is set is sinside ::SetSrcInner() (below the call to mRequest->Cancel()) inside
the if (mOwnerDocument) and every time ::SetSrcInner() is called and
mOwnerDocument is non-null we set mOwnerDocument to null
(NS_RELEASE(mOwnerDocument)), and mOwnerDocument is only initialized once per
object (in ::Iniitialize()).

(assuming I'm correct this means the old code (mLoader->RemoveFrame(this)) was
moot too)

I don't get it, key me in here, please?

Comment 8

[neeti]

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

Comment 9

[John Morrison]

As of Tuesday morning's builds, all of the URL's noted above are loading 
in the page loading test, as well as 
http://jrgm.mcom.com/perf/loadtime5/base/www.yahoo.com/index.html, which 
hadn't been loading for some period of time before the libpr0n stuff was
switched on. I don't know the specific code change that caused this to 
start happening (but it may have been the change for bug 5569).

Comment 10

[Stuart Parmenter]

fixed.

Comment 11

[Stuart Parmenter]

er, really fixed.

Comment 12

[John Morrison]

er, well it was fixed, but the last tinderbox cycle's page loading test showed
it timing out on the above pages (with the exception of Gamespot).

Comment 13

[John Morrison]

Er, make that with the exception of Gamespot, and Spinner.com, for two cycles.
Note also that the Yahoo.com page is timing out (which isn't part of the 
imglib2 change per se, since it had been around prior to that, but obviously
is responding to the same change).

Comment 14

[Stuart Parmenter]

just checked in another fix.. its better once again.

Comment 15

[Terri Preston]

Verified fixed w98 build 2001051704
Verified fixed mac build 2001051705
Verified fixed linux build 2001051706