192284 - support nsIChannel::open for all protocols

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P5)

Description

[Darin Fisher]

support nsIChannel::open for all protocols.  i know we once had a bug filed for
this, but i just can't seem to locate it.  anyhow, we decided yesterday that
there is need to provide implementations of the sync load for all protocols. 
the solution will most likely involve pushing an event queue to "fake" blocking
i/o on top of AsyncOpen.  this can be implemented as a helper class for use with
all the remote necko protocols.

Comment 1

[Juan José Mata]

Plussing topembed nomination, discussed at 2/11 EDT meeting.

Comment 2

[Michael Buckland]

Discussed in topembed bug triage.  Minusing.

Comment 3

[Darin Fisher]

Attachment: v0 patch : prototype w/ nsHttpChannel::Open implemented

Comment 4

[Darin Fisher]

this implementation is not quite complete... remaining issues:

1- when can the consumer of the HTTP channel check the response headers? 
shouldn't we wait to return from nsHttpChannel::Open until we have the headers?
 NOTE: i think this can be easily implemented by just calling Available on the
nsIInputStream returned from NS_NewSyncStreamListener.

2- in this patch, i'm not pushing or popping event queues.  i'm just using the
existing/current event queue, which i think is ok.  i don't see much value in
pushing an event queue in this case.  NOTE: the event loop happens whenever
Available would return zero bytes available (assuming OnStopRequest has not yet
been called).

Comment 5

[Darin Fisher]

Attachment: v1 patch

final patch.  the only issue i have with this patch is that only some of the
channels implement nsIChannel::Open using AsyncOpen.  as a result, load group
and progress notifications will occur only for those channels which implement
Open via AsyncOpen.  this might lead to subtle bugs when a component starts
sync loading URLs of a different scheme.  we might want to fix this by making
all channels implement Open via AsyncOpen (or perhaps only when a load group
and/or progress event sink exist).

Comment 6

[Doug Turner (:dougt)]

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

worse is better.  lets file a new bug for the known issue and check what what
you got in.  

r=dougt

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

erm, isn't this patch missing the new files?

Comment 8

[Darin Fisher]

sicking: yes it is.. oops!  though there was no change from the v0 patch in that
department.

Comment 9

[Darin Fisher]

Attachment: v1.1 patch - including the new files this time ;-)

Comment 10

[Darin Fisher]

once the patch for bug 210125 goes in, it would not be that difficult to
implement a load flag which would cause the sync load to not trigger any events
other than those needed by the nsSyncStreamListener.  such a load flag could be
used in places where we need sync loading, but cannot risk re-entrancy problems.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132155 [details] [diff] [review]
v1.1 patch - including the new files this time ;-)

>Index: netwerk/base/public/nsISyncStreamListener.idl

>+interface nsISyncStreamListener : nsIStreamListener
>+{
>+    /**
>+     * Returns an input stream that when read will fetch data delivered
>+     * to the sync stream listener.  The nsIInputStream implementation
>+     * will wait for OnDataAvailable events before returning from Read.
>+     */

Do we want to explicitly comment that it will spin the current thread event
queue to get those events and that callers should be ready for it?

>Index: netwerk/base/public/nsNetUtil.h

>+/**
>+ * Implement the nsIChannel::Open(nsIInputStream**) method using the channel's
>+ * AsyncOpen method.
>+ */

And maybe the same warning here.

>Index: netwerk/base/src/nsSyncStreamListener.cpp

>+nsSyncStreamListener::OnDataAvailable(nsIRequest     *request,
>+    nsresult rv = mPipeOut->WriteFrom(stream, count, &bytesWritten);
>+    if (NS_FAILED(rv))
>+        return rv;

Add a comment about why we can ignore bytesWritten (it'll be count for an
infinite pipe; we may want to add an assert to that effect) and maybe why we
don't set mStatus to rv here?

sr=bzbarsky with those nits addressed.	I'm sort of curious as to how the
existing callers of Open() will deal with this patch; perhaps we should make a
list of them and see how likely they are to hit this code?

Comment 12

[Darin Fisher]

fixed-on-trunk