82435 - Need ability to force reload a specific image url from javascript

Status: VERIFIED FIXED

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

Description

[Charles Manske]

Comment 1

[Charles Manske]

Use Composer and click on Image button to bring up dialog. Click on "Choose
File" button. (If it isn't visible, Cancel dialog and bring it up again --
that's a known XPFE problem.)
Select a local image file -- it displays in preview box and its "natural width
and height" are reported next to the image.
Click on "Choose File" button again and select a different image. Again, the
image displays correctly and shows the width and height.
But if you select an image that you've already selected before,
The "onload" handler is called, but "naturalWidth" and "naturalHeight" are
returned as "0" for both.
So it seems that a cached image is failing to return its size.
Note that this happens for any image once loaded, thus another way of showing
the bug is to simply load a page with an image in it. Double click on that
image and it will be shown in the preview window, but the witdth and height
are not reported, because GetNaturalWidth() and GetNaturalHeight() have
returned 0.)
This might be related to bug 7019.

Comment 2

[Charles Manske]

Johnny: Given the number of bugs that pav has, do you think you can fix this?

Comment 3

[Stuart Parmenter]

bug 7019 is fixed (and has been since 0.9) so I don't think it is related.  If 
you can post a very simple testcase of this, I can probably get to it sooner.

Comment 4

[Johnny Stenback (:jst)]

Charles, I'm sorry but this one really is Pav's. This is a lowlevel problem in
the image loading code, the problem is that loading an image from cache ends up
fireing the onload event synchronously (from within nsImageFrame::LoadImage())
so mImageRequest in the image frame is null when the onload event fires, thus we
can't ask the image request for the size of the image.

Comment 5

[Charles Manske]

Right, I did some serious debugging today and found that out as well.
This sucks!

Comment 6

[Charles Manske]

A "simple test case" isn't going to make debugging any easy, imho.
Using the Image dialog's "Choose File" button as described in first bug
description is really the easiest way to show the problem.
Just break in imgLoader::LoadImageWithChannel() and at
nsImageFrame::GetNaturalImageSize() and you'll see what's going on.
After the first load, mImageRequest exists so we return the size info.
During the second load for same URL, GetNaturalImageSize() is called
synchronously as JST described, and mImageRequest null, so we can't get the size.

Comment 7

[Stuart Parmenter]

Please do not change the target milestones of bugs you don't own.

Comment 8

[Charles Manske]

Sorry! Because of dependency, I thought it was ok to do that. I won't any more.

Comment 9

[Charles Manske]

So the current problem is essentially the same as the request in bug 72922:
We need to be able to tell image loader to "ignore cache and always reload the
image."  If that problem is solved, so will this bug.
From the editor context, we only deal with nsHTMLImageElement, so the
problem is selectively the flags on the nsILoadGroup used to load the image.
Putting this code in nsImageFrame::LoadImage()
  PRUint32 flags = 0;
  loadGroup->GetLoadFlags(&flags);
  flags |= nsIRequest::LOAD_BYPASS_CACHE;
  loadGroup->SetLoadFlags(flags);
just before
  return il->LoadImage(uri, loadGroup, mListener, aPresContext, aRequest);
fixes the problem (and bug 72922) by always forcing a new image load.
I see two possible solutions to make this happen only for Editor image loads:
1. During parsing of tags for editor, put a "-moz-bypass-cache" attribute on
each <img> tag ("-moz..." attributes are always stripped out when writing
output). This attribute would be checked in nsHTMLImageElement to set a
class member flag, and this would have to be passed as a new parameter to
nsImageFrame::LoadImage().
2. Another solution might be to detect when we are in an editor (I think we
can do that from nsIPresContext) and always set the
nsIRequest::LOAD_BYPASS_CACHE flag when imgLoader::LoadImage is called.
That is probably easier, but wouln't that also ignore cache for UI images and
other images we use that aren't part of the page content?
I'm no expert in Image code, and I'm sure there's probably a better solution,
but I hope this stimulates finding it!

Comment 10

[Charles Manske]

Note that using a "-moz-bypass-cache" attribute is a good idea only for *locale*
images. You probably wouldn't want that if SRC URI is a remote image!
This argues against ignoring cache for editor in general.

Comment 11

[Charles Manske]

Attachment: Use special attribute on image tag to ignore cache when changing image SRC

Comment 12

[Charles Manske]

