137819 - Changed images don't reload [file:// URLs]

Status: RESOLVED FIXED

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

Description

[Theo Brinkman]

When a page is visited and an image linked to that page is changed, the image is
not refreshed when the reload button is pressed.  To see the altered image, you
must clear the browser's cache.  This happens even when 'Compare the page in the
cache to the page on the network' is set to 'Every time I view the page'.

To verify this:
1 Throw together a page containing an image.
2 View the page.
3 Edit/save the page.
4 Refresh.  (Note: The current page is displayed.)
5 Edit/save the image.
6 Refresh.  (Note: The old image is displayed.)
7 Clear your cache.
8 Refresh.  (Note: the new image is displayed.)

Comment 1

[Matthias Versen [:Matti]]

-> Cache

Have you tried shift+reload ?

Comment 2

[Theo Brinkman]

Ok.  Shift+Reload displays the new image.

If you don't mind my asking, why is this required?

Comment 3

[R.K.Aa.]

Shift+reload reloads from the net. Reload alone reloads from cache.
I really wish it was the other way around: I never need to reload from cache.
But this is how it's designed to work.

Resolving as invalid.

Comment 4

[Theo Brinkman]

I'd accept that if it behaved the same way for the page as it does for the
image.  Shouldn't images, etc. be handled the same way the page is (i.e.: check
to see if the image has changed under the same circumstances the page is checked
for the same?)  Short of doing that, it's inconsistant behavior at best, a
fairly major cache bug at worst.

Comment 5

[R.K.Aa.]

Reopening.

Comment 6

[Shane Caraveo]

I'm going to throw in my two bits on this... I think this is a critical bug,
it's going to break some sites for moz.  I use a technique that is common, a php
script to dynamicly choose an image:

img src='/path/to/image.php'

It returns a number of headers pleading various browser versions/http
versions/whatever to not cache the image returned, sets the Content-Type to
image/gif, and returns the image data.

Mozilla 1.0 RC1 caches the first image.  If I access '/path/to/image.php'
directly , and shift-reload, the image changes, but if I then simply hit reload,
the image reverts to the first image again (no I am not resending the first
image).  If I access a page that has an img tag with the src path to the php
script, shift-reload will not even reload a new image.

No amount of emptying cache, changing cache prefs, etc. changes the behaviour.

The script works fine with all non-moz browsers.

An example script is below.
Shane

------ image.php -------
// place a couple gifs in the same directory as this test file.
// start.gif will only be shown the first time the script is accessed
// mozilla 1.0 RC1 will always show start.gif, unless shift-reload
// do shift-reload then another reload, and moz returns to start.gif
if (array_key_exists('done',$_COOKIE)) {
    $image = 'done.gif';
} else {
    setcookie('done','1');
    $image = 'start.gif';
}
$basepath = dirname($_SERVER["PATH_TRANSLATED"]);
header("Expires: Mon, 26 Jul 1997 05:00:00 GMT");    // Date in the past
header("Last-Modified: " . gmdate("D, d M Y H:i:s") . " GMT"); 
                                                     // always modified
header("Cache-Control: no-store, no-cache, must-revalidate");  // HTTP/1.1
header("Cache-Control: post-check=0, pre-check=0", false);
header("Pragma: no-cache");                          // HTTP/1.0
header('Content-Type: image/gif');
readfile($basepath.'/'.$image);

Comment 7

[Daniel van Vugt]

This is a problem on Linux too (up to 1.0rc1 2002041711 that I'm using now) and
has been for as long as I remember.
I would officially confirm the bug if I had permission :)

Comment 8

[a_geek]

I would like to confirm that this bug also applies to a file: URL.
Just generated a series of analog statistics and loaded them one
after another while the images were always the same. I forgot to
shift-reload, but cleared the cache instead which made new images
appear...

I'd like to see page and images in sync, regardless which way this
behaviour will be defined, but having them work differently breaks
page consistency. This probably needs to be extended to cover
scripts and similar stuff, too.

Comment 9

[a_geek]

*** This bug has been confirmed by popular vote. ***

Comment 10

[Theo Brinkman]

Any chance at all that this will get fixed before Moz 1.0?

