The default bug view has changed. See this FAQ.

support nsIChannel::open for all protocols

RESOLVED FIXED in mozilla1.6alpha

Status

()

Core
Networking
P5
enhancement
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Darin Fisher, Assigned: Darin Fisher)

Tracking

({topembed-})

Trunk
mozilla1.6alpha
topembed-
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

14 years ago
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.
(Assignee)

Updated

14 years ago
Priority: -- → P3
Target Milestone: --- → mozilla1.4alpha

Comment 1

14 years ago
Plussing topembed nomination, discussed at 2/11 EDT meeting.
Keywords: topembed → topembed+
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
Target Milestone: mozilla1.4alpha → mozilla1.4beta

Comment 2

14 years ago
Discussed in topembed bug triage.  Minusing.
Keywords: topembed+ → topembed-
(Assignee)

Comment 3

14 years ago
Created attachment 128935 [details] [diff] [review]
v0 patch : prototype w/ nsHttpChannel::Open implemented
(Assignee)

Updated

14 years ago
Severity: normal → enhancement
Priority: P3 → P5
Target Milestone: mozilla1.4beta → mozilla1.5beta
(Assignee)

Comment 4

14 years ago
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).
(Assignee)

Updated

14 years ago
Blocks: 100022
(Assignee)

Updated

14 years ago
Target Milestone: mozilla1.5beta → mozilla1.6alpha
Depends on: 218584
(Assignee)

Comment 5

14 years ago
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).
Attachment #128935 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #131697 - Flags: review?(dougt)

Comment 6

14 years ago
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
Attachment #131697 - Flags: review?(dougt) → review+
(Assignee)

Updated

14 years ago
Attachment #131697 - Flags: superreview?(bz-vacation)
Comment on attachment 131697 [details] [diff] [review]
v1 patch

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

Comment 8

14 years ago
sicking: yes it is.. oops!  though there was no change from the v0 patch in that
department.
(Assignee)

Comment 9

14 years ago
Created attachment 132155 [details] [diff] [review]
v1.1 patch - including the new files this time ;-)
Attachment #131697 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #132155 - Flags: superreview?(bzbarsky)
(Assignee)

Updated

14 years ago
Attachment #131697 - Flags: superreview?(bzbarsky)
(Assignee)

Comment 10

14 years ago
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.
Blocks: 194198
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?
Attachment #132155 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 12

14 years ago
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.