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

RESOLVED WORKSFORME

Status

()

Core
ImageLib
P2
normal
RESOLVED WORKSFORME
15 years ago
9 years ago

People

(Reporter: Henry Jia, Unassigned)

Tracking

({testcase})

Trunk
mozilla1.5beta
x86
All
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

15 years ago
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.
(Reporter)

Comment 1

15 years ago
Sorry.

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

Comment 2

15 years ago
Created attachment 120802 [details]
test case ('unzip' it or uncompress it with 'winzip'...)

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

Comment 3

15 years ago
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
Severity: normal → critical
Keywords: crash, testcase
(Reporter)

Comment 4

15 years ago
No crash for the nightly build. But the gif graph keeps unchanged. Degrade the
severity back to normal.
Severity: critical → normal
Keywords: crash

Comment 5

15 years ago
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

15 years ago
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

15 years ago
Created attachment 120865 [details] [diff] [review]
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

15 years ago
Created attachment 120870 [details] [diff] [review]
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.
Attachment #120865 - Attachment is obsolete: true

Comment 9

15 years ago
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

15 years ago
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).
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

15 years ago
Created attachment 120887 [details] [diff] [review]
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
Attachment #120870 - Attachment is obsolete: true
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
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.
err, that should've been openRes in the current code

Comment 17

15 years ago
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

15 years ago
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
Attachment #120969 - Flags: review-
Created attachment 120986 [details] [diff] [review]
patch v2

addresses darin's comments but is untested.
Attachment #120969 - Attachment is obsolete: true
Comment on attachment 120986 [details] [diff] [review]
patch v2

I now tested this patch, and it works.
Attachment #120986 - Flags: superreview?(darin)
Attachment #120986 - Flags: review?(pavlov)

Comment 21

15 years ago
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
Attachment #120986 - Flags: superreview?(darin) → superreview+
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

15 years ago
Comment on attachment 120986 [details] [diff] [review]
patch v2

r=pavlov
Attachment #120986 - Flags: review?(pavlov) → review+
reassigning to me, as it's my patch
also changing os->all, happens on linux too
Assignee: jdunn → cbiesinger
OS: Windows 2000 → All
Priority: -- → P2
Target Milestone: --- → mozilla1.5alpha
Status: NEW → ASSIGNED
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.
Attachment #120865 - Attachment is obsolete: false
Attachment #120865 - Flags: review?(pavlov)

Comment 26

15 years ago
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

15 years ago
Created attachment 124556 [details] [diff] [review]
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?
Attachment #120865 - Attachment is obsolete: true
Attachment #120887 - Attachment is obsolete: true
jdunn: ah, that approach looks good.

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

Comment 29

15 years ago
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.

Updated

15 years ago
Attachment #124556 - Flags: superreview?(darin)
Attachment #124556 - Flags: review?(pavlov)

Comment 30

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

why NS_BINDING_ABORTED and not |rv| ?

Comment 31

15 years ago
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

15 years ago
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?
Attachment #124556 - Flags: superreview?(darin) → superreview-
Target Milestone: mozilla1.5alpha → mozilla1.5beta
-> default owner
Assignee: cbiesinger → jdunn
Status: ASSIGNED → NEW

Updated

15 years ago
Attachment #120865 - Flags: review?(pavlov)
Note that bug 281467 has a slightly better (imo) version of the "patch to stop
throbbing".
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

13 years ago
Is this fixed, then?

Updated

12 years ago
Attachment #124556 - Flags: review?(pavlov) → review-
Assignee: jdunn → nobody
QA Contact: tpreston → imagelib

Comment 37

9 years ago
No crash for me on Leopard using Firefox trunk.  The throbber never stops, though.  I filed bug 536637 for that.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.