Last Comment Bug 192284 - support nsIChannel::open for all protocols
: support nsIChannel::open for all protocols
Status: RESOLVED FIXED
: topembed-
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All All
: P5 enhancement (vote)
: mozilla1.6alpha
Assigned To: Darin Fisher
: benc
: Patrick McManus [:mcmanus]
Mentors:
Depends on: 218584
Blocks: 100022 194198
  Show dependency treegraph
 
Reported: 2003-02-07 11:38 PST by Darin Fisher
Modified: 2003-10-07 21:27 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v0 patch : prototype w/ nsHttpChannel::Open implemented (19.40 KB, patch)
2003-07-31 03:20 PDT, Darin Fisher
no flags Details | Diff | Splinter Review
v1 patch (13.60 KB, patch)
2003-09-18 16:49 PDT, Darin Fisher
doug.turner: review+
Details | Diff | Splinter Review
v1.1 patch - including the new files this time ;-) (25.66 KB, patch)
2003-09-25 11:03 PDT, Darin Fisher
bzbarsky: superreview+
Details | Diff | Splinter Review

Description Darin Fisher 2003-02-07 11:38:34 PST
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 2003-02-11 15:14:10 PST
Plussing topembed nomination, discussed at 2/11 EDT meeting.
Comment 2 Michael Buckland 2003-05-05 13:52:39 PDT
Discussed in topembed bug triage.  Minusing.
Comment 3 Darin Fisher 2003-07-31 03:20:36 PDT
Created attachment 128935 [details] [diff] [review]
v0 patch : prototype w/ nsHttpChannel::Open implemented
Comment 4 Darin Fisher 2003-07-31 03:25:49 PDT
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 2003-09-18 16:49:47 PDT
Created attachment 131697 [details] [diff] [review]
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) 2003-09-22 08:13:18 PDT
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 2003-09-25 10:05:06 PDT
Comment on attachment 131697 [details] [diff] [review]
v1 patch

erm, isn't this patch missing the new files?
Comment 8 Darin Fisher 2003-09-25 10:56:36 PDT
sicking: yes it is.. oops!  though there was no change from the v0 patch in that
department.
Comment 9 Darin Fisher 2003-09-25 11:03:40 PDT
Created attachment 132155 [details] [diff] [review]
v1.1 patch - including the new files this time ;-)
Comment 10 Darin Fisher 2003-09-25 11:06:56 PDT
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 [:bz] (still a bit busy) 2003-10-02 23:02:05 PDT
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 2003-10-07 21:27:11 PDT
fixed-on-trunk

Note You need to log in before you can comment on or make changes to this bug.