226972 - The LoadURI function of nsIWebnavigation does not break the read loop when readSegments returns failure code.

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Medappa]

User-Agent:       Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.5) Gecko/20031007 Firebird/0.7
Build Identifier: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.5) Gecko/20031007 Firebird/0.7

When an instance of inpustream is passed as 'postdata' or 'headerData' to
LoadURI function of nsIWebnavigation interface, the code reading the data from
the inputstream does not break the loop if readSegments fails (returns failurecode).

And also the 'read' function of the 'inputstream' is never called.
If the 'read' function is not called for instance of inpustream passed to
LoadURI function it should have been documented in nsIWebNavigation that streams
passed to that function must implement 'readSegments'.


Reproducible: Always

Steps to Reproduce:
1.Create a class implementing nsIInputStream with ReadSegments function
returning 'NS_ERROR_NOT_IMPLEMENTED'
2. Pass the instance of the inputstream to LoadURI as postData/headers.
3.

Actual Results:  
The Available function of nsIInputStream is called in a infinite loop.
Read function of nsIInputStream is not called.

Expected Results:  
- The error code returned by ReadSegment function must be checked.
- The requirement to implement ReadSegment function of nsIInputStream for
streams passed to LoadURI function must be documented in nsIWebNavigation

Comment 1

[Darin Fisher]

-> me

Comment 2

[Boris Zbarsky [:bzbarsky]]

Confirming.  Darin, any chance of a fix to this?  This can be a pretty serious
problem for embeddors....

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

adding as blocker for "freeze nsiwebnav", for this part of comment 0:
>If the 'read' function is not called for instance of inpustream passed to
>LoadURI function it should have been documented in nsIWebNavigation that streams
>passed to that function must implement 'readSegments'.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

the function in question is nsDocShell::AddHeadersToChannel

which is currently at:
http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#5738

Comment 5

[Darin Fisher]

Attachment: v1 patch

fix this bug and cleanup the surrounding code.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

please document this requirement (that streams have to impl readSegments) near
http://lxr.mozilla.org/seamonkey/source/docshell/base/nsIWebNavigation.idl#147

looks like postData streams have to impl this too, although I can't find a loop
as described in comment 0...

Comment 7

[Darin Fisher]

biesi: yeah, i will document that.  the loop for postdata is actually deep in
necko, but see nsIUploadChannel.  we should also note that the upload data
stream may be read on a background thread.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 165238 [details] [diff] [review]
v1 patch

>Index: nsDocShell.cpp
>+AppendSegmentToString(nsIInputStream *in,

>+    nsCString* buf = (nsCString*) closure;

I'd prefer you cast this to the actual concrete type it is (nsCAutoString*),
and use NS_STATIC_CAST.

That way if for some strange reason the nsCString* and nsCAutoString* ever
don't agree (vtables or something) you don't run into issues.

>+    return NS_ERROR_UNEXPECTED;
> }

(At end of the AddHeadersToChannel function).
Want to put an NS_NOTREACHED here too?	With the while (PR_TRUE) above, we
really shouldn't be ending up down here.  ;)

sr=bzbarsky with those nits.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #7)
> biesi: yeah, i will document that.  the loop for postdata is actually deep in
> necko, but see nsIUploadChannel.  we should also note that the upload data
> stream may be read on a background thread.

ah, yeah... maybe a comment "The postData stream must follow the rules from
nsIUploadChannel"?

Comment 10

[Darin Fisher]

biesi: yeah, i'll incorporate those comment changes with my patch for bug 99625.

boris: nsCAutoString is and always has been a subclass of nsCString.  but,
sure... i'll make the change before checking in.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Darin, the situation that would worry me is this:

class nsCAutoString : public something, public nsCString {
};

(I know it's not likely to happen, but dealing with it safely in this code is
easy, so no reason not to do it).

Comment 12

[Darin Fisher]

Attachment: v1.1 patch

revised patch

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 165480 [details] [diff] [review]
v1.1 patch

+    nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(aGenericChannel);
+    NS_ENSURE_STATE(httpChannel);

ok... guess returning a failure code here is OK, since we failed to add headers
(since the channel does not support headers). but, do we really want to warn in
that case?

+    // Iterate over the headersString: for each "\r\n" delimeted chunk,

"delimited"

+	 const nsCSubstring &oneHeader = StringHead(headersString, crlf);

hmm, this will include the CR, won't it? Guess it doesn't matter due to our
Trim later on.

+	 colon = oneHeader.FindChar(':');
+	 if (colon == kNotFound)
+	     return NS_ERROR_UNEXPECTED;

I wonder... would processing the next header be a better way to handle this?
guess it doesn't matter much.


+	 rv = httpChannel->SetRequestHeader(headerName, headerValue, PR_TRUE);
+	 NS_ENSURE_SUCCESS(rv, rv);

here too - would a continue; be better?


r=me either way. nice code cleanup!

Comment 14

[Darin Fisher]

biesi: thanks for the review comments...


> ok... guess returning a failure code here is OK, since we failed to add 
> headers (since the channel does not support headers). but, do we really want 
> to warn in that case?

i figured that if someone supplied headers, then they would probably like to
know if their expectation of being able to set headers failed.


> +    // Iterate over the headersString: for each "\r\n" delimeted chunk,
> 
> "delimited"

good catch.


