Mozilla doesn't warn when memory and disk cache is set to zero, and you try to view source. Instead, it asks you, whether you want to repost data and then displays in viewsource window the page just like as it is rendered in browser. This affects just pages generated using POST method; others are displayed properly. The browser should warn, that it is unable to display it, or it should keep the last source in some `special` cache - to be able to display even if both (d&m) caches are set to 0 kB. The page it could be tested on is http://xhaven.net/bugtest.php, follow the instructions there Win2k, 2002-04-05-03

The real bug here is that after we repost we display the page instead of the source. Taking this; I'll check it out.

I've tested this more, and it seems, that memory cache has no effect - the disk cache must be on. With my settings of 1024kB mem. and 0 disk, the problem still was there ... 0 mem and 1024 disk - everything fine ...

Re comment #2 -- HTML is not stored in memory cache right now, only in disk cache. I'll be looking into this bug when I get back to my tree in 6 days. I have a fairly good idea of where things fail, but I want to verify it and can't do it from where I am right now. So if everyone just holds on to their hats for a week, I'll post what I find here.

>Re comment #2 -- HTML is not stored in memory cache right now, only in disk > cache. eeek. That's obviously a problem... html *should* be stored in the memory cache so that 'no-cache' pages can have proper behavior for things like 'view-source' (the infamous bug 40867). Should I file a separate bug regarding that, or is it being taken care of?

That's handled in the various bugs on having a true multi-tier cache, iirc. no-store documents _are_ stored in memory cache, however. It's only regular non-https non-no-store HTML documents that go in disk cache. The real issue is not where the things are stored by default bit the fact that once you say where you prefer to store it the cache fails to store it completely if there is no space there insteaf of falling back on a different cache device (which is what the multi-tier bugs are about). I'd provode bug numbers, but I can't find them....

OK. What happens here is the following: 1) nsHttpChannel::Connect fails because it can't open the cache entry and returns NS_ERROR_DOCUMENT_NOT_CACHED 2) nsHttpChannel::AsyncAbort(NS_ERROR_DOCUMENT_NOT_CACHED) is called 3) The HTTP channel is removed from its loadgroup. Since it's a foreground channel, this triggers a call to nsDocLoaderImpl::OnStopRequest (this is called from nsLoadGroup::RemoveRequest). 4) The loadgroup has an active count of 0 at this point. So we call nsDocLoaderImpl::doStopDocumentLoad which fires a state change notification to nsDocShell::OnStateChange, which calls nsWebShell::EndPageLoad Now steps 3 and 4 are passing around the HTTP channel as the nsIRequest, not the viewsource channel. So the HTTP channel is the one EndPageLoad sees. EndPageLoad is where we pose that post data prompt and reload the url from the nsIRequest arg if the user says to resend the postdata. So EndPageLoad ends up getting the url from the HTTP channel (which does not have the "view-source:" on the front) and loading it in the docshell. Possible solutions: 1) Add the nsViewSourceChannel to the same loadgroup as the nsHttpChannel it opens (not sure why that's not happening to start with). 2) Move that prompt to a slightly more sane place than EndPageLoad (like into nsHttpChannel, if possible) 3) Something I'm not thinking of. Darin? Thoughts?

hmm, the view-source channel should try to hide the details of the http channel from the rest of gecko as much as possible. it'd seem that view-source should add itself to the load group instead of the http channel, and the view-source channel should listen to errors from the http channel and deal with the errors in OnStopRequest. what do you think? is there any hope of cleaning this up?

Aha! I didn't realize that the HTTP channel adds itself to the loadgroup in AsyncOpen. It seems like what Darin says should be doable...

yeah, the unspoken truth about AsyncOpen is that it should return with the channel added to the loadgroup (unless it throws an exception).

> it'd seem that view-source should add itself to the load group instead of the > http channel I've been thinking about that... The problem is that the http channel looks for things like auth prompts and the like on the loadgroup. So not setting its loadgroup may require quite a bit of work (though it would be doable). An alternate approach may be: 1) Have the view source channel add itself to the loadgroup 2) Make sure that the child channel's loadflags do not include nsIChannel::LOAD_DOCUMENT_URI 3) remove the view source channel from the loadgroup in nsViewSourceChannel::OnStopRequest. I have this implemented in my tree at the moment and it works great as far as I can tell.... Is this a valid way to do things? Or should the child channel just not be added to the loadgroup at all?