Why I think the suggested fix in 5/26/01 patch is the correct solution:
1. The  "_moz-bypass-cache" attribute is set true only on the <img> tag in the
actual dialog. We do not have to do this on the img tags in the document, as I
suggested in original proposal. (Patch for changes to EdImageProps.xul to use
this attribute is attached to bug 78351.)
2. Normal image loading in Browser and Composer is not affected; the user's
image cache settings in prefs still control normal image loading. The test for
the "_moz-bypass-cache" attribute only happens in Composer Image dialog and in
DHTML uses where the SRC attribute is changed after initial page loading.
(In-house DHTML users might also find this attribute useful for forcing image
reloading.)
3. This also fixes bug 72922, a very annoying problem in Composer that prevents
users from seeing any changes they make to images while editing a page
that includes them (must delete cache, close program, restart!)
4. Fix is very simple and restricted to just nsImageFrame; nsHTMLImageElement
and cache code do not have to be changed.

The attached patch also contains JST's fix for bug 76177, which is required
as part of this fix. The removal of "mNaturalImageWidth" and
"mNaturalImageHeight" from nsImageFrame.h is cleanup -- they were not used
and should not be (we shouldn't store any size information in the frame
now that the SRC can be changed.)

Comment 13

[Johnny Stenback (:jst)]

I *really* dislike the idea of adding a _moz-bypass-cache attribute only to hack
around a real problem with imglib, which we can fix or get around with less
trouble, I'm sure. If we don't wanto fix imglib to not call its notification
methods synchronously when loading an image from the cache then we can fix this
in the image frame by setting the image request pointer in the notification
callbacks if we're called synchronously (which one can tell by checking if
mImageRequest == null in the notification callbacks). There are issues to be
sorted out here (such as potential leaks from poking a new value into a nsCOMPtr
when there's a getter_AddRefs() on the stack for the same nsCOMPtr), but nothing
too complicated, let's fix this the right way in stead of adding editor specific
hacks to the code (which only fixes this problem in the editor case, the same
problem can be demonstrated in web content as well).

Comment 14

[Charles Manske]

I'd be much happy as well if we could fix the basic imglib problems, but that
still wouldn't fix 72922, would it? We would still need some way to ask for
an image load that bypasses the cache, right?

Comment 15

[Johnny Stenback (:jst)]

You're right, this wouldn't fix bug 72922 but I'm sure there's a better fix for
that as well. See bug 72922 for comments.

Comment 16

[Johnny Stenback (:jst)]

Rick, this is the bug about loading images from the cache ends up calling the
notification callbacks synchronously.

Comment 17

[Charles Manske]

If the ImgLib work required as suggested by JST isn't done,
then I suggest we use the "_moz-bypass-cache" strategy at least for
an interim fix.

Comment 18

[Stuart Parmenter]

you could set mImageRequest in OnStartDecode, however, i'm not sure how you 
could tell if this is the low source url or not.  perhaps it doesn't matter.

Comment 19

[Charles Manske]

I'm sure it does matter. The "natural" width and height are usually different
for the lowsrc. We need to get the final image's size.

Comment 20

[Stuart Parmenter]

Attachment: patch to add a method on imgICache to remove a URI

Comment 21

[Charles Manske]

r=cmanske
Tested and it works great.

Comment 22

[Johnny Stenback (:jst)]

Pavlov, why this change in imgCache.cpp:

@@ -197,9 +209,9 @@
 PRBool imgCache::Remove(nsIURI *aKey)
 {
   LOG_STATIC_FUNC(gImgLog, "imgCache::Remove");
+  if (!aKey) return PR_FALSE;
 
   nsresult rv;

The code that's called later on in this method doesn't properly handle a null
URI, so why remove the null check?

Comment 23

[Stuart Parmenter]

huh?  i added the check for null there.  the reason i don't check for null in 
the other function is that the other callers of it all have null checks in other 
places.

Comment 24

[Johnny Stenback (:jst)]

Eh, huh, duh! Stupid me, geez, man, how lame is that, I can't even read diff's
any more!

sr=jst (which obviously doesn't mean much any more ;-)

(feeling stupid)

Comment 25

[tor]

a=tor for attachment 38322 [details] [diff] [review] checkin to trunk

Comment 26

[Stuart Parmenter]

marking fixed.

Comment 27

[Terri Preston]

Verified fixed w2k build 2001062004