176919 - support for async input/output streams

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Darin Fisher]

with the current mozilla architecture, data is pushed from the transport layer,
through the protocols, up to the consumers.  each layer must process all the
data pushed to it (via OnDataAvailable).  this requirement often complicates
stream listener implementations (e.g., additional buffering).

instead, consumers should be able to "ask" to be notified when an input stream
(or output stream) becomes "ready" ... by "ready" i mean either closed,
readable, or writable.  then consumers could be in complete control of the data
flow.

i think a lot of networking code could be simplified if the streams could tell a
consumer when they are ready.

i've put together a prototype implementation of nsPipe that provides these
interfaces:

  nsIAsyncInputStream : nsIInputStream
  {
    void asyncWait(in nsIInputStreamListener listener);
  }
  nsIAsyncOutputStream : nsIOutputStream
  {
    void asyncWait(in nsIOutputStreamListener listener);
  }

the listener interfaces are:

  nsIInputStreamListener : nsISupports
  {
    void onInputStreamReady(in nsIAsyncInputStream stream);
  }
  nsIOutputStreamListener : nsISupports
  {
    void onOutputStreamReady(in nsIAsyncOutputStream stream);
  }

there are a lot of details to work out, and the patch i'm about to submit is
only a prototype.

Comment 1

[Darin Fisher]

Attachment: prototype patch

Comment 2

[Asa Dotzler [:asa]]

There are quite a few bugs, some nasty, which are waiting on this fix. Setting
blocking1.3b+ flag because drivers would like to see the dependencies fixed for
1.3beta.

Comment 3

[Darin Fisher]

Attachment: patch: xpcom - v1

this patch includes all changes under xpcom/.  the biggest changes include:

1- addition of nsIAsyncInputStream and nsIAsyncOutputStream
2- addition of nsIInputStreamNotify and nsIOutputStreamNotify
3- pipe rewrite; nsPipe2 is superceded by nsPipe3
4- nsStreamUtils, including NS_AsyncCopy for copying data between two
asynchronous streams (i.e. connects an async input stream to an async output
stream).

Comment 4

[Darin Fisher]

Attachment: patch: necko transport - v1

this patch includes only transport related changes under netwerk/base/.  this
patch should only depend on changes made under xpcom/.

the biggest changes include:

1- nsITransport no longer has asyncRead/asyncWrite methods.  returns
input/output streams optionally supporting nsIAsync{In,Out}putStream interfaces
depending on flags passed to open the streams.
2- rewrite of socket transport (nsSocketTransport2 supercedes
nsSocketTransport)
3- file transport replaced by more appropriately named stream transport
service.

Comment 5

[Darin Fisher]

Attachment: patch: cache - v1

this patch includes all changes under netwerk/cache/.  changes include:

1- cache descriptor returns blocking input/output streams instead of a
transport.
2- storage transport is eliminated in favor of storage stream (from xpcom/io/).

Comment 6

[Darin Fisher]

Attachment: patch: necko utility classes - v1

this patch includes necko utility classes for common operations that leverage
async streams.	classes include:

1- nsInputStreamPump: drives a nsIStreamListener given a nsI{Async}InputStream
2- nsInputStreamChannel: wraps nsInputStreamPump in an implementation of
nsIChannel (this is an existing class that has just been modified).
3- nsAsyncStreamCopier: copies data asynchronously from a nsIInputStream to a
nsIOutputStream.  this basically wraps NS_AsyncCopy in a nsIRequest impl.

Comment 7

[Darin Fisher]

Attachment: patch: http - v1

all changes in netwerk/protocol/http/.	biggest items:

1- moved HTTP connection management out into nsHttpConnectionMgr class.  this
was done to better factor the code, and keep multithreading issues well
isolated.  the HTTP connection mgr uses the new nsISocketTransportService::
postEvent method to move specific tasks onto the socket transport thread to
avoid locks.  this greatly eliminates the amount of MT code, which was an very
important goal of this work since the existing code is far too complex making
it difficult to fix bugs and add features.

2- a nsHttpConnection object now lives almost exclusively on the socket
transport thread.  this also greatly simplifies the HTTP MT logic.

3- the nsHttpPipeline now requests a new transaction from the connection mgr
when a transaction completes.  this helps keep the connection saturated, and
has proven to yield a significant improvement in pipelined Tp.

4- the HTTP channel uses a nsInputStreamPump to read from either a transaction
or the a cache input stream.

5- the buffer used to buffer a byte range response is now eliminated as the
nsInputStreamPump fully supports Suspend/Resume.

Comment 8

[Darin Fisher]

Attachment: patch: jar, file - v1

includes changes in netwerk/protocol/jar/ and netwerk/protocol/file/.  its a
large patch because the file and jar protocol channels needed to be rewritten
in terms of the nsInputStreamPump.  the changes are fairly straightforward.

Comment 9

[Darin Fisher]

Attachment: patch: ftp, gopher, finger, datetime, about - v1

changes to other channel implementations.  gopher, finger, and datetime changes
are almost identical.  these channels actually share a lot of code (hmm.. maybe
they could be better factored into some kind of base class!)  about: changes
are just to deal with some minor API changes.

Comment 10

[Darin Fisher]

some comments about the FTP changes:

1- eliminated nsFtpStreamProvider.  the nsFtpControlConnection now just opens a
buffered output stream on the socket transport and writes commands directly to
it.    (this same thing was done to eliminate nsIStreamProvider implementations
in gopher, finger, and datetime.)

2- uses a nsInputStreamPump wrapping a buffered socket input stream to drive the
DataRequestForwarder.

Comment 11

[Darin Fisher]

Attachment: patch: misc changes to necko - v1

this patch just includes some misc changes to necko.  mostly removing/moving
code and fixing up nsNetUtil.h to provide inline constructors for
nsInputStreamPump and friends.

Comment 12

[Darin Fisher]

Attachment: patch: chatzilla - v1

backwards compatible patch for chatzilla.  attempts to detect old necko
interfaces and use them if available.  otherwise it uses the much simpler new
necko interfaces.  the new code path eliminates the StreamProvider class.

Comment 13

[Darin Fisher]

Attachment: patch: mailnews - v1

changes under mailnews/.  these changes follow a simple theme.	nsMsgProtocol
opens input and output streams.  then it uses a nsInputStreamPump to AsyncRead
from the input stream.	nsMsgAsyncWriteProtocol uses nsIOutputStreamNotify to
asynchronously write to the output stream (used when sending messages). 
nsMsgAsyncWriteProtocol could probably be implemented in terms of
nsAsyncStreamCopier, but i haven't fully thought that through.	the
implementation in this patch conforms to the existing design as closely as
possible.

IMAP changes are more interesting.  it likewise utilizes nsInputStreamPump to
AsyncRead from the stream and depends heavily on Suspend/Resume working
correctly.  performance downloading large imap folders seems very good, though
i haven't run any benchmarks (are there any?).

Comment 14

[Darin Fisher]

Attachment: patch: other - v1

other misc changes outside netwerk/.  big items include:

1- nsWyciwygChannel changes (cache API changes; now uses nsInputStreamPump).
2- nsJSProtocolHandler changes (nsIStreamIO no longer exists).
3- slight change to nsIDocShell::loadStream method.

Comment 15

[Darin Fisher]

full patch available if anyone wants it.

Comment 16

[Darin Fisher]

Attachment: new IDL files listed for your reviewing convenience

Comment 17

[gordon]

Comment on attachment 111458 [details] [diff] [review]
patch: cache - v1

Looks good.  Ship it!

Comment 18

[Darin Fisher]

Attachment: overall static code/data reduction from patch

cathleen got Tz numbers for the latest patch.  here's a summary:

Overall Change in Size:
    Total:	   -25051 (+198700/-223751)
    Code:	  -22671 (+187098/-209769)
    Data:	   -2380 (+11602/-13982)


The biggest reduction was in necko.dll, a total of 33k

necko
    Total:	   -33792 (+90963/-124755)
    Code:	  -30777 (+81814/-112591)
    Data:	   -3015 (+9149/-12164)


Other areas of reductions include:
mime
    Total:	     -960 (+724/-1684)
    Code:	    -816 (+720/-1536)
    Data:	    -144 (+4/-148)


docshell
    Total:	     -726 (+2480/-3206)
    Code:	    -656 (+2448/-3104)
    Data:	     -70 (+32/-102)

emitter
    Total:	     -696 (+704/-1400)
    Code:	    -464 (+704/-1168)
    Data:	    -232 (+0/-232)


addrbook
    Total:	     -610 (+4228/-4838)
    Code:	    -544 (+3984/-4528)
    Data:	     -66 (+244/-310)

msgbase
    Total:	     -594 (+5200/-5794)
    Code:	    -528 (+5168/-5696)
    Data:	     -66 (+32/-98)

rdf
    Total:	     -594 (+2384/-2978)
    Code:	    -528 (+2336/-2864)
    Data:	     -66 (+48/-114)

msgcompo
    Total:	     -554 (+3370/-3924)
    Code:	    -576 (+3264/-3840)
    Data:	     +22 (+106/-84)

gkcontent
    Total:	     -352 (+37472/-37824)
    Code:	    -352 (+37472/-37824)
    Data:	      +0 (+0/+0)

urildr
    Total:	     -192 (+1408/-1600)
    Code:	    -208 (+1376/-1584)
    Data:	     +16 (+32/-16)

xpinstal
    Total:	      -48 (+1680/-1728)
    Code:	     -64 (+1632/-1696)
    Data:	     +16 (+48/-32)

appcomps
    Total:	      -32 (+2768/-2800)
    Code:	     -32 (+2768/-2800)
    Data:	      +0 (+0/+0)


NOTE: there are codesize increases in xpcom and parts of mailnews.

Comment 19

[Darin Fisher]

cathleen sent me these mini-ibench results [win2k, 2.4 GHz, 1GB]:

default cache settings:

baseline
--------------
First loop (uncached?)  :           10.422
Avg. subsequent loop (cached?):      7.906
Total for all loops :               34.141
Number of loops :                        4

w/patch
--------------
First loop (uncached?)  :            8.938
Avg. subsequent loop (cached?) :     7.505
Total for all loops :               31.453
Number of loops:                         4


Disk cache = 0:

baseline
------------------
First loop (uncached?):              8.829
Avg. subsequent loop (cached?):      8.536
Total for all loops :               34.438
Number of loops :                        4

w/patch
------------------
First loop (uncached?):              8.485
Avg. subsequent loop (cached?):      8.365
Total for all loops :               33.579
Number of loops :                        4


Disk cache = 0  & memory cache = 0:

baseline
---------------
First loop (uncached?) :            13.688
Avg. subsequent loop (cached?):     16.917
Total for all loops:                64.438
Number of loops:                         4

w/patch
---------------
First loop (uncached?) :            12.297
Avg. subsequent loop (cached?):     12.318
Total for all loops :                49.25
Number of loops:                         4

Comment 20

[Darin Fisher]

cathleen also sent me these jrgm/cowtools results:

win98, 200mHz, 64MB
-----------------------------
baseline:  2500 ms
w/patch:   2475 ms

win2k, 2.4 GHz, 1GB
-----------------------------
baseline:   309 ms
w/patch:    302 ms


these are cummulative numbers (5 cycles)

Comment 21

[Darin Fisher]

window open:
--------------------
1) 2.4 GHz, 1GB, win2k desktop
    baseline:  293ms   288ms  290ms
    w/patch:   297ms   291ms  294ms
2) 650 MHz, 512MB, win2k laptop
    baseline:  434ms   432ms  430ms
    w/patch:   432ms   428ms  427ms
3) 200MHz, 64MB, win98 desktop
    baseline: 2026ms  2013ms 2009ms
    w/patch:  2013ms  2026ms 2010ms

Seems like there is not much difference in window open.
Each score is an average of 10 window open test.

Startup
--------------
1) 2.4 GHz, 1GB, win2k desktop
     baseline: 1412, 1600, 1287, *1131*, 1350, 1881, 1850, 1225
     w/patch:  1396, 1662, 1053, *928*, 1475, 1037, 1037, 1615
2) 650 MHz, 512MB, win2k laptop
     baseline: 2077, 2168, 2080, 2312, 2424, 2215, 2630, *2074*
     w/patch:  2419, 2632, *1787*, 1880, 1952, 2633, 2531, 2576
3) 200MHz, 64MB, win98 desktop
     baseline: 7790, 7590, 7720, 7430, 7490, 7930, *7290*, 7700
     w/patch:  *7130*, 7760, 7280, 7470, 7230, 7330, 7430, 7170

There could be possible win in startup.  Test on tinderbox reports the lowest
score of 10 runs.  I marked out the lowest core of 8 runs here.

Comment 22

[Darin Fisher]

forgot to add that the Ts and Txul numbers are again courtesy of cathleen :-)

Comment 23

[Chris McAfee]

Ts up 20% on comet, ~ flat on beast/win32

Comment 24

[Aleksey Nogin]

Comment on attachment 111475 [details] [diff] [review]
patch: jar, file - v1

This commit have added a warning on brax TBox:

+netwerk/protocol/jar/src/nsJARChannel.cpp:443
+ `nsresult rv' might be used uninitialized in this function

Indeed, the warning appears to be pointing out a real issue - if ext is
non-NULL, but mimeServ is NULL, then the code will check NS_FAILED on an
uninitialized nsresult - see
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/protocol/jar/src/ns
JARChannel.cpp&rev=1.96&mark=446,469,470,471,481#443

Comment 25

[Aleksey Nogin]

Comment on attachment 111473 [details] [diff] [review]
patch: necko utility classes - v1

This checkin have added a warning on brad TBox:

+netwerk/base/src/nsInputStreamPump.cpp:310
+ `PRUint32 nextState' might be used uninitialized in this function

Indeed, if mState is neither of the 3 values handled in the switch, the
nextState will stay uninitialized - see
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/base/src/nsInputStr
eamPump.cpp&rev=1.1&mark=310,311,323#304

P.S. See bug 59652 and	bug 179819 for more discussion of "uninitialized"
warnings.

Comment 26

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

so... which of these patches are now checked in? all of them?
if so, is this bug fixed?

Comment 27

[Darin Fisher]

comment #24 is legitimate if the mime service fails to be initialized.
comment #25 is invalid.  the warning occurs because of the switch statement
appearing to allow nextState to be unset.  it cannot be unset because mState can
only be within the set of handled cases.

Comment 28

[Mike Potter]

I think that this checkin broke reading and writing files for applications that
use jslib.  I've fixed reading files for the calendar, and will send the fix
back to them, hopefully someone there will fix writing files.
People should be on the look out for add on applications that no longer work
after this checkin.

Comment 29

[Mike Potter]

http://mozdev.org/bugs/showattachment.cgi?attach_id=674 has the patch that was
applied to the calendar code to allow it to read files using the jslib code.

http://mozdev.org/bugs/show_bug.cgi?id=2954 is the bug.

Comment 30

[Darin Fisher]

mike: sorry for the jslib bustage.  i had no idea of its existance, and these
are not frozen APIs :-(

marking FIXED (all patches landed).

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 111455 [details] [diff] [review]
patch: xpcom - v1

OK... here goes.

>Index: xpcom/build/dlldeps.cpp

>-    NS_NewPipe(NULL, NULL, 0, 0, PR_FALSE, PR_FALSE, NULL);
>+    NS_NewPipe2(NULL, NULL, PR_FALSE, PR_FALSE, 0, 0, NULL);

I guess we're just revving this function name because the signature changed and
it has nothing to do with the nsPipe3.cpp name of the file... a little
confusing, but I can see why we'd want to keep it that way.  Though, we're
keeping NS_NewPipe around too, no (if I read nsIPipe.idl correctly)?  Why?

>Index: xpcom/io/nsIAsyncInputStream.idl

>+interface nsIInputStreamNotify;

This seems like a very awkward name for that beastie.... nsIInputStreamListener
perhaps?

This interface could use a comment explaining why you'd want to use one in
general (eg 'Use an nsIAsyncInputStream when you want to have an input stream
that will notify you when data is available and wait for you to read it').

>+    void closeEx(in nsresult reason);

Maybe "closeWithReason()"?  "closeEx" is somewhat random...

>+     * @param aRequestedCount
>+     *        Wait until at least this many bytes may be read.  This is only
>+     *        a suggestion to the underlying stream; it may be ignored.

Maybe callers should be able to pass "0" if they don't care?

>+     * @param aEventQ

aEventQueue, maybe?  The more I think about it, the less I care... ;)

>+interface nsIInputStreamNotify : nsISupports

Should this live in a separate IDL file?  If we'll have to do that anyway at
some point (before freezing this) we may as well do it now...


>Index: xpcom/io/nsIAsyncOutputStream.idl

Basically the same exact comments apply here as for nsIAsyncInputStream.idl

>Index: xpcom/io/nsIPipe.idl

Again, a short blurb explaining why, where, and how this would be used would be
great.

>-     * supports nsIObservableInputStream and nsISearchableInputStream

Perhaps leave the part about nsISearchableInputStream in there?

>+inline nsresult
>+NS_NewPipe(nsIInputStream **pipeIn,

Could we move all this C++ out of the IDL file?  ;)


>Index: xpcom/io/nsInputStreamTee.cpp

Could we leave a debug-only version of that code in, possibly?	Doesn't need to
return an error, jsut to assert that NS_FAILED(rv) || !nonBlocking... That
would catch misuse more reliably than the assert when we try to actually write
to it, imo...

>Index: xpcom/io/nsPipe3.cpp

>+class nsPipeEvents

This could use a short comment explaining what it's used for....

>+class nsPipeInputStream : public nsIAsyncInputStream

Same here

>+    NS_DECL_ISUPPORTS_INHERITED

This is a very fishy use of NS_DECL_ISUPPORTS_INHERITED... at least comment why
you're using it even though you are not in fact inheriting from anyone who's
implementing a refcount?

Maybe use an nsAutoRefCnt instead of an nsrefcount?  Not really sure it
matters.

Please document what OnInputReadable actually does?  In particular, document
the return value...


>+class nsPipeOutputStream : public nsIAsyncOutputStream

Same comments here as for the input stream.  (And document OnOutputWritable).

>+    nsPipeInputStream* InputStream() { return &mInput; }
>+    nsPipeOutputStream* OutputStream() { return &mOutput; }

Do these two methods need to exist?  Why not access those members directly?

>+nsPipe::nsPipe()
>+{
>+    NS_INIT_ISUPPORTS();
>+}

You don't need to call NS_INIT_ISUPPORTS() anymore... dbaron has removed almost
all uses in the tree at this point.  Take that out, please?

>+    InputStream()->SetNonBlocking(nonBlockingIn);
>+    OutputStream()->SetNonBlocking(nonBlockingOut);

mInput.SetNonBlocking(nonBlockingIn);
mOutput.SetNonBlocking(nonBlockingOut);

>+nsPipe::PeekSegment(PRUint32 index, char *&cursor, char *&limit)

This has gotten a face-lift since this patch landed anyway... so has the
AdvanceReadCursor() code... (just in case anyone else decides to review this,
don't bother with the patch in these parts; read the actual file).

>+        InputStream()->ReduceAvailable(bytesRead);

mInput.ReduceAvailable(bytesRead);

>+            // notify output stream that pipe is writable
>+            if (OutputStream()->OnOutputWritable(events))

mOutput.OnOutputWritable(events);

>+        if (mReadCursor == mWriteCursor)
>+            NS_ERROR("read cursor is bad");

NS_ASSERTION here?

>+                OutputStream()->SetWritable(PR_FALSE);

mOutput.SetWritable(PR_FALSE);

>+        if (InputStream()->OnInputReadable(bytesWritten, events))

mInput.OnInputReadable(bytesWritted, events);

>+        if (outputOnly && !InputStream()->Available())

mInput.Available();

>+            if (InputStream()->OnInputException(reason, events))

mInput.OnInputException(reason, events);

>+        if (OutputStream()->OnOutputException(reason, events))

mOutput.OnOutputException(reason, events);

>+    // return error if pipe closed with error.
>+    if (!mAvailable && NS_FAILED(mPipe->mStatus))
>+        if (mPipe->mStatus != NS_BASE_STREAM_CLOSED)

Shouldn't that be one |if| statement with just a && tossed in there?

>+nsPipeInputStream::ReadSegments(nsWriteSegmentFun writer, 
>+            mPipe->OnPipeException(rv);

Hmm.. so this would notify the reader that the stream is ready and _then_
return an error from ReadSegments?  That seems a little odd....

>+            if (NS_FAILED(rv) || (writeCount == 0)) {

That second condition is overparenthesized....

>+nsPipeInputStream::Search(const char *forString, 
>+            // exception, then we should just inform that caller that all

"the caller"

I guess we're hoping no one ever calls Search with a string that has parts in 3
separate segments, huh?  I guess that's a decent-ish assumption....


>+nsPipeOutputStream::WriteSegments(nsReadSegmentFun reader,
>+            mPipe->OnPipeException(rv);

Same as the similar code in nsPipeInputStream::ReadSegments...	Is this really
what we want to do?

>+            rv = reader(this, closure, segment, *writeCount, segmentLen, &readCount);

Could we have an assert in the success case that readCount <= segmentLen? 
Similarly for ReadSegments in nsPipeInputStream.

>+            if (NS_FAILED(rv) || (readCount == 0)) {

This is overparenthesized.

>+nsPipeOutputStream::WriteFrom(nsIInputStream* fromStream,
>+                                      PRUint32 count,
>+                                      PRUint32 *writeCount)

Wacky indentation.

>+        // replace a pending notify
>+        mNotify = 0;

So... is there a good reason you can think of why this code would ever get
called when mNotify is non-null with a |mNotify != notify|?  I can't think of
any offhand....  Wouldn't doing that force the first mNotify to never get the
notification it's waiting for?	(All this applies to the nsPipeInputStream
too.)

>Index: xpcom/io/nsStreamUtils.cpp

>+    nsInputStreamReadyEvent(nsIInputStreamNotify *notify,
>+    {
>+        NS_INIT_ISUPPORTS();
>+    }

No need for that.

>+    static void *PR_CALLBACK EventHandler(PLEvent *plevent)

PR_STATIC_CALLBACK(void*) EventHandler(PLEvent *plevent)

>+        nsInputStreamReadyEvent *ev = (nsInputStreamReadyEvent *) plevent;

NS_STATIC_CAST, please

>+    static void PR_CALLBACK EventCleanup(PLEvent *plevent)

PR_STATIC_CALLBACK(void) EventCleanup(PLEvent *plevent)

>+        nsInputStreamReadyEvent *ev = (nsInputStreamReadyEvent *) plevent;

NS_STATIC_CAST


Same comments for the nsOutputStreamReadyEvent

>+    nsStreamCopierIB(nsIAsyncInputStream *in,
>+        { NS_INIT_ISUPPORTS(); }

No need for that.

>+    NS_IMETHOD OnOutputStreamReady(nsIAsyncOutputStream *out)
>+            if (NS_FAILED(rv) || (n == 0)) {

Overparenthesized

>+    nsStreamCopierOB(nsIAsyncInputStream *in,
>+        { NS_INIT_ISUPPORTS(); }

The usual.  ;)

>+    NS_IMETHOD OnInputStreamReady(nsIAsyncInputStream *in)
>+            if (NS_FAILED(rv) || (n == 0)) {

Overparenthesized

>+NS_AsyncCopy(nsIAsyncInputStream *source,
>+        //
>+        // fire off two async copies :-)
>+        //
>+        rv = NS_AsyncCopy(source, pipeOut, segmentSize, PR_FALSE, PR_TRUE);
>+        rv = NS_AsyncCopy(pipeIn, sink, segmentSize, PR_TRUE, PR_FALSE);

Surely the segmentSize should come _after_ the booleans.

>+        // maybe calling NS_AsyncCopy twice is a bad idea!
>+        NS_ASSERTION(NS_SUCCEEDED(rv), "uh-oh");

Let's make this assert a _little_ more informative, ok?  ;)  Something like
"Async copying into pipe that will never empty"?

>Index: xpcom/tests/TestPipes.cpp
>+class nsPump : /*public nsIInputStreamObserver, 
>+               public nsIOutputStreamObserver,*/

Why leave the commented out code there?

>+        rv = WriteAll(out1, buf, len, &writeCount);
>+        //rv = out1->Write(buf, len, &writeCount);

Same here.

>+    //rv = TestPipeObserver();
>+    //NS_ASSERTION(NS_SUCCEEDED(rv), "TestPipeObserver failed");

And here.

>Index: xpcom/threads/nsEventQueueUtils.h
>+inline nsresult
>+NS_GetEventQueueService(nsIEventQueueService **result)
>+{
>+  nsresult rv;
>+
>+  static NS_DEFINE_CID(kEventQueueServiceCID, NS_EVENTQUEUESERVICE_CID);
>+  nsCOMPtr<nsIEventQueueService> eqs = do_GetService(kEventQueueServiceCID, &rv);
>+  if (NS_FAILED(rv)) return rv;
>+
>+  NS_ADDREF(*result = eqs);
>+  return NS_OK;
>+}

replace the body with:

static NS_DEFINE_CID(kEventQueueServiceCID, NS_EVENTQUEUESERVICE_CID);
return CallGetService(kEventQueueServiceCID, result);

??

Comments on other diffs coming up later (tomorrow or Friday)

Comment 32

[Darin Fisher]

lot's of good suggestions.  thx bz!

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 111455 [details] [diff] [review]
patch: xpcom - v1

One more comment on the XPCOM stuff.. I'm a little worried about the ownership
issues of the notify stuff.  Should we make it clearer that the stream we are
AsyncWait()ing on does _not_ hold a useful ref to the notify object passed in
(it does hold a ref, but it drops this ref, so this ref should not be counted
on to keep the object alive....)

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 111455 [details] [diff] [review]
patch: xpcom - v1

Speaking of the memory issues...  I just remembered why it's a problem. 
NS_AsyncCopy creates a StreamCopier, sets it to wait, and then drops the ref to
it.  So the copier is held alive only by the stream it's waiting on.  So the
code that possibly kills the "notify" object should be fixed, I think...

Comment 35

[Darin Fisher]

discussed this with bz.. not a prob.  just needs to be thoroughly documented ;-)

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 111455 [details] [diff] [review]
patch: xpcom - v1

OK, never mind me.  The ref nsPipeEvents holds saves the day for spidy.  Could
we get that documented so no one mucks with it?

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 111457 [details] [diff] [review]
patch: necko transport - v1

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

At risk of starting to sound repetitive, add a few lines explaining how this
interface would typically be used?  If I understand correctly, this lets you
set up a pipe and gives you back the input and output streams for that pipe;
then you can give the output stream to someone to pump data into and pass the
input stream to someone to read data from?  And the difference from nsIPipe is
that one of the streams may Just Exist already and we just want the other one? 
It would be good to document this.

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

>+/**
>+ * nsISocketTransport
>+ *
>+ * NOTE: This is a free-threaded interface.
>+ */

Could we just say "this interface may be called (used?) on any thread"?  That
would help people who may not know that term...

>+    [noscript] void getAddress(in PRNetAddrStar addr);

Hmm... This would be better as:

native PRNetAddr(PRNetAddr);
[noscript] PRNetAddr getAddress();

That compiles to the same C++ and is a lot clearer, imo.

>+     * Security info object returned from the PSM socket provider.

Maybe "SSL socket provider"?

>+     * Security notification callbacks passed to PSM

Same here?

It's a nit; feel free to ignore it.  ;)

>+    const unsigned long STATUS_RESOLVING      = 0x804b0003;
>+    const unsigned long STATUS_CONNECTED_TO   = 0x804b0004;
>+    const unsigned long STATUS_SENDING_TO     = 0x804b0005;
>+    const unsigned long STATUS_RECEIVING_FROM = 0x804b0006;
>+    const unsigned long STATUS_CONNECTING_TO  = 0x804b0007;
>+    const unsigned long STATUS_WAITING_FOR    = 0x804b000a;

I'm torn between listing these in numeric order and chronological order (with
CONNECTING_TO moved up to the second spot, and WAITING_FOR possibly moved too).
 Again, no real preference on this one.

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


>+    nsISocketTransport createTransport([array, size_is(aTypeCount)]
>+                                       in string aSocketTypes,

Put those on one line?	That looks very confusing (as an extra arg instead of a
qualifier).

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

>+    static NS_METHOD FillPipeSegment(nsIOutputStream *, void *, char *,
>+                                     PRUint32, PRUint32, PRUint32 *);

Give those params names please?

>+nsInputStreamTransport::Run()
>+        if (mOffset != ~0U) {

Shouldn't that be PRUint32(-1)?  The IDL specifies to pass "-1".. (yes, I know
we have problems if 
PRUint32(-1) != ~0U, but...).

>+    static NS_METHOD ConsumePipeSegment(nsIInputStream *, void *, const char *,
>+                                        PRUint32, PRUint32, PRUint32 *);

Param names.

>+        if (mOffset != ~0U) {

As above.

>+nsOutputStreamTransport::OpenOutputStream(PRUint32 flags,
>+    NS_ASSERTION(flags == 0, "unexpected flags");

Does this assert need to be added to nsInputStreamTransport::OpenInputStream? 
Or removed from here?

>+nsStreamTransportService::nsStreamTransportService()
>+    : mLock(PR_NewLock())
>+{
>+    gSTS = this;

Please assert that gSTS is null here.  We don't want someone calling
do_CreateInstance on us and thus creating _two_ transport services that will
start scribbling on the gSTS pointer, etc....

>+nsStreamTransportService::Init()
>+{
>+    nsAutoLock lock(mLock);

Assert that mPool is null here?  Or do we not care if someone calls Init()
twice?	If so, why are we locking?

>Index: netwerk/base/src/nsStreamTransportService.h
>+    /** used internally **/
>+    nsresult Dispatch(nsIRunnable *);

make this protected and declare some friend classes (and remove that comment)? 
;)

>+private:
>+    nsresult Dispatch_Locked(nsIRunnable *);

This seems to no longer be implemented.  Remove the decl?

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


>+static PRErrorCode RandomizeConnectError(PRErrorCode code)
>+    // correctly despite the random occurance of there errors.

"these", not "there".

>+NS_IMPL_QUERY_INTERFACE2(nsSocketInputStream,
>+                         nsIInputStream,
>+                         nsIAsyncInputStream)

This would really belong before the OnSocketReady method imo....  Not a big
deal either way.

Same for the nsSocketOutputStream impl.

>+nsSocketTransport::Init(const char **types, PRUint32 typeCount,
>+        mTypes = (char **) PR_Malloc(mTypeCount + sizeof(char *));

mTypeCount * sizeof(char*)

>+nsSocketTransport::SendStatus(nsresult status)
>+        sink->OnTransportStatus(this, status, progress, ~0U);

Again, perhaps PRUint32(-1)

>+nsSocketTransport::OnMsgInputClosed(nsresult reason)
>+    // check if event should effect entire transport

affect

>+nsSocketTransport::OnMsgOutputClosed(nsresult reason)
>+    // check if event should effect entire transport

affect

>+nsSocketTransport::OpenInputStream(PRUint32 flags,
>+        rv = NS_AsyncCopy(&mInput, pipeOut, PR_FALSE, PR_TRUE,
>+                          segsize, 1, segalloc);

Why "1" for the segment count?	Isn't that going to be unused in this case
anyway?  Wouldn't it be more consistent to pass segcount or 0?

>+nsSocketTransport::OpenOutputStream(PRUint32 flags,
>+        rv = NS_NewPipe2(getter_AddRefs(pipeIn), getter_AddRefs(pipeOut),
>+                         PR_TRUE, !openBlocking, segsize, segcount, segalloc);

Same.

>+    // flag output stream as open
>+    mInputClosed = PR_FALSE;

mOutputClosed = PR_FALSE;

+// called on the socket thread only

Could the methods for which this is true assert so?
  NS_ASSERTION(PR_GetCurrentThread() == gSocketThread, "Called on wrong
thread");
or something along those lines?  Or is gSocketThread not guaranteed to be set
to the right thing?


>Index: netwerk/base/src/nsSocketTransport2.h

+class nsSocketInputStream : public nsIAsyncInputStream
>+    nsIOutputStreamNotify *mNotify;

Could we make that a strong ref?  It means we pay an extra addref/release in
OnSocketReady, but it'd be a good deal safer...

I'd do that as a separate checkin from anything else and I'd be ok with backing
it out if the cost of that extra addref/release is really noticeable....

>+class nsSocketOutputStream : public nsIAsyncOutputStream
>+    nsIOutputStreamNotify *mNotify;

Same.

>+    // we have to be careful with these.  they are modified on the DNS thread,
>+    // while we are in the resolving state.  once we've received the event
>+    // MSG_DNS_LOOKUP_COMPLETE, these can only be accessed on the socket thread.

Could accesses to these in the code assert that either the event has not been
received or the thread is the socket thread?

Also, if we have code here that must be called off the main thread (not sure if
there is such), could we assert that too?


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

>+nsSocketTransportService::nsSocketTransportService()
>+    gSocketTransportService = this;

Assert that this is null in case someone does a do_CreateInstance on us....

>+nsSocketTransportService::~nsSocketTransportService()
>+    PR_DestroyLock(mEventQLock);

Shouldn't that null-check the lock?

>+//-----------------------------------------------------------------------------
>+// socket api (socket thread only)

Could these assert being called on the socket thread?

The hashtable access should probably check that the hashtable is inited (by
looking at its .ops member) and bail if it's not....

>+    static PRBool PR_CALLBACK MatchEntry(PLDHashTable *, const PLDHashEntryHdr *, const void *);
>+    static void   PR_CALLBACK ClearEntry(PLDHashTable *, PLDHashEntryHdr *);

PR_STATIC_CALLBACK(PRBool)/PR_STATIC_CALLBACK(void)

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 111458 [details] [diff] [review]
patch: cache - v1

>Index: netwerk/cache/public/nsICacheEntryDescriptor.idl

>+     * @param offset
>+     *        read starting from this offset into the cached data.

>+     * @param offset
>+     *        write starting from this offset into the cached data.

What happens if the offset is past the end of the existing data?  For writing,
it looks to me like we'll just expand the entry and leave a chunk of garbage
data in the middle?  For reading, I'm not sure what happens.  It would be good
to document this, in any case.

>Index: netwerk/cache/src/nsCacheEntryDescriptor.cpp

>+nsresult nsCacheEntryDescriptor::
>+nsOutputStreamWrapper::OnWrite(PRUint32 count)
>+    if (count > 0x7FFFFFFF)  return NS_ERROR_UNEXPECTED;

PR_INT32_MAX, please.

>Index: netwerk/cache/src/nsDiskCacheStreams.cpp

>+// NOTE: called with service lock held
> nsresult
> nsDiskCacheStreamIO::Seek(PRInt32 whence, PRInt32 offset)
> {
>-    nsAutoLock lock(nsCacheService::ServiceLock()); // grab service lock
>+    //nsAutoLock lock(nsCacheService::ServiceLock()); // grab service lock

Just remove the line, don't comment it....

Looks good other than that!  ;)

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 111473 [details] [diff] [review]
patch: necko utility classes - v1

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

Some brief documentation explaining why one would use this would be good.  The
comments for the two methods cover that information in sum, but it would be
good to just have an overall description of the interface...

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

>+     * This attribute can only be changed before the channel is opened.
>+     */
>+    attribute nsIInputStream contentStream;

What exception is thrown if you try to change it afterward?

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

Description of overall purpose in life?

>+    /**
>+     * asyncRead causes the input stream to be read in chunks and delivered
>+     * asynchronously to the listener via OnDataAvailable.
>+     *
>+     * @param aListener
>+     *        receives notifications.
>+     * @param aListenerContext
>+     *        passed to listener methods.
>+     */
>+    void asyncCopy(in nsIRequestObserver   aObserver,
>+                   in nsISupports          aObserverContext);

This comment looks like it came from nsIInputStreamPump and just got lost here.
 Please fix?

>Index: netwerk/base/src/nsInputStreamPump.cpp
>+    , mStreamLength(~0U)

If you mean -1, use -1.  If you mean PR_UINT32_MAX, use that....

>+nsInputStreamPump::EnsureWaiting()
>+{
>+    if (!mWaiting) {
>+        nsresult rv = mAsyncStream->AsyncWait(this, 0, mEventQ);
>+        if (NS_FAILED(rv)) {
>+            NS_ERROR("AsyncWait failed");
>+            return rv;
>+        }
>+        mWaiting = PR_TRUE;

I asumme that we will never have two threads calling that?

>+nsInputStreamPump::AsyncRead(nsIStreamListener *listener, nsISupports *ctxt)

>+    if (nonBlocking)
>+        mAsyncStream = do_QueryInterface(mStream);
...
>+    // mStreamOffset now holds the number of bytes currently read.  we use this
>+    // to enforce the mStreamLength restriction.
>+    mStreamOffset = 0;

Um.  What if mStream implements both nsISeekableStream and nsIAsyncInputStream?
 Shouldn't we seek it?

>+nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream *stream)
>+        if (mSuspendCount || mState == STATE_IDLE) {
>+            mWaiting = PR_FALSE;
>+            break;
>+        }

Hmm... So if I have an input stream pump.  I call AsyncRead on it, suspend it
and then forget about it, ok?  At this point the only ref to it is the one that
the async input stream holds (since the pump is waiting on it).  This
OnInputStreamReady gets called and we just bounce out of it without doing
anything, at which point the ref to us is dropped (by the event that called
us), and we go away, taking the stream and such with us.

I suppose that makes sense, since suspending a pump and then dropping the ref
to it means that you will never unsuspend it and thus do not care whether it
dies.... but shouldn't the pump's destructor close the streams involved or
something?  Or is it just relying on the stream destructors to clean up?  The
StreamTransport is held by the transport service and only goes away if it does
not re-dispatch itself... what cleans it up?

>+nsInputStreamPump::OnStateTransfer()
>+        if (avail + mStreamOffset > mStreamLength) {
>+            avail = mStreamLength - mStreamOffset;
>+            if (avail > mSegSize)
>+                avail = mSegSize;
>+        }

Shouldn't that inner if statement be done no matter whether avail +
mStreamOffset > mStreamLength?

>+nsInputStreamPump::OnStateStop()
>+    mAsyncStream = 0;

Same question here.  We don't need to explicitly close mAsyncStream?

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

>+nsInputStreamChannel::GetName(nsACString &result)
>+    return mURI->GetSpec(result);

What if mURI is null?

>+nsInputStreamChannel::Open(nsIInputStream **result)

What if this gets called multiple times?  We'll just keep passing back this
same stream, I guess...  Do we know for sure that this stream is blocking so
when the consumer calls Read() it will block?

>+nsInputStreamChannel::AsyncOpen(nsIStreamListener *listener, nsISupports *ctxt)
>+    if (mContentLength == -1)
>+        mContentStream->Available((PRUint32 *) &mContentLength);
>+    nsresult rv = NS_NewInputStreamPump(getter_AddRefs(mPump), mContentStream,
>+                                        -1, mContentLength, 0, 0, PR_TRUE);
>+    if (NS_FAILED(rv)) return rv;

What if mContentStream is being filled from the other end (eg a pipe)?	Then
its Available will _not_ return the complete amount of data it may have
available; just what's there right now.  And we will only pump out that much
data, leaving the rest in the stream....  Seems wrong to me.

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

>+nsInputWrapper::Available(PRUint32 *avail)
>+{
>+    nsresult status;
>+    if (mCopier->IsComplete(&status))
>+        return status;

Hmm... can't it be complete with a NS_OK status?  Don't we want to set *avail
to 0 in this case?

>+nsInputWrapper::Read(char *buf, PRUint32 count, PRUint32 *countRead)
>+    nsresult rv = mSource->Read(buf, count, countRead);
>+    /*
>+    if (NS_FAILED(rv)) {
>+        if (rv != NS_BASE_STREAM_WOULD_BLOCK)
>+            CloseEx(rv);
>+        // else, not a real error
>+    }
>+    else if (*countRead == 0)
>+        CloseEx(NS_BASE_STREAM_CLOSED);
>+    */
>+    return rv;

What's up with this code? Is it in or out?

>+nsInputWrapper::ReadSegments(nsWriteSegmentFun writer, void *closure,
>+                             PRUint32 count, PRUint32 *countRead)

Same question about a similar block of code.

>+nsInputWrapper::IsNonBlocking(PRBool *result)

Again, can we be complete with an NS_OK status?

>+nsInputWrapper::AsyncWait(nsIInputStreamNotify *notify, PRUint32 amount, nsIEventQueue *eventQ)
>+{
>+    // we'll cheat a little bit here since we know that NS_AsyncCopy does pass
>+    // an event queue.
>+    NS_ASSERTION(eventQ == nsnull, "unexpected");

Shouldn't that be NS_ASSERTION(eventQ, "unexpected") ?

>+nsOutputWrapper::Write(const char *buf, PRUint32 count, PRUint32 *countWritten)
>+    /*
>+    if (NS_FAILED(rv) && rv != NS_BASE_STREAM_WOULD_BLOCK)
>+        CloseEx(rv);
>+    */

In or out?  ;)

>+nsOutputWrapper::WriteSegments(nsReadSegmentFun reader, void *closure,
>+    /*
>+    if (NS_FAILED(rv) && rv != NS_BASE_STREAM_WOULD_BLOCK)
>+        CloseEx(rv);
>+    */

Again.

>+nsOutputWrapper::IsNonBlocking(PRBool *result)

The usual question about being complete with NS_OK status.  ;)

>+nsOutputWrapper::AsyncWait(nsIOutputStreamNotify *notify, PRUint32 amount, nsIEventQueue *eventQ)
>+    // we'll cheat a little bit here since we know that NS_AsyncCopy does pass
>+    // an event queue.
>+    NS_ASSERTION(eventQ == nsnull, "unexpected");

Again, the assert seems to be reversed....

>Index: netwerk/base/src/nsAsyncStreamCopier.h

>+    class nsInputWrapper : public nsIAsyncInputStream
>+        NS_DECL_ISUPPORTS_INHERITED

Please comment explaining why you're doing that even though it's obviously not
inheriting an nsISupportsImpl....

>+    //-------------------------------------------------------------------------
>+    // nsInputWrapper
>+    //-------------------------------------------------------------------------
>+
>+    class nsOutputWrapper : public nsIAsyncOutputStream

Fix the comment?

Same question about NS_DECL_ISUPPORTS_INHERITED.

I didn't give the new tests a very thorough review, but they look more or less
ok.

Comment 40

[timeless]

Comment on attachment 111455 [details] [diff] [review]
patch: xpcom - v1

>+interface nsIAsyncInputStream : nsIInputStream
>+{
>+     * Any pending asyncWait will be notified.
>+    void closeEx(in nsresult reason);
>+     * @param aNotify
>+     *        This object is notified when the stream becomes ready.
does being closed count as ready? I'm fairly certain from reading closeEx()
that aNotify will be called when the stream is closed
>+    void asyncWait(in nsIInputStreamNotify aNotify,

>+interface nsIAsyncOutputStream : nsIOutputStream
>+{
>+     * @param aNotify
>+     *        This object is notified when the stream becomes ready.
same comment.
>+    void asyncWait(in nsIOutputStreamNotify aNotify,

>+NS_IMETHODIMP
>+nsPipe::GetInputStream(nsIAsyncInputStream **aInputStream)
>+{
>+    NS_ADDREF(*aInputStream = &mInput);
>+    return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsPipe::GetOutputStream(nsIAsyncOutputStream **aOutputStream)
>+{
>+    NS_ADDREF(*aOutputStream = &mOutput);
>+    return NS_OK;
>+}
It appears that the refcount for mInput and mOutput starts at 0 and goes up to
1 when Get<>Stream is called, which measn that they could be destroyed when a
caller drops its reference to the object.  Did I miss something?

Comment 41

[Boris Zbarsky [:bzbarsky]]

> Did I miss something?

Yes.  mInput and mOutput implement custom AddRef/Release methods (see comment
31) which refcount the pipe itself.

Comment 42

[timeless]

Comment on attachment 111457 [details] [diff] [review]
patch: necko transport - v1

>+    void close(in nsresult aReason);
bz suggested closeWithReason, could you possibly use the same name for the same
signature?

> interface nsISocketTransport : nsITransport 
>+     * Security info object returned from the PSM socket provider.  This object
bz nit'd about the explicit reference to PSM. I too don't want to see PSM
explicitly referenced in this stuff.

>+    if (mTypeCount) {
>+        mTypes = (char **) PR_Malloc(mTypeCount + sizeof(char *));
>+            mTypes[type++] = PL_strdup(proxyType);
>+            mTypes[type++] = PL_strdup(types[i]);
hrm, i didn't see the pairs to this

>+nsSocketTransport::OpenInputStream(PRUint32 flags,
>+    if (!(flags & OPEN_UNBUFFERED) || (flags & OPEN_BLOCKING)) {
>+    else
>+        *result = &mInput;
>+    NS_ADDREF(*result);
Again, it looks like the default refcount will be 1 which would lead to a
caller killing it.

>+nsSocketTransport::OpenOutputStream(PRUint32 flags,
>+    if (!(flags & OPEN_UNBUFFERED) || (flags & OPEN_BLOCKING)) {
>+    else
>+        *result = &mOutput;
>+    NS_ADDREF(*result);
again...

>+    // these members are "set" at initialization time and are never modified
>+    // afterwards.  this allows them to be safely accessed from any thread.
>+    char      **mTypes;

>+nsSocketTransportService::ClearEntry(PLDHashTable *table,
>+    PL_strfree((char *) he->key);

>+    // this condition variable will be checked to determine if the socket
>+    // handler should be detached.  it must only be accessed on the socket
>+    // thread.
>+    nsresult mCondition;
why is it an nsresult and what results is it likely to have?

>+    // LookupHost checks to see if we've previously resolved the hostname
>+    // during this session.  We remember all successful connections to prevent
>+    // ip-address spoofing.  See bug 149943.
Does going offline flush this?

>+    nsresult LookupHost(const nsACString &host, PRUint16 port, PRIPv6Addr *addr);
>+        // give background threads a chance to finish whatever work they may
>+        // be doing.
this is broken :-).  I know you don't care, but if you could file a bug against
me, i'll address it the next time i run through test apps.
>+        LOG(("waiting 1 second before exiting...\n"));
>+        PR_Sleep(PR_SecondsToInterval(1));
>+    } // this scopes the nsCOMPtrs

Comment 43

[(not reading, please use [e-mail] instead)]

Comment on attachment 111480 [details] [diff] [review]
patch: mailnews - v1

this landed, so clearing the review request

Comment 44

[[e-mail]]

Seems like the original checkin on January is the root cause for bug 192808. 

I can't pinpoint the exact location and don't have conclusive evidence, I did
confirm however that the problem started occurring after January 17. This change
is the most likely candidate for the XPI hangers we have been seeing with 1.3b.

Comment 45

[Mathieu Fenniak]

>>> People should be on the look out for add on applications that no longer work
after this checkin.

Greetings.  I am the lead developer of a Mozilla add-on application which uses
networking code through XPCOM.  I am most furious about this bug.

One of the fundamental concepts of COM is that interfaces never change.  I see
absolutely no reason why it was necessary to change function definitions and
outright remove entire functions from public interfaces which are being widely
used by Mozilla add-on applications.  Interfaces /never/ change.  Interfaces can
be added to, and new interfaces can be created, but existing interfaces should
/never/ change.  Certainly changes which involve outright removing of functions
should scream "CREATE A NEW INTERFACE" to the developer.

Mozilla's interfaces are frozen by the act of publishing the software. 
Developers cannot limit themselves to working mearly on your officially frozen APIs.

Because of this change, numerous developers like myself are forced to branch
their projects and maintain a release which will work with pre-1.3b Mozilla, and
another that will work with post-1.3b Mozilla.  I'm unable to simply update my
project to work with 1.3b and ignore users with older software.

Please, consider all APIs frozen by act of software publication.  This is not
going to harm your development, it will simply make Mozilla a friendly
development environment.

Comment 46

[Boris Zbarsky [:bzbarsky]]

> Please, consider all APIs frozen by act of software publication.

That would mean never checking in any work-in-progress, making development
_very_ difficult.  Right now the @FROZEN tag exists to mark interfaces public;
any interface not marked such is a work in progress and should _NOT_ be treated
as a public interface -- it is not one....

If you're finding that you have to use a non-frozen interface, please let us
know!  (n.p.m.embedding, or mailing the module owner involved).  We want to know
about such things so that we can freeze interfaces that are in a finalized state
and are genuinely useful to people (the latter is sometimes more important than
the former).

Comment 47

[Boris Zbarsky [:bzbarsky]]

More importantly, which particular interfaces are the problem in this case?  The
socket transport and the inability to use it to stream data to/from files?

Comment 48

[Mathieu Fenniak]

>>> That would mean never checking in any work-in-progress, making development
_very_ difficult.

I don't suggest that work in progress is not checked in.  I suggest that
interfaces which are present in released software (cut, versioned releases)
should not change.

>>> If you're finding that you have to use a non-frozen interface, please let us
know!  (n.p.m.embedding, or mailing the module owner involved).  We want to know
about such things so that we can freeze interfaces that are in a finalized state
and are genuinely useful to people (the latter is sometimes more important than
the former).

I find it unlikely that anybody thought nsITransport and derived interfaces were
only being used internally.  Third party developers have to use these interfaces
to do any low-level networking, right?  If these interfaces aren't to be
considered public, then I can't make useful software with Mozilla.

>>> More importantly, which particular interfaces are the problem in this case?
 The socket transport and the inability to use it to stream data to/from files?

The actual concept behind these changes is what causes me problems.  I need to
re-write my socket code, which was previously done through asyncRead/asyncWrite,
to use an entirely different interface.  The redeclaration of
nsISocketTransportService::createTransport, nsITransport::openInputStream, and
nsITransport::openOutputStream also breaks existing code.

Comment 49

[Boris Zbarsky [:bzbarsky]]

> cut, versioned releases

Then drivers need to get sane and not cut releases every 4 weeks.... :(

Comment 50

[Darin Fisher]

Mathieu: i'm sorry for the headache this has caused you, and i am more than
willing to help you make the transition to the new interfaces.  but, most
importantly as boris stated we are learning through this that these interfaces
need to be frozen.  we all know these are important interfaces that would be
required if anyone wanted to write an external component that makes use of the
socket transport for instance, but we were not aware of any such external
components.  moreover, the policy of mozilla.org with regard to frozen
interfaces and such is exactly as boris stated it.  only interfaces marked
"@status FROZEN" should be considered static from release to release.

can you please tell us more about your application?  thx!

Comment 51

[Mathieu Fenniak]

> Mathieu: i'm sorry for the headache this has caused you, and i am more than
willing to help you make the transition to the new interfaces.  

Thank you for the offer Darin, but I should be able to figure it out based off
the Chatzilla diff attached to this bug.  My networking code was originally
borrowed from there a year and a half ago, anyways. :)

> moreover, the policy of mozilla.org with regard to frozen
interfaces and such is exactly as boris stated it.  only interfaces marked
"@status FROZEN" should be considered static from release to release.

I understand.  I still believe that this is a flawed policy that makes external
development more difficult.

Without static interfaces, XPCOM doesn't seem to serve any purpose, except that
it provides a common point for multiple programming languages to access each
other.  MSCOM strongly discourages changing interfaces (making it very
difficult), as well as clearly documenting that interfaces should never change.
 As a result, ActiveX controls and other COM-based applications are very cleanly
plug-in-able.  (well, you know, besides the flaws in MS's implementation).

Anyways, this isn't the proper forum to suggest change, and I doubt such a
suggestion would be welcome anyways.  After banging my head on the desk this
morning when reading this bug, I couldn't stop myself from saying /something/.

> can you please tell us more about your application?  thx!

The application is a Mozilla based HTML-rendering MOO/MUD client, similar to
Cacho's old Pueblo application.  It's webpage is http://www.moo.ca/moozilla.

Thanks for listening to my complaints, fellows.  Have a good day.

Comment 52

[Darin Fisher]

>Without static interfaces, XPCOM doesn't seem to serve any purpose, except that
>it provides a common point for multiple programming languages to access each
>other.  MSCOM strongly discourages changing interfaces (making it very
>difficult), as well as clearly documenting that interfaces should never change.
> As a result, ActiveX controls and other COM-based applications are very
>cleanly plug-in-able.  (well, you know, besides the flaws in MS's >implementation).

mozilla.org stongly discourages changes to frozen interfaces (verboten!).  the
fact that XPCOM is used to link components _internal_ to the mozilla
implementation is in fact just an implementation detail.  the fact that mozilla
has frozen so few interfaces is the unfortunate bit IMO.  we are working to
freeze more interfaces.

Comment 53

[Roland Mainz]

Darin Fisher wrote:
> we are working to freeze more interfaces.

Please be carefull not to freeze too much. There are already a couple of
(badly-designed) interfaces which are frozen and now we have to work around that
(yes, I know, the alternative would be to create a new interface with a new name
- but some of this stuff like nsIPrefsBranch covers most of Mozilla's code...
;-(( ) - which costs _much_ time... ;-((

Comment 54

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 111474 [details] [diff] [review]
patch: http - v1

I would like to keep this on my radar, since I would still like to review the
rest of this code....

What's the bug number for landing the changes that correspond to my comments up
to now, Darin?

Comment 55

[Darin Fisher]

bz: bug 193917