yeah, you make a good point. if we don't add the child channel to the load group then we'd need to explicitly get the notification callbacks from the load group and set them on the channel if the channel doesn't already have notification callbacks... or rather... we'd need to make a special implementation of nsIInterfaceRequestor that returned the union of the two. yuck! i say if you have a working (simple) solution that doesn't feel like a hack, then it's probably the way to go ;-) unfortunately i don't know enough about how view-source hooks into the rest of mozilla. you should make sure rpotts reviews your patch.

Implements the suggestion from comment #10. I'm not sure whether it feels like a hack, basically. How does it look to you?

Comment on attachment 78799 [details] [diff] [review] Proposed patch Index: netwerk/protocol/viewsource/src/nsViewSourceChannel.cpp >+ /* XXX Gross hack -- NS_NewURI goes into an infinite loop on >+ non-flat specs. See bug 136980 */ >+ return NS_NewURI(aURI, nsCAutoString(NS_LITERAL_CSTRING("view-source:")+spec), nsnull); SetSpec currently calls PromiseFlatCString anyways :-/ >+ if (loadGroup) >+ loadGroup->AddRequest(NS_STATIC_CAST(nsIViewSourceChannel*, >+ this), nsnull); >+ > return mChannel->AsyncOpen(this, ctxt); need to check for NS_FAILED(rv)... in which case you should remove yourself from the load group. another option would be to only add yourself to the load group if AsyncOpen succeeds. >+ // This should actually be just nsIChannel::LOAD_DOCUMENT_URI but >+ // the win32 compiler fails to deal... tries to do a method lookup >+ if (mIsDocument) >+ *aLoadFlags |= ::nsIChannel::LOAD_DOCUMENT_URI; doesn't plain old LOAD_DOCUMENT_URI work? >+ // These should actually be just nsIRequest::LOAD_FROM_CACHE and >+ // nsIChannel::LOAD_DOCUMENT_URI but the win32 compiler fails to >+ // deal... tries to do a method lookup >+ mIsDocument = (aLoadFlags & ::nsIChannel::LOAD_DOCUMENT_URI) ? PR_TRUE : PR_FALSE; same here... you usually shouldn't need the nsIChannel:: prefix if you are a nsIChannel implementation. otherwise, this looks good to me. sr=darin with the suggested changes.

> SetSpec currently calls PromiseFlatCString anyways :-/ Um... the problem happens in ExtractURLScheme(), not in SetSpec. See the stack trace in bug 136980. That calls PromiseFlatCString too, but then it uses the non-flat string when it calls Substring() on it, and that's the part that hangs. I considered changing ExtractURLScheme() in this patch, but wasn't sure that was needed, assuming bug 136980 gets fixed. > doesn't plain old LOAD_DOCUMENT_URI work? Not sure... It works on Linux, but I don't have a Win32 compiler to test on. I'll try to corner someone with a Win32 compiler and get them to test. I'll post an updated patch later tonight.

hey boris, this looks good to me... it's kinda tricky, so you might want to add some more comments explaining why it's necessary for the view-source channel to maintain the state of LOAD_DOCUMENT_URI flag while the rest of the load flags are kept in the underlying http channel... I was thinking of something along the lines of 'because the view-source channel is the first request that is added to the load group, it becomes the DOCUMENT_URI. Therefore it is necessary to keep this flag distinct from those on the underlying channel... Otherwise, it would look like there were TWO document URIs :-)...' Feel free to elaborate or make a more coherent summary ;-) I was just thinking that another possible approach would be to add the underlying channel first (so it becomes the DOCUMENT_URI) and then add the view-source channel after the call to AsyncOpen()... WIth this approach, you'll need to keep a flag indicating whether the listener's OnSTopRequest() was called... and only add the view-source channel, if this flag is FALSE (ie. OnSTopRequest has not been called yet)... This flag prevents the view-source channel from being added *after* the entire transaction as completed... this strategy doesn't save any storage (because you'll still need a flag) but it might make the code a bit clearer... it's your call... -- rick

