72679 - nsDocumentOpenInfo dispatches content too early

Status: RESOLVED DUPLICATE of bug 106558

(Core :: Networking, defect)

Description

[Doug Turner (:dougt)]

In nsDocumentOpenInfo::OnStartRequest, we call DispatchContent(), and forward
the onStartRequest to the targeted listener.  This works for most of the cases.  

However, most protocols have a state where the onStart has been fired, no data
has been received and an error occurs.  (these three conditions happen quite
frequently in the ftp protocol since there is a sometime a long delay between
the onStartRequest fired by the data connection and the OnDataAvaiable which is
caused by a command on the control connection).  In this case, the
OnStartRequest creates a content viewer which destroys the original content of
the page.  The protocol realizes that there is an error, an issues an
onStopRequest.  The content viewer complete its pageload.  The end result is
that an error in the protocol buildup replaces the original content of a page.

Now, if we only set a flag in nsDocumentOpenInfo::OnStartRequest.  Then in
nsDocumentOpenInfo::OnDataAvaiable, checked this flag and fired the
OnStartRequest, I believe that this problem would go away. Not to mention that
the special http 202 case could probably be removed.

Also, as Vidur suggested, this may improve perceived performance since the
content area will not be disposed until there is data available.


Comments?  Suggestions?

Comment 1

[Blake Ross]

I get into the state that doug described often, and I think this is also what 
pink mentioned in another bug yesterday...

Comment 2

[Scott MacGregor]

Hmmm your proposal seems okay on the surface.....I can't think of it breaking
something off the top of my head so there's only one way to find out and that's
to try it =).

Comment 3

[Doug Turner (:dougt)]

Attachment: Diff to delay firing of onStartRequest until data shows up.

Comment 4

[Doug Turner (:dougt)]

I am not sure that this really does anything.  On some pages it appears to
reduce the delay between page transitions (eg. the blank page), but I am not
sure that the attached diff is the cause.

Comment 5

[Doug Turner (:dougt)]

Attachment: Same as above but chaging the return type of DelayedOnStartRequest

Comment 6

[Doug Turner (:dougt)]

This patch will allow protocols to fire both onStartRequest and onStopRequest 
without blowing away the existing content if the there is no onDataA fired.

Comment 7

[Darin Fisher]

looks nice and straightforward to me (r/sr=darin)

Comment 8

[Doug Turner (:dougt)]

setting milestone...

Comment 9

[Doug Turner (:dougt)]

to mscott.

scott, with this patch, protocols that run into an error can send a
OnStartRequest followed by an OnStopRequest without having the original content
be destroyed.  

Basically what was happening was that FTP we be using the tree widget.  It may
run into a problem (too many users connect, ect).  The protocol would send a
OnStartRequest, then an OnStopRequest.  The OnStartRequest would tear down the
current view of the origanal webpage leaving a nice black window.

Please sr= and reassign back to me or check it in.

Comment 10

[scottputterman]

reassigning back to dougt.  Scott, can you do a sr= or should we find someone
else to look at this?

Comment 11

[Doug Turner (:dougt)]

Too late to get in at the last minute.  maybe next time.

Comment 12

[benc]

mass move, v2.
qa to me.

Comment 13

[Doug Turner (:dougt)]

since no one cares nor do I.

Comment 14

[Doug Turner (:dougt)]

what is milestone "mozilla1.0" anyway?  Moving to future.

Comment 15

[Doug Turner (:dougt)]

Attachment: updated to trunk

Comment 16

[rpotts (gone)]

hey doug,

i'm not sure that this (entire) patch is necessary.  I think that it is a NOP
for http (and other protocols that have to parse out protocol-specific headers
at the beginning of transactions) because, by the time OnStartRequest(...) is
fired there is *already* data from the server waiting...

So, delaying viewer construction until the first ODA is just prolonging the
inevitable :-)  The ODA event "should" be processed *very* soon afterwards
(probably the next time through the message loop)...

I do see the need for a patch which checks the status code in
OnStartRequest(...) and *only* tears down the current document if the status
code is *not* failure !

It looks like right now we only handle NS_ERROR_ABORTED failures, not random
connection failures :-(

so, i think that a better patch could just be:

    nsDocumentOpenInfo::OnStartRequest(...)
    {
                  :
                  :
        nsresult status = NS_OK;
        rv = request->GetStatus(&status);
        // nsIRequest::GetStatus(...) should never fail.  So, ASSERT to catch
        // stubbed out implementations...
        NS_ASSERTION(NS_SUCCEEDED(rv));
        if (NS_FAILED(rv)) return NS_ERROR_FAILURE;

        if (NS_FAILED(status)) {
            // Returning a failure code will cause the transaction to abort.
            return status;
        }
                  :
                  :
    }
   
This would take care of the FTP case where OnStartRequest receives an error
status...

what do you think?
-- rick

Comment 17

[Doug Turner (:dougt)]

Aren't you just saying that  ProcessCanceledCase() should check for any failure
code?


Index: nsURILoader.cpp
===================================================================
RCS file: /cvsroot/mozilla/uriloader/base/nsURILoader.cpp,v
retrieving revision 1.84
diff -u -r1.84 nsURILoader.cpp
--- nsURILoader.cpp	2001/08/21 06:26:59	1.84
+++ nsURILoader.cpp	2001/08/27 20:25:09
@@ -141,7 +141,7 @@
     request->GetStatus(&rv);
 
     // if we were aborted or if the js returned no result (i.e. we aren't
replacing any window content)
-    if (rv == NS_BINDING_ABORTED || rv == NS_ERROR_DOM_RETVAL_UNDEFINED)
+    if (NS_FAILED(rv))
     {
       canceled = PR_TRUE;
       // free any local state for this load since we are aborting it so we 

Comment 18

[rpotts (gone)]

Basically...  The only difference is that in the case of NS_BINDING_ABORTED we
want to return NS_OK...

After thinking about it... though...  I'm not sure why BINDING_ABORTED gets
special treatment...  

I can see NS_ERROR_DOM_RETVAL_UNDEFINED and the new NS_ERROR_NO_CONTENT status
codes returning NS_OK.  But it seems like NS_ERROR_BINDING_ABORTED should be
propagated out to the OnStopRequest(...) of the listener.

-- rick

Comment 19

[Doug Turner (:dougt)]

adding to ricks pile of stuff to do.

Comment 20

[rpotts (gone)]

-> 0.9.6

Comment 21

[rpotts (gone)]

This issue will be solved by the patch attached to bug #106558.

-- rick


*** This bug has been marked as a duplicate of 106558 ***