142976 - OnStopRequest is not always fired if a storage transport fails.

Status: VERIFIED WORKSFORME

(Core :: Networking: Cache, defect)

Description

[rpotts (gone)]

Sometimes when a storage transport is canceled (or fails in some other way) the
OnStopRequest(...) is NOT fired.  This can cause (among other things) the
throbber to keep going...

Steps to reproduce:
  1. Disable disk cache
  2. Load a large image... such as:
http://www.danheller.com/images/Models/Andrea/x-files-big.jpg
  3. hit the BACK button
  4. hit the FORWARD button.

Notice how the throbber NEVER stops.

Comment 1

[rpotts (gone)]

i forgot to mention that after setting the disk cache to 0, the browser must be
restarted...

Comment 2

[rpotts (gone)]

Attachment: Patch to fire OnStopRequest if an error occurs ...

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Darin Fisher]

Comment on attachment 82796 [details] [diff] [review]
Patch to fire OnStopRequest if an error occurs ...

r/sr=darin (looks good!)

Comment 5

[rpotts (gone)]

this patch is important for HTTPS functionality, because HTTPS content is cached
in the memory cache...

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 82796 [details] [diff] [review]
Patch to fire OnStopRequest if an error occurs ...

r/sr=jst, looks good to me.

Comment 7

[rpotts (gone)]

patch checked into the trunk.

Comment 8

[Judson Valeski]

re: comment #5 . I don't see the memory cache/https implications here. Rick, can
you please ellaborate?

Comment 9

[Kathleen :Brade]

I'm probably missing something but this seemed odd to me:

659 if (NS_FAILED(rv)) {
660   Cancel(rv);
661 }
662 // avoid an infinite loop
663 else if (priorOffset == mTransferOffset) {
664   NS_WARNING("nsIStreamListener::OnDataAvailable implementation did not "
665              "consume any data!");
666   // end the read
667   (void) Cancel(NS_ERROR_UNEXPECTED);
668 }
669 
670 // post the next message...
671 return Process();


if there is a failure, we used to just return rv at line 660; now we call
Cancel() and then call Process().  Is Process aware of the cancellation?

Comment 10

[Darin Fisher]

brade: that OnDataAvailable implementation is proxied... so the return value is
always lost.  that's in fact part of the problem with the original code. 
instead of returning rv, it should have called Cancel before calling Process. 
otherwise, the failure would have gone unnoticed.  Process will see the error
and call OnStopRequest (since the transport has been canceled).

Comment 11

[rpotts (gone)]

hey jud,

the nsStorageTransport is used to pump data from the memory cache.  for, https
and (no-cache) content the memory cache is used instead of the disk cache -- to
avoid persistant storage...

Right now, https content is the main consumer of the memory cache.  So, this bug
is likely to occur on https content with more severe consequences than the 'full
page image' case.

The most likely result would be that the onLoad handler for a https page would
never fire because the page never 'finished'...  This would cause many problems :-)

-- rick

Comment 12

[Darin Fisher]

> and (no-cache) content the memory cache is used instead of the disk cache -- to

s/no-cache/no-store/

'cache-control: no-cache' only states that a useragent must revalidate the
response before reusing it.  'cache-control: no-store' states that the useragent
must not store the document in a persistent manner.

Comment 13

[Susie Wyshak]

added keywords with assumption that this fixed bug 111727

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 82796 [details] [diff] [review]
Patch to fire OnStopRequest if an error occurs ...

a=[e-mail] for 1.0branch checkin. Please change mozilla1.0.2+ to
fixed1.0.2 when checked in.  Please check in ASAP to make 1.0.2!

This is very important to embedders who don't use disk cache.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Until we upgrade to a milestone with this fix we'll be taking it on our local tree

Comment 16

[Susie Wyshak]

Do you all agree that this bug is what fixed Putnam Investments? Is there any
way we can verify this sooner rather than later so we'll know if it's a
different issue affecting Putnam's Individual Investor link? 

bug 111727

Comment 17

[Susie Wyshak]

Response to my last comment please?

Comment 18

[rpotts (gone)]

yes.  i've finally been able to verify that this patch fixes the loading
problems with Putnam Investments.

-- rick

Comment 19

[rpotts (gone)]

checked into the mozilla_1_0 branch:

Checking in nsStorageTransport.cpp;
/cvsroot/mozilla/netwerk/base/src/nsStorageTransport.cpp,v  <--  nsStorageTransp
ort.cpp
new revision: 1.13.2.3; previous revision: 1.13.2.2
done
Checking in nsStorageTransport.h;
/cvsroot/mozilla/netwerk/base/src/nsStorageTransport.h,v  <--  nsStorageTranspor
t.h
new revision: 1.6.14.2; previous revision: 1.6.14.1
done

Comment 20

[Jaime Rodriguez, Jr.]

posthumus edt1.0.2+. pls make sure the get DT approval (e.g. Starting tomorrow,
pls send mail to [e-mail] when requesting 1.0 branch approval), in
attention to Mozilla Driver's for checkin to the 1.0 branch. thanks!

Comment 21

[rpotts (gone)]

oops...

-- rick

Comment 22

[rpotts (gone)]

adding fixed1.0.2 keyword 

Comment 23

[Jaime Rodriguez, Jr.]

tever: can you pls verify this as fixed in tomorrow's branch builds, then
replace "fixed1.0.2" with "verified1.0.2". thanks!

Comment 24

[Tom Everingham]

verified 1.0.2 - throbber is now stopping in original testcase  - 09/18/02
branch -  winNT4, linux rh6, mac osX

Comment 25

[Tom Everingham]

Rick, did this patch make it into the trunk?  I am still seeing the throbber
running using the original steps.    

Builds tested - WinNT 2002091808, Mac 2002091808, Linux 2002091804

Comment 26

[rpotts (gone)]

hmmm... i just tried it against my trunk build and everything worked fine.  It
DOES NOT work in 7.0...

-- rick

Comment 27

[Susie Wyshak]

wfm with Gecko/20020923 commercial branch, win2K

(http://www.putnam.com --> individual investors loads as expected)

Comment 28

[Tom Everingham]

This is still happening on trunk builds pulled from sweetlou and using the
original test case from the first comment.  The cache does not need to be disabled.
 
Tested 2002092408 on win NT. 

Comment 29

[Michael Buckland]

discussed in edt:  rpotts, is comment 28 relevant to this bug or perhaps to some
other cause?

Comment 30

[Michael Dunn]

Does anyone no the state of this bug? Who should be the owner? Did a fix land on
the trunk?

Comment 31

[Michael Dunn]

--> Darin... since rick is gone.

Comment 32

[Michael Buckland]

Discussed in topembed bug triage.  Minusing.

Comment 33

[Darin Fisher]

fun... we don't even have a storage transport in the tree anymore.  i'm guessing
this is fixed.

Comment 34

[benc]

VERIFIED:
lxr says it is gone.

darin also has said that setting disk cache to zero or disabled is unsupported.
If there is some testcase that comes from this discussion, please let me know.

Comment 35

[gordon]

Setting the disk cache to zero to disable it is supported.  Setting the memory
cache to zero or disabling it, isn't recommended.  Things break without a memory
cache.

Comment 36

[benc]

V.