Comment on attachment 78832 [details] [diff] [review] Patch v1.1 r=rpotts@netscape.com

Comment on attachment 78832 [details] [diff] [review] Patch v1.1 r=rpotts@netscape.com

doh!!! i got bitten by the double submit bug in 6.2 ;-)

> I was just thinking that another possible approach would be to add the > underlying channel first (so it becomes the DOCUMENT_URI) and then add the > view-source channel after the call to AsyncOpen() I tried that first, actually. :) It triggers an assertion in nsDocLoaderImpl::OnStartRequest (at http://lxr.mozilla.org/seamonkey/source/uriloader/base/nsDocLoader.cpp#471). I figured that that made it a bad idea...

Oh, and I'll post an updated patch with a coherent comment tomorrow; I'm not sure I can write a coherent one till I get some sleep.

i wish i understood our codebase ;-) i can't imagine what the DocLoader thinks the 'mDocumentRequest' is when a view-source: channel is first opened!! the view-source channel should *always* represent a 'top level' document load... so the associated docLoader 'should' be idle when aViewSourceChannel->AsyncOpen(...) is called. of course, it could be that it's just too late and i'm forgetting something very simple ;-) -- rick

> i can't imagine what the DocLoader thinks the 'mDocumentRequest' is when a > view-source: channel is first opened!! Nothing, at that point. But when we AsyncOpen the HTTP channel, it adds itself to the loadgroup, which fires an OnStartRequest on the docloader, which sees that it's a LOAD_DOCUMENT_URI and sets it as 'mDocumentRequest'. If we add the view-source channel after the HTTP channel, and they both have LOAD_DOCUMENT_URI set then the DocLoader attempts to set the viewsource channel as mDocumentRequest but finds that's set already, etc. I think twiddling the LOAD_DOCUMENT_URI flag is simpler than keeping track of the async issues like whether OnStopRequest got fired and what to do if OnStopRequest is fired _before_ we've managed to add ourselves to the loadgroup (I think that scenario will cause this bug again).

This just adds the comments that Rick asked for

Comment on attachment 78915 [details] [diff] [review] Patch v1.2 sr=darin

This is identical to 1.2, except I got a windows person to build it and the Win32 compiler barfs on LOAD_DOCUMENT_URI and LOAD_FROM_CACHE, since the inheritance of nsViewSourceChannel from nsIChannel and nsIRequest is ambiguous... So this patch goes back to using the fully qualified names (and adds a comment explaining why).

Comment on attachment 78924 [details] [diff] [review] Patch v1.3 sr=darin makes sense... i didn't realize there was multiple inheritance from nsIChannel/nsIRequest going on here :(

Comment on attachment 78924 [details] [diff] [review] Patch v1.3 Confirming that this patch builds in Win32

Checked in on the trunk. I'll ask for driver approval for the branch in a day or two once I can make a good case for lack of regressions.

Boris, regarding comment #3 and comment #5, it seems silly not to use the memory cache at all when loading regular html. This obviously breaks view-source if the disk cache is set to zero, even if there's a memory cache. I have tried to find bugs on the subject but I can't seem to find any. Does your patch to this bug address the problem? Or should I file a new bug?

> it seems silly not to use the memory cache at all when loading regular html Well... sure. But the cache does not support falling back to a different cache device. So when you set up a cache entry you have to say where you want to store it... Again, I'm fairly certain there's already a bug on the cache supporting multi-tier storage. If you can't find one, though, please do file one. This patch does nothing about the cache, by the way. It just fixes the "repost" case. Oh, and drivers don't feel this is safe enough to take on the branch. So marking fixed (it's fixed on trunk).

resummarizing to make this findable. Old summary: "[FIX]Lack of warning when unable to view the page source"

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

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

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

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

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

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

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