135833 - [FIX]Lack of warning when unable to view the page source (renders page instead of source)

Status: RESOLVED FIXED

(Core Graveyard :: View Source, defect, P1)

Description

[Robert Jirik]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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

Comment 2

[Robert Jirik]

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 ...

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Nathanael C. Nerode]

>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?

Comment 5

[Boris Zbarsky [:bzbarsky]]

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....

Comment 6

[Boris Zbarsky [:bzbarsky]]

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?

Comment 7

[Darin Fisher]

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?

Comment 8

[Boris Zbarsky [:bzbarsky]]

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...

Comment 9

[Darin Fisher]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 11

[Darin Fisher]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed 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 13

[Darin Fisher]

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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch v1.1

Comment 16

[rpotts (gone)]

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 17

[rpotts (gone)]

Comment on attachment 78832 [details] [diff] [review]
Patch v1.1

r=[e-mail]

Comment 18

[rpotts (gone)]

Comment on attachment 78832 [details] [diff] [review]
Patch v1.1

r=[e-mail]

Comment 19

[rpotts (gone)]

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

Comment 20

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 21

[Boris Zbarsky [:bzbarsky]]

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.

Comment 22

[rpotts (gone)]

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

Comment 23

[Boris Zbarsky [:bzbarsky]]

> 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).

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch v1.2

This just adds the comments that Rick asked for

Comment 25

[Darin Fisher]

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

sr=darin

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch v1.3

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 27

[Darin Fisher]

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 28

[Simon Montagu :smontagu]

Comment on attachment 78924 [details] [diff] [review]
Patch v1.3

Confirming that this patch builds in Win32

Comment 29

[Boris Zbarsky [:bzbarsky]]

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.

Comment 30

[Lorenzo Colitti]

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?

Comment 31

[Boris Zbarsky [:bzbarsky]]

> 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).

Comment 32

[Boris Zbarsky [:bzbarsky]]

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

Comment 33

[Boris Zbarsky [:bzbarsky]]

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

Comment 34

[Boris Zbarsky [:bzbarsky]]

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

Comment 35

[Boris Zbarsky [:bzbarsky]]

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

Comment 36

[Boris Zbarsky [:bzbarsky]]

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

Comment 37

[Gilles Durys]

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

Comment 38

[Boris Zbarsky [:bzbarsky]]

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

Comment 39

[Boris Zbarsky [:bzbarsky]]

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