202369 - crash or unchanged when refresh a page with gif files when these gif files removed

Status: RESOLVED WORKSFORME

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

Description

[Henry Jia]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030210
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030210

When refreshing a page with gif files in it and the gif fils are removed before
the refreshing, mozilla will crash or keep unchanged.

Reproducible: Always

Steps to Reproduce:
1.Display a page with some gif files in it
2.Delete or remove these gif files
3.Refresh the page

Actual Results:  
Mozilla show the page and indicate that these files are missing.

Expected Results:  
Mozilla 1.3b will crash.

My debug build will keep the gif unchanged.

Comment 1

[Henry Jia]

Sorry.

'Actual Results' part and 'Expected Results' part should be replaced with each
other.

Comment 2

[Henry Jia]

Attachment: test case ('unzip' it or uncompress it with 'winzip'...)

Here is the test case. Please uncompress it with 'winzip', 'unzip'...

Comment 3

[Adam Hauner]

Reporter, could you install the latest nightly build and try it again?
http://ftp.mozilla.org/pub/mozilla/nightly/latest/
If you still crash, please provide TalkBack ID.

WFM 2003041416/trunk/W2K

Comment 4

[Henry Jia]

No crash for the nightly build. But the gif graph keeps unchanged. Degrade the
severity back to normal.

Comment 5

[Jim Dunn]

today's build seems to handle this ok if the html & gifs are online.
If files are local, then then you remove the gif (testcase)
we still display it (I assume because it is cached).  If you put
this testcase online, display it, remove the gif, refresh, we display
the ALT text.

Not sure what the "policy" on local files is, but for online, this
problem is fixed.

Comment 6

[Darin Fisher]

if an image no longer exists, and the user presses reload, then we should show a
broken image icon (or the alt text).. even for local image files.

Comment 7

[Jim Dunn]

Attachment: partial fix... if we can't check LastModifiedTime, something is wrong

This is the first part of what is needed (working on the next part).
If GetLastModifiedTime returns failure, I think it is safe to say
that something is 'wrong' with the file and we should indicate
that it has aHasExpired so that later we must validate the file once again.

However, just doing this doesn't solve the problem... I am investigating now
why when we kick off a NewChannel we get the "image" rather than the
Alt text (my guess is that we are finding it in our disk cache and are
using that).

Comment 8

[Jim Dunn]

Attachment: updated patch

If LastModifiedTime returns "failure" then assume that
the file doesn't exist (aHasExpired==true) and doom
the cache entry so that we don't find this when we
re-request this.

Comment 9

[Darin Fisher]

hmm... what if you don't Doom the image cache entry?  my feeling is that it
should be OK to use the cache entry to satisfy back/forward or printing.  isn't
it sufficient to just return hasexpired = true?

Comment 10

[Jim Dunn]

Nope, I tried this at first, but we still "see" the image.
I set break points and found that we come back in later and
"get" this out of our cache.  So I tried removing it (doom)
and that did the trick.  (not saying that it is the right
solution, but one that gave me the correct results).

Comment 11

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

hmm. I am not sure if this is a good solution, I don't particularly like it...

The first patch looks better but as you said does not fix this bug...

I would look at the code around line 372 in imgLoader.cpp, the validating
stuff... maybe something is wrong there... however, the cache entry should be
doomed in imgCacheValidator, so maybe something is wrong there

Comment 12

[Jim Dunn]

Attachment: updated patch

SORRY for the confusion
I had done 2 changes at the same time... I was "dooming"
because I thought I should do the clean up, but as Chris
points out, not necessary.  The IMPORTANT change is the
return PR_FALSE

Comment 13

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

hmmm
something is strange here

      openRes = newChannel->AsyncOpen(NS_STATIC_CAST(nsIStreamListener *, hvc),
nsnull);
That line fails (imgLoader.cpp line 429)
(And LoadImage terminates then)

I then get:
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file
/home/chb/mozilla/content/base/src/nsImageLoadingContent.cpp, line 436

amazingly, the image still loads... still investigating

Comment 14

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

I found the real issue.

Something like this:
      if (NS_FAILED(rv))
        NS_RELEASE(*_retval);

Must be added near
http://lxr.mozilla.org/seamonkey/source/modules/libpr0n/src/imgLoader.cpp#428

I'll attach a patch to do that. This possibly requires either the first patch
here or the patch for bug 202475.

Comment 15

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

err, that should've been openRes in the current code

Comment 16

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

Attachment: patch

thoughts?

Comment 17

[Jim Dunn]

I just tested this patch (along with your other patch for bug 202475)
and it seems to do the trick.  I didn't go through the patch to fully
understand it (and have to run out now), but will.  Just wanted to
let you know it looks good so far (THANKS ALOT Chris)

Comment 18

[Darin Fisher]

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

>Index: modules/libpr0n/src/imgLoader.cpp

>+      CreateNewProxyForRequest(request, aLoadGroup, aObserver,
>+                               requestFlags, aRequest, _retval);
>+
>+

nit: bag extra newline

real problem: instead of killing "rv =" .. why not check the result
and return early on failure?  suppose we were out of memory.