I agree with Shane's 'critical' assesment of this bug now, may even be a blocker
but I don't know the criteria for going that high.  I actually ran into a
problem with this 'in the wild' over the weekend.  Some sites use a security
mechanism of dynamically generated images for a code the user has to manually
enter into a text box.  They do this to make it more difficult for a bot to
influence results.  I don't remember where I was at the time though, so I can't
include a url for testing.  Sorry.

Also, if it's a bug on Windows AND Linux, I suspect it's a bug on ALL platforms.

Comment 11

[gordon]

Changing component to HTTP as it involves HTTP reload policy.

Comment 12

[gordon]

->darin

Comment 13

[Darin Fisher]

reporter, anyone: please provide a testcase.  also, this bug seems really old. 
i suspect this has been fixed since we've not heard of any other reports of this
kind of thing.  such a major regression would have generated a lot of noise.

WORKSFORME please reopen if you can repro this bug.

Comment 14

[Theo Brinkman]

Not fixed.  A static test-case for this can't be made because it involves 
handling of dynamic content.

The process for reproducing this bug is outlined in the description.  If you 
have a dynamic page which uses the same url string to put an image on the page 
the image will be reloaded from the browser's cache even if the image on the 
server has changed since it was originally loaded.  As an example, I'll go 
through the process once more.

1. Create an image (untitled.gif).
2. Create a page (test.html) containing the following:
- - - - -
<html>
	<head>
		<title>Image Reload Testcase</title>
	</head>
	<body>
		Sample Text Before Image Change.<br />
		<img src="./untitled.gif"/>
	</body>
<html>
- - - - -
3. View the page.
4. Change the page contents to:
- - - - -
<html>
	<head>
		<title>Image Reload Testcase</title>
	</head>
	<body>
		Sample Text AFTER Image Change.<br />
		<img src="./untitled.gif"/>
	</body>
<html>
- - - - -
5. Edit the image in some easily visible way.  (change its color or something)
6. Refresh the page, and notice that the text in the page has changed, but 
you're still seeing the old image.  Both the html page and the image have 
changed on disk since the last time the page was loaded, but only the html is 
refreshed, the image is still pulled from the browser's cache.

Internet Explorer handles this case properly.  The image has changed since the 
last time the page was loaded, so it loads the new image.

Comment 15

[Darin Fisher]

WORKSFORME linux build 2002100808.  i used apache 1.3.23 as the server.  i
created html file and image as described.  i loaded the page once.  modified the
html and the image.  and then reloaded the page (using the reload button).  it
worked as expected.

reporter: how are you reloading the page?  are you perhaps using file:// URLs?

Comment 16

[Theo Brinkman]

Yes I am currently using a 'file://' url, but when I first discovered this bug 
I was using a dynamic image generated as part of a PHP script.

Why should using a 'file://' url matter?  If it reloads the page, it should 
reload the image as well.

Comment 17

[Darin Fisher]

it matters because the validation process is different for http:// and file://.
 in http:// we might send a conditional request to the server (such as
If-Modified-Since), whereas for file:// there is no equivalent request.  my fear
is that imagelib might be caching file:// URLs for a fixed amount of time.  it
might not be properly checking the file modification time.

reopening for investigation.

Comment 18

[Darin Fisher]

reporter: does shift-reload help?  also, when you used http:// URLs, were you
connecting via a HTTP proxy?  if so, what is the proxy type/version?  thx!

Comment 19

[Darin Fisher]

ok, i'm able to repro this bug when using file:// URLs to load the page.

-> imagelib component

downgrading severity.  i'll investigate this for mozilla 1.3.

Comment 20

[gordon]

Wow!  If it happens even with file:// urls, Imagelib must be incorrectly pulling
it out of its cache session.

How can we unify some of the caching policies between various cache clients.

Comment 21

[Darin Fisher]

gordon: iirc, imagelib makes up an expiration time for file:// URLs.  like maybe
10seconds, or 10minutes.  it really needs to stat the files to determine if they
have changed.

Comment 22

[gordon]

Okay, but how about only once per pageload.  It could be painful if you have a
small image repeated many times on a page.

Comment 23

[Darin Fisher]

true, and i would only stat in the reload case.

Comment 24

[gordon]

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

Comment 25

[gordon]

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

Comment 26

[gordon]

Darin, it looks like bug 155275 is a dup of this one.  Do you concur?

Bug 167514 also seems somewhat similar, but it isn't file: specific, so I'm
inclined to leave it as a separate bug.

Comment 27

[gordon]

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

Comment 28

[Darin Fisher]

yes, bug 167514 seems different.  for that one we should check the load flags
for the image load.

Comment 29

[Matthias Versen [:Matti]]

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

Comment 30

[Matthias Versen [:Matti]]

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

Comment 31

[benc]

-> imagelib qa

Comment 32

[Christian :Biesinger (don't email me, ping me on IRC)]

taking bug

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

ok now this works.

however, I do not know if this is the right way to fix this bug. darin, stuart,
please tell me.

I will attach a diff -uw version of the patch in a second.

Comment 34

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: diff -uw

Comment 35

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 107876 [details] [diff] [review]
patch

Here's my hoping that stuart does read request mails...

Comment 36

[Darin Fisher]

Comment on attachment 107876 [details] [diff] [review]
patch

biesi: the patch looks basically ok, but can you please attach a diff with
whitespace changes removed? 
also, how about taking this opportunity to rename httpValidateChecker to
something much more appropriate, like imgCacheValidator (or anything else not
specific to http).

Comment 37

[Christian :Biesinger (don't email me, ping me on IRC)]

darin: http://bugzilla.mozilla.org/attachment.cgi?id=107877&action=view is the
-uw version of the patch

sure, I can rename httpValidateChecker to imgCacheValidator, fine by me.

Comment 38

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 39

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 107876 [details] [diff] [review]
patch

clearing requests because I'll attach a new patch in a second

Comment 40

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

as above, but s/httpValidateChecker/imgCacheValidator/g in the libpr0n src dir

Comment 41

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2 -uw

Comment 42

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 107936 [details] [diff] [review]
patch v2

see the next attachment for the patch without whitespace changes.

Comment 43

[Darin Fisher]

Comment on attachment 107937 [details] [diff] [review]
patch v2 -uw

>Index: imgCache.cpp

>+          if (SecondsFromPRTime((PRTime)fileLastMod) > lastModTime)
>+            *aHasExpired = PR_TRUE;
>+          else
>+            *aHasExpired = PR_FALSE;

no need for an extra branch here, just do this:

  *aHasExpired = SecondsFromPRTime((PRTime)fileLastMod) > lastModTime;


seems fine otherwise, sr=darin (assuming you've thoroughly tested this...
e.g., make sure it doesn't negatively impact chrome).

Comment 44

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 107936 [details] [diff] [review]
patch v2

marking darin's sr on the non-uw patch

Comment 45

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 107936 [details] [diff] [review]
patch v2

ok, giving up on waiting for review from stuart

tor: So, could you review this patch?
It does several things:
firstly, if an entry is put into the cache, and the scheme is file:, it sets
the metadata attribute to revalidate when the entry is expired.

later, in the check if an entry has expired, it checks the last modified time
to find out if the entry has expired.

thirdly, if the request must be validated, and if a protocol does not impl.
nsICachingChannel, the entry is reloaded, instead of the cached version being
used.

finally, I made httpValidateChecker cope with non-caching channels. ah yeah,
and I renamed it to imgCacheValidator.


I still have to test if this affects chrome performance, but I don't think it
should, because this is mostly file:-specific.

Comment 46

[tor]

Comment on attachment 107936 [details] [diff] [review]
patch v2

sr=tor

Remember darin's change.

Tinderbox appears bored at the moment - liven it up with a Ts/Txul test. :)

Comment 47

[Christian :Biesinger (don't email me, ping me on IRC)]

ok, checked in (with darin's change)

Comment 48

[Christian :Biesinger (don't email me, ping me on IRC)]

comet's Txul and Ts times:
before my checkin:
Txul:467ms
Ts:1464ms

after my checkin:
Txul:465ms
Ts:1462ms

-> no measurable impact.

Comment 49

[Matthias Versen [:Matti]]

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

Comment 50

[R.K.Aa.]

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

Comment 51

[Felix Miata]

Looks fixed in OS/2 trunk 2003021213.

Comment 52

[Kai Lahmann (is there, where MNG is)]

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

Comment 53

[Matthias Versen [:Matti]]

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