> +	 const nsCSubstring &oneHeader = StringHead(headersString, crlf);
> 
> hmm, this will include the CR, won't it? Guess it doesn't matter due to our
> Trim later on.

Hmm... no, that parameter is the length of the substring.  so, if CR is at index
N, then the substring would be of length N.  It would represent characters from
index 0 to index N-1 (inclusive).  The CR would be past the end of the substring.


> +	 colon = oneHeader.FindChar(':');
> +	 if (colon == kNotFound)
> +	     return NS_ERROR_UNEXPECTED;
> 
> I wonder... would processing the next header be a better way to handle this?
> guess it doesn't matter much.

Well, if we didn't find a required colon, then how do we know that we can
proceed?  Seems to me that it would be better to bail then to try to recover
from bad input data.  We might end up setting a header value as the header name
or something evil like that! :(


> +	 rv = httpChannel->SetRequestHeader(headerName, headerValue, PR_TRUE);
> +	 NS_ENSURE_SUCCESS(rv, rv);
> 
> here too - would a continue; be better?

again, better to bail if something goes wrong here.  it could only mean certain
badness.

Comment 15

[Darin Fisher]

fixed-on-trunk

Comment 16

[guillaume.lundy]

hello, 

I'm embedding Mozilla 1.7.3 in a windows java appli using JRex 1.0b_html2
I had to make HTTP POST calls so I encountered this bug.

I back ported the v1.1 patch in the mozilla 1.7.3 source code
but it still doesn't work because of the line

    nsresult rv = aHeadersData->ReadSegments(AppendSegmentToString,
                                             &headersString,
                                             PR_UINT32_MAX,
                                             &numRead);
 

JRex CPP part makes an allocation error.

I think that Jrex tries to allocate a memoy bloac with the size PR_UINT32_MAX.
But PR_UINT32_MAX is > 4 000 000 000.

==> alloc error.
In my test, I set 65536 instead of PR_UINT32_MAX in the call 
   aHeadersData->ReadSegments(...)  and it worked fine

I'm not a "true" Mozilal developper so could you make a cleaner fix 
than mine in the current dev. line.

Regards, guillaume 

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #16)
> I think that Jrex tries to allocate a memoy bloac with the size PR_UINT32_MAX.
> But PR_UINT32_MAX is > 4 000 000 000.

what's the concrete type of aHeadersData?

Comment 18

[guillaume.lundy]

(In reply to comment #17)
> what's the concrete type of aHeadersData?

It is a complementary part of POST HEADER. so it is a String.
(It is not the POST DATA, only a subpart of the header.
In my apply, I send
ContentType : .....
Content-Lenght : ....

rgds, guillaume

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

> It is a complementary part of POST HEADER. so it is a String.

no, it's not. it's a stream. so what kind of stream is it?

Comment 20

[guillaume.lundy]

(In reply to comment #19)
> no, it's not. it's a stream. so what kind of stream is it?

Well  it is a class form JREX.

JNIEXPORT void JNICALL 
Java_org_mozilla_jrex_navigation_WebNavigationImpl_LoadURI
  (JNIEnv *env, jobject navigObj, jint jrexPeer, jstring juri, jint loadFlags, 
jstring jrefUri, jobject postData, jobject headers){

...

	JREX_LOGLN("LoadURI()--> **** juri <"<<juri<<"> loadFlags 
	nsIInputStream* headersIn=NOT_NULL(headers)?new JRexInputStream
(headers):nsnull;

...
}


JRexInputStream.h
=================
#include "JRex_JNI_Util.h"

class JRexInputStream : public nsIInputStream{
	public:
		static jmethodID closeMID;
		static jmethodID availableMID;
		static jmethodID readMID;

		NS_DECL_ISUPPORTS
		NS_DECL_NSIINPUTSTREAM

	 	JRexInputStream(jobject inStream);
	 	virtual ~JRexInputStream();
	private:
		jobject mJavaInputStream;
		PRBool mIsClosed;
};

extract form JRexInputStream.cpp
================================
NS_IMETHODIMP JRexInputStream::ReadSegments(nsWriteSegmentFun aWriter, void * 
aClosure, PRUint32 aCount, PRUint32 *_retval){
	NS_ENSURE_ARG_POINTER(_retval);

	JREX_LOGLN("ReadSegments()--> ****  aCount<"<<aCount<<"> ****")
	nsresult rv=NS_OK;
	PRUint32 rVal=0;

	char* from = (char*)nsMemory::Alloc(aCount);
	if(!from){
		JREX_LOGLN("ReadSegments()--> **** Alloc FAILED!!! ****")
		rv= NS_ERROR_OUT_OF_MEMORY;
		goto clean;
	}


Rgds guillaume

Comment 21

[Darin Fisher]

It looks like the problem is with JRexInputStream::ReadSegments.  If that code
cannot be corrected, then we can perhaps attempt a workaround in the Mozilla
source.  That said, I would strongly encourage you to try to fix JRex.  It is
not implementing nsIInputStream::ReadSegments correctly.  It should only
allocate a buffer that is as large as the data it has available, even if the
consumer has said that he is willing to receive more than that.

Comment 22

[guillaume.lundy]

(In reply to comment #21)
> It looks like the problem is with JRexInputStream::ReadSegments.

Thanks for the code review.
Medappa posted a mail in the jrex mailing list to say he fixed it.

bye.