290371 - OnDataAvailable called again after returning an error

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Tommi Komulainen]

Originally reported to GNOME bugzilla as
http://bugzilla.gnome.org/show_bug.cgi?id=300591

Galeon code was written based on the guarantee (hi biesi) that OnDataAvailable()
will not be called again if we return an error code from there once.  But
apparently it is.

What we have is roughly as follows (complete code can be found at
http://cvs.gnome.org/viewcvs/galeon/mozilla/mozilla-embed-persist.cpp?view=markup
 , look for GStreamListener::OnDataAvailable)

channel = ioService->NewChannel ("http://www.publisite.com/favicon.ico");
listener = new MyListener(...);
channel->AsyncOpen(listener, nsnull);

MyListener::OnDataAvailable(...)
{
  rv = aInputStream->ReadSegments (MyReaderFunc, ...);
  NS_ENSURE_SUCCESS(rv, rv);
  return NS_ERROR_FAILURE;
}

MyReaderFunc()
{
  return NS_ERROR_FAILURE;
}

Comment 1

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

Attachment: patch

- don't call OnDataAvailable again if the first ODA or OnStartRequest failed
- ensure that we always call OnStartRequest

Comment 2

[Darin Fisher]

Comment on attachment 180889 [details] [diff] [review]
patch

hmm... shouldn't the nsUnknownDecoder's OnDataAvailable check for errors
returned by FireListenerNotifications?	If it is failing, then calling
OnDataAvailable on the real listener seems like a bad idea to me.

Comment 3

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

isn't that exactly what the first part of this patch does...?

Comment 4

[Darin Fisher]

Comment on attachment 180889 [details] [diff] [review]
patch

>Index: netwerk/streamconv/converters/nsUnknownDecoder.cpp

>+  // Must not fire ODA again if it failed once
>+  if (aCount && NS_SUCCEEDED(rv)) {

yup... that's what i was looking for and yet i missed it somehow :-/


>+  if (NS_FAILED(rv)) {
>+    request->Cancel(rv);
>+    mNextListener->OnStartRequest(request, aCtxt);
>     return rv;
>+  }

so, the explicit call to Cancel is needed so that the status of the
request is set appropriately when mNextListener checks it.  please
document this.	It took me a few moments to realize that that was
what was going on.


> 
>   // Fire the OnStartRequest(...)
>   rv = mNextListener->OnStartRequest(request, aCtxt);
> 
>+  if (!mBuffer) return NS_ERROR_OUT_OF_MEMORY;

what is the concern here?  it seems to me that we check mBuffer in
OnDataAvailable before calling FireListenerNotifications, so why is
this necessary again?  are you concerned about what mNextListener
might do?

Comment 5

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

(In reply to comment #4)
> >+  if (!mBuffer) return NS_ERROR_OUT_OF_MEMORY;
> 
> what is the concern here?

I just moved that line. It used to be near the top of this function. The reason
I moved it is that I want to ensure that we always call OnStartRequest, and this
seemed to be the easiest way to do that.

Comment 6

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

Attachment: patch v2: comment added

Comment 7

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

Comment on attachment 181165 [details] [diff] [review]
patch v2: comment added

this would be good to have... it basically makes sure that a return value is
checked, and that OnStartRequest is called in some error circumstances. it
should be low risk.

Comment 8

[Asa Dotzler [:asa]]

Comment on attachment 181165 [details] [diff] [review]
patch v2: comment added

a=asa

Comment 9

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

Checking in netwerk/streamconv/converters/nsUnknownDecoder.cpp;
/cvsroot/mozilla/netwerk/streamconv/converters/nsUnknownDecoder.cpp,v  <-- 
nsUnknownDecoder.cpp
new revision: 1.65; previous revision: 1.64
done