also, the convention is usually to wait until the end of the function
to assign _retval.  that way this problem is easily avoided.

/darin

Comment 19

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

Attachment: patch v2

addresses darin's comments but is untested.

Comment 20

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

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

I now tested this patch, and it works.

Comment 21

[Darin Fisher]

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

>+      hvc->AddProxy(NS_STATIC_CAST(imgRequestProxy*,
>+                                   NS_STATIC_CAST(imgIRequest*, req.get())));

why is this cast necessary?  nsCOMPtr<imgIRequest> works, no?

sr=darin

Comment 22

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

If I only use .get() without an additional cast, I get errors about
nsDerivedSafe<something>* not being convertable to imgRequestProxy or something
like that... so it seems necessary.

Comment 23

[Stuart Parmenter]

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

r=pavlov

Comment 24

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

reassigning to me, as it's my patch
also changing os->all, happens on linux too

Comment 25

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

Comment on attachment 120865 [details] [diff] [review]
partial fix... if we can't check LastModifiedTime, something is wrong

I checked the latest patch on this bug in.

However; bug 202475 is not quite ready yet; so I'd like to get this patch (from
jdunn) checked in.

Comment 26

[Jim Dunn]

Using the 5/29 nightly build (most recent) this problem is now more of
un-stopped network requests.  Meaning, if you load the local test, remove
the image, hit reload; the throbber throbs, the cursor is the hourglass
and this just continues on and on.

Tracing, I found that in imgLoader.cpp we get into the case of
  if (request && bValidateRequest) {
    /* Case #2: the cache request cache must be revalidated. */

and when we       
rv = newChannel->AsyncOpen(NS_STATIC_CAST(nsIStreamListener *, hvc), nsnull);
which fails with rv = NS_ERROR_FILE_NOT_FOUND which is returned by
nsFileChannel::EnsureStream() from "rv = file->IsDirectory(&mIsDir);"

So it looks like we are in an unresolved state... we probably need to do
some cleanup if AsyncOpen fails.

Comment 27

[Jim Dunn]

Attachment: Followon patch to stop throbbing.

Looking at the case where we load a local image that isn't there
 /* Case #1: no request from the cache.  do a new load */
I saw that we hand back a request proxy object that has a canceled load
if AsyncOpen fails.  
I cut and pasted the relevant code and got this for a diff,  whatdoyouthink?

Comment 28

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

jdunn: ah, that approach looks good.

is attachment 120865 [details] [diff] [review] then not needed to fix this bug?

Comment 29

[Jim Dunn]

correct, the other patch isn't necessarily needed because in RevalidateEntry:
  if (aFlags & nsIRequest::VALIDATE_ALWAYS) {
    bValidateEntry = PR_TRUE;
  }
returns TRUE and so therefore we must revalidate.

However, we might want to add the change to imgCache.cpp for completeness,
it however, isn't needed.

Comment 30

[Darin Fisher]

Comment on attachment 124556 [details] [diff] [review]
Followon patch to stop throbbing.

why NS_BINDING_ABORTED and not |rv| ?

Comment 31

[Jim Dunn]

I cut and pasted this code from down below when AsyncOpen fails, 
I didn't use NS_BINDING_ABORTED specifically, it was in the cut code.

http://lxr.mozilla.org/seamonkey/source/modules/libpr0n/src/imgLoader.cpp#502

changing the code to
+        request->OnStopRequest(newChannel, nsnull, rv);
is fine

Comment 32

[Darin Fisher]

Comment on attachment 124556 [details] [diff] [review]
Followon patch to stop throbbing.

>+      if (NS_FAILED(rv)) {
>+        /* If AsyncOpen fails, then we want to hand back a request proxy
>+           object that has a canceled load.
>+         */

C++ style comments please ;-)


>+        request->OnStartRequest(newChannel, nsnull);
>+        request->OnStopRequest(newChannel, nsnull, NS_BINDING_ABORTED);

hmm.. these notifications usually arrive asynchronously via a PLEvent.
i'm not sure if it matters here, but it might put the caller of this
function, LoadImage, into a recursive state, which could cause other
crashes or at the very least some extremely fragile code.  better to
allocate a nsIRequestObserver proxy using NS_NewRequestObserverProxy
and call the start/stop methods on it instead.

but this begs the question... why isn't it ok to return with a failure
code at this point?  why do we need to go through the trouble of
delivering the failure through these notifications?  in other places
inside LoadImage we return failure codes... what's different about
this spot?

Comment 33

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

-> default owner

Comment 34

[Boris Zbarsky [:bzbarsky]]

Note that bug 281467 has a slightly better (imo) version of the "patch to stop
throbbing".

Comment 35

[Boris Zbarsky [:bzbarsky]]

The patch for bug 281467 finishes off what's left of this bug, I think.... 
Could someone please retest with tomorrow's builds?

Comment 36

[mmc]

Is this fixed, then?

Comment 37

[Jesse Ruderman]

No crash for me on Leopard using Firefox trunk.  The throbber never stops, though.  I filed bug 536637 for that.