Closed
Bug 176919
Opened 22 years ago
Closed 22 years ago
support for async input/output streams
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla1.3beta
People
(Reporter: darin.moz, Assigned: darin.moz)
References
(Blocks 3 open bugs)
Details
Attachments
(13 files, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
gordon
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details | |
(deleted),
application/x-gzip
|
Details |
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.
Assignee | ||
Comment 1•22 years ago
|
||
Assignee | ||
Updated•22 years ago
|
Target Milestone: --- → mozilla1.4alpha
Assignee | ||
Updated•22 years ago
|
Target Milestone: mozilla1.4alpha → mozilla1.3beta
Comment 2•22 years ago
|
||
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.
Flags: blocking1.3b+
Assignee | ||
Comment 3•22 years ago
|
||
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).
Attachment #104283 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #111455 -
Flags: review?(dougt)
Assignee | ||
Updated•22 years ago
|
Attachment #111455 -
Flags: superreview?(brendan)
Assignee | ||
Comment 4•22 years ago
|
||
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.
Assignee | ||
Updated•22 years ago
|
Attachment #111457 -
Flags: superreview?(bzbarsky)
Attachment #111457 -
Flags: review?(dougt)
Assignee | ||
Comment 5•22 years ago
|
||
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/).
Assignee | ||
Updated•22 years ago
|
Attachment #111458 -
Flags: superreview?(bzbarsky)
Attachment #111458 -
Flags: review?(gordon)
Assignee | ||
Comment 6•22 years ago
|
||
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.
Assignee | ||
Comment 7•22 years ago
|
||
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.
Assignee | ||
Comment 8•22 years ago
|
||
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.
Assignee | ||
Comment 9•22 years ago
|
||
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.
Assignee | ||
Comment 10•22 years ago
|
||
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.
Assignee | ||
Comment 11•22 years ago
|
||
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.
Assignee | ||
Comment 12•22 years ago
|
||
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.
Assignee | ||
Comment 13•22 years ago
|
||
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?).
Assignee | ||
Comment 14•22 years ago
|
||
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.
Assignee | ||
Comment 16•22 years ago
|
||
Assignee | ||
Updated•22 years ago
|
Attachment #111479 -
Flags: review?(rginda)
Assignee | ||
Updated•22 years ago
|
Attachment #111480 -
Flags: review?(sspitzer)
Assignee | ||
Updated•22 years ago
|
Attachment #111473 -
Flags: superreview?(bzbarsky)
Attachment #111473 -
Flags: review?(dougt)
Comment 17•22 years ago
|
||
Comment on attachment 111458 [details] [diff] [review] patch: cache - v1 Looks good. Ship it!
Attachment #111458 -
Flags: review?(gordon) → review+
Assignee | ||
Comment 18•22 years ago
|
||
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.
Assignee | ||
Comment 19•22 years ago
|
||
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
Assignee | ||
Comment 20•22 years ago
|
||
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)
Assignee | ||
Comment 21•22 years ago
|
||
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.
Assignee | ||
Comment 22•22 years ago
|
||
forgot to add that the Ts and Txul numbers are again courtesy of cathleen :-)
Comment 24•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
so... which of these patches are now checked in? all of them? if so, is this bug fixed?
Assignee | ||
Comment 27•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 30•22 years ago
|
||
mike: sorry for the jslib bustage. i had no idea of its existance, and these are not frozen APIs :-( marking FIXED (all patches landed).
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
![]() |
||
Comment 31•22 years ago
|
||
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 33•22 years ago
|
||
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•22 years ago
|
||
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...
Assignee | ||
Comment 35•22 years ago
|
||
discussed this with bz.. not a prob. just needs to be thoroughly documented ;-)
![]() |
||
Comment 36•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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! ;)
Attachment #111458 -
Flags: superreview?(bzbarsky) → superreview+
![]() |
||
Comment 39•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
> Did I miss something? Yes. mInput and mOutput implement custom AddRef/Release methods (see comment 31) which refcount the pipe itself.
Comment 42•22 years ago
|
||
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•22 years ago
|
||
Comment on attachment 111480 [details] [diff] [review] patch: mailnews - v1 this landed, so clearing the review request
Attachment #111480 -
Flags: review?(sspitzer)
Comment 44•22 years ago
|
||
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•22 years ago
|
||
>>> 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•22 years ago
|
||
> 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•22 years ago
|
||
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•22 years ago
|
||
>>> 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•22 years ago
|
||
> cut, versioned releases
Then drivers need to get sane and not cut releases every 4 weeks.... :(
Assignee | ||
Comment 50•22 years ago
|
||
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•22 years ago
|
||
> 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.
Assignee | ||
Comment 52•22 years ago
|
||
>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•22 years ago
|
||
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... ;-((
Assignee | ||
Updated•22 years ago
|
Attachment #111455 -
Flags: superreview?(brendan)
Attachment #111455 -
Flags: review?(dougt)
Assignee | ||
Updated•22 years ago
|
Attachment #111457 -
Flags: superreview?(bzbarsky)
Attachment #111457 -
Flags: review?(dougt)
Assignee | ||
Updated•22 years ago
|
Attachment #111473 -
Flags: superreview?(bzbarsky)
Attachment #111473 -
Flags: review?(dougt)
Assignee | ||
Updated•22 years ago
|
Attachment #111479 -
Flags: review?(rginda)
![]() |
||
Comment 54•22 years ago
|
||
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?
Attachment #111474 -
Flags: superreview?(bzbarsky)
![]() |
||
Updated•20 years ago
|
Attachment #111474 -
Flags: superreview?(bzbarsky)
Updated•2 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•