65160 - http is using proxies when it doesn't need to.

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[Judson Valeski]

http is using proxy object to marshall callbacks. ruslan put this in to fix some 
problems when OpenInputStream was used. This is not necessary in the AsyncRead 
case and can cause deadlock (posting proxy events when the UI thread is 
wait()ing on another thread (say the socket transport). We should only be 
creating proxies when we need to (i.e. in the OpenInputStream() case).

Comment 1

[Judson Valeski]

I believe this is the cause of 65146.

Comment 2

[Chris Waterson]

Attachment: proposed solution; only create proxy when synchronous

Comment 3

[Judson Valeski]

Chris, on the ride home last night I realized there may be some issues w/ the 
changes I suggested (reflected in the patch you provide).

HTTP is creating 3 proxies:
EventSink (created async)
Prompt (created sync)
ProgressEventSink (created sync)

I believe those that are created "sync" will be fine going direct as a result of 
the patch. However, the semantics of the async one, EventSink, are now 
different. (note: the EventSink (nsIHTTPEventSink) is the one causing trouble 
for you in the deadlock bug.

I suspect we'll need to apply the conditional proxification to the "sync" 
proxies as I don't see any reason why they need to be proxiesin the 
nsIHTTPChannel::AsyncRead() case. And for the EventSink, I suspect we'll need to 
marshall it's methods (those on nsIHTTPEventSink) over manually (probably just 
creating an "async" handler class like 
http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsAsyncStreamListener.h
#62, or just using raw PL_Events). That would prevent event firing (as a result 
of a proxy object being released) in the http destructor altogether.

Comment 4

[Darin Fisher]

HTTP's OpenInputStream implementation is currently unused (and should not be
used because HTTP is not threadsafe).  So, I think that if these proxies are
not needed for AsyncRead, then we should really just do away with them.

As a side note, the implementation of OpenInputStream is going to be replaced
by a single-threaded truly synchronous design.  This will eliminate the need
to do proxying in the synchronous case (and hence in HTTP as a whole).

Comment 5

[Darin Fisher]

this code no longer exists, and the new code is careful to only use proxies when
necessary... marking FIXED.

Comment 6

[John Unruh]

Verified.