So the fix for bug 536273 is not going to work for large files--it does a blocking read of the entire nsIInputStream into a buffer and send across IPDL. The solution that bz/biesi and I have discussed on IRC is to add ParamTraits logic to serialize whatever subclasses of nsInputStream are used during POSTs/PUTs, and copy only the *file name* when the stream is backed by a file. There will involve at least the following classes: - nsIStringInputStream: data backed by a string: read data, serialize, reconstruct new stream in chrome - nsIFileInputStream: get nsIFile that stream is opened to, serialize file name only across IPDL, create new fileIstream in chrome with file name. This is the big win here--avoid schlepping all the data across IPDL - nsBufferedInputStream: I think this generally shows up in front of an nsIFileInputStream, i.e it does buffering for it. Serialize nsIFile file name, make sure both buffered and backing file stream created in chrome. - nsIMultiplexInputStream: this is essentially a container for other streams. I believe HTML forms POSTs get one of these, which contains an nsIStringInputStream per form element, and an nsIBufferredInputStream for file upload elements. I get the impression from bz that it may also contain the MIME separators that are used. If we can omit serializing the separators and have chrome create them, that's a very minor win. But the main thing is to serialize and reconstruct all the stream children, so it walks and talks the same on chrome. There may be other inputstream subclasses involved--this is just what I saw from a quick gdb session. The ultimate destination for this code is for the PHttpChannel Send/RecvAsyncOpen methods, but that requires all the pieces to work at once to test. It may be easier to start by adding dummy IPDL msgs that take the individual stream types, then tackle nsIMultiplexInputStream. That way we can also get started before bug 536273 lands. I can't tell from a quick browse of of the IDL files if there's any way to get the nsIFile from a stream once we pass it into Init. Anyone know? If not, is there an existing IDL interface we can add an accessor method to? Or it may be simpler to just make ParamTraits a friend class. It's fine to split this bug up into ones for particular inputstream subclasses if that works best. "One patch checked in per bug" is the general operating procedure.

That patch leaks all the substreams, right?

Comment on attachment 445926 [details] [diff] [review] wip Dumb question: why do you need the nsIInputStream ParamTraits, and what does it do? Do the subclasses' ParamTraits not get called automatically w/o this? The implementation is just "WriteParam(stream)"--does that somehow call the right ParamTraits::Write for each subclass?

It won't compile, since what it's trying to do isn't implemented :)

Note: from a discussion with bz/biesi, it sounds like we should probably change things under e10s so that POST/PUT file streams are not buffered, and then we won't have to deal with getting the buffer, etc. (Use nsHttpChannel::mRemoteChannel to fork the logic for the e10s case). But we need to see if the multiplex channel uses ReadSegments to read from the bufferedstream: if it does, then we may need to keep the bufferedStream.

I have a question. What should I do for ParamTraits<InputStream> Write/Read to call the correct type of Write/Read? IPC::InputStream(nsIStringInputStream) needs to call functions in ParamTraits<nsIStringInputStream*>. I was thinking Write() nsCOMPtr<String> s(do_QI(aParam.mStream)); if (s) Write s.get() else nsCOMPtr<File> f(do_QI(aParam.mStream)); if (f) Write f.get() else DIE Read String* s Read succeed? write to aResult and return true fail? File* f Read succeed? write to aResult and return true else return false

Note that the IPC::URI/InputStream construct isn't quite correct. See bz's request in bug 571166 comment 5.

More specifically, I've filed bug 580450 on that issue.

jdm - does this construct block review or landing of this bug? I think no + follow up.

If bug 580450 gets r+ before this goes in, I think it's probably worth getting right the first time. Otherwise we can address it in the other bug if this lands.

Comment on attachment 458793 [details] [diff] [review] almost ready (I think) +#ifdef MOZ_IPC +#include "base/basictypes.h" +#include "IPC/IPCMessageUtils.h" +#endif + Is basictypes.h needed? I tis included in nsFileStreams, but not in nsMIMEInputStream. #include "nsNetUtil.h" //#include "nsFileTransportService.h" +#include "nsIProgrammingLanguage.h" While you are here, please just remove that commented out line. + nsresult rv = NS_NewLocalFile(path, PR_TRUE, getter_AddRefs(file)); + if (NS_FAILED(rv)) + return PR_FALSE; + + Init(file, -1, -1, flags); Init returns a nserror if somethign bad happens. We should probably test for that here. in the mime stream, we aren't looking at either mCLStream or mStream. Nor are we remoting mStartedReading. Isn't that going to be a problem. Is it really true that all of the input streams are MAIN_THREAD_ONLY? I think they are single threaded, but it really doesn't matter which thread you run them from. Also, shouldn't Read() set these "untouched/unremoted" values to zero/null? Pretty close. Also, are there tests for this?

Doug, can you review https://bugzilla.mozilla.org/show_bug.cgi?id=566577#c12 ?

Since bug 580450 was cleaned up, can you make the corresponding changes here Jae-Seong?

>+ InputStream(nsIInputStream* aStream) : mStream(aStream) { fprintf(stderr, "!!!\n"); } This should be fixed up either as a real warning (why?) or deleted.

>+private: >+ // Unimplemented >+ InputStream(InputStream&); More lessons learned from bug 580450 - the default copy constructor is actually invoked for some strange reason on MacOS and Maemo, so you should get rid of this private declaration.

Comment on attachment 461644 [details] [diff] [review] patch 1, sorry for the comment jdm made. Also, I do not understand why we are removing bit that support REOPEN_ON_REWIND. Does no one use that that we know about?

> Also, > I do not understand why we are removing bit that support REOPEN_ON_REWIND. > Does no one use that that we know about? That is from Bug 566577 (comment 1, comment 10). To serialize mFile, I needed mFile to be set all the time.

Comment on attachment 463306 [details] [diff] [review] patch, 2, comment 20 > /** > + * The file being read. > + */ > + readonly attribute nsIFile file; > + > + /** > + * The flags specifying behaviors. > + */ > + readonly attribute long behaviorFlags; Who uses these attributes? > + AppendStream(stream); Shouldn't we check here for an error? This is in nsMultiplexInputStream::Read() and nsMIMEInputStream::Read(). > + PRUint8 uploadStreamHasHeaders, This should be PRBool. Otherwise it looks good.

(In reply to comment #23) > Who uses these attributes? Nobody, I took them out.

Can I assume I got reviews from Doug and Michal? Do I need another review?

Comment on attachment 466556 [details] [diff] [review] patch 3, update Jason, review or check-in please.

Should I grab the review here? If jduell's overloaded I can jump in.

Comment on attachment 466556 [details] [diff] [review] patch 3, update Yoink.

Comment on attachment 466556 [details] [diff] [review] patch 3, update >diff --git a/netwerk/base/src/nsBufferedStreams.cpp b/netwerk/base/src/nsBufferedStreams.cpp >+PRBool >+nsBufferedInputStream::Read(const IPC::Message *aMsg, void **aIter) >+{ >+#ifdef MOZ_IPC >+ using IPC::ReadParam; >+ >+ PRUint32 bufferSize; >+ if (!ReadParam(aMsg, aIter, &bufferSize)) >+ return PR_FALSE; >+ Everything from here... >+ nsCAutoString cidStr; >+ nsCID cid; >+ if (!ReadParam(aMsg, aIter, &cidStr) || >+ !cid.Parse(cidStr.get())) >+ return PR_FALSE; >+ >+ nsCOMPtr<nsIInputStream> stream = do_CreateInstance(cid); >+ if (!stream) >+ return PR_FALSE; >+ >+ nsCOMPtr<nsIIPCSerializable> serializable = do_QueryInterface(stream); >+ if (!serializable || !serializable->Read(aMsg, aIter)) >+ return PR_FALSE; >+ ... to here, can be done with IPC::InputStream. Just call ReadParam, and make an nsCOMPtr<nsIInputStream> from it? >+void >+nsBufferedInputStream::Write(IPC::Message *aMsg) >+{ >+#ifdef MOZ_IPC >+ using IPC::WriteParam; >+ >+ WriteParam(aMsg, mBufferSize); >+ >+ nsIInputStream *stream = Source(); Same comment -- everything from here... >+ nsCOMPtr<nsIIPCSerializable> serializable = do_QueryInterface(stream); >+ NS_ABORT_IF_FALSE(serializable, "QI to nsIIPCSerializable failed"); >+ >+ nsCOMPtr<nsIClassInfo> classInfo = do_QueryInterface(stream); >+ char cidStr[NSID_LENGTH]; >+ nsCID cid; >+ nsresult rv = classInfo->GetClassIDNoAlloc(&cid); >+ NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv), "All IPDL streams must report a valid class ID"); >+ >+ cid.ToProvidedString(cidStr); >+ WriteParam(aMsg, nsCAutoString(cidStr)); >+ serializable->Write(aMsg); ... to here. Just construct an IPC::InputStream from 'stream' and call WriteParam? >diff --git a/netwerk/base/src/nsFileStreams.cpp b/netwerk/base/src/nsFileStreams.cpp >@@ -220,40 +228,33 @@ nsFileInputStream::Open(nsIFile* aFile, > > if (mBehaviorFlags & DELETE_ON_CLOSE) { > // POSIX compatible filesystems allow a file to be unlinked while a > // file descriptor is still referencing the file. since we've already > // opened the file descriptor, we'll try to remove the file. if that > // fails, then we'll just remember the nsIFile and remove it after we > // close the file descriptor. > rv = aFile->Remove(PR_FALSE); >- if (NS_FAILED(rv) && !(mBehaviorFlags & REOPEN_ON_REWIND)) { >- // If REOPEN_ON_REWIND is not happenin', we haven't saved the file yet >- mFile = aFile; >- } Hmm. The possibility of DELETE_ON_CLOSE in the child worries me a bit, but I don't think that's a problem we need to fix here. There's potential for a race if the child deletes the file before the parent has deserialized the stream info and opened it, but that needs to be fixed by denying the child filesystem mutation access. So I think we're OK here. In other news, 'rv' is now unused, so you shouldn't assign into it. >@@ -354,16 +355,115 @@ nsFileInputStream::Seek(PRInt32 aWhence, >+PRBool >+nsFileInputStream::Read(const IPC::Message *aMsg, void **aIter) >+{ >+#ifdef MOZ_IPC >+ using IPC::ReadParam; >+ >+ nsString path; >+ PRInt32 flags; >+ if (!ReadParam(aMsg, aIter, &path) || >+ !ReadParam(aMsg, aIter, &flags)) >+ return PR_FALSE; >+ >+ nsCOMPtr<nsILocalFile> file; >+ nsresult rv = NS_NewLocalFile(path, PR_TRUE, getter_AddRefs(file)); You can use NS_NewNativeLocalFile (with an nsCString) to avoid native --> unicode and unicode --> native conversions -- a little faster. I think we should propagate followLinks (not sure if it's necessary, but it can't hurt). Call GetFollowLinks on the Write side and serialize it over? >+ if (NS_FAILED(rv)) >+ return PR_FALSE; >+ >+ rv = Init(file, -1, -1, flags); Put a comment here about IO flags = -1 meaning readonly and permissions being unimportant since we're reading. >+void >+nsFileInputStream::Write(IPC::Message *aMsg) >+{ >+#ifdef MOZ_IPC >+ using IPC::WriteParam; >+ >+ nsString path; >+ mFile->GetPath(path); Can use GetNativePath here (see above). >+ WriteParam(aMsg, path); >+ WriteParam(aMsg, mBehaviorFlags); >+#endif >+} >diff --git a/netwerk/base/src/nsMIMEInputStream.cpp b/netwerk/base/src/nsMIMEInputStream.cpp >+PRBool >+nsMIMEInputStream::Read(const IPC::Message *aMsg, void **aIter) >+{ >+#ifdef MOZ_IPC >+ using IPC::ReadParam; >+ >+ if (!ReadParam(aMsg, aIter, &mHeaders) || >+ !ReadParam(aMsg, aIter, &mContentLength)) >+ return PR_FALSE; >+ >+ mHeaderStream->ShareData(mHeaders.get(), -1); >+ mCLStream->ShareData(mContentLength.get(), -1); Instead of -1, you can pass foo.Length() here -- saves a strlen call in ShareData. However, I don't think unconditionally calling ShareData is quite right. There are two cases in nsMIMEInputStream that affect these two streams: 1) mStartedReading is false: http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsMIMEInputStream.cpp#147. In this case, mHeaderStream should be initialized with ShareData, but the length should be set to zero. 2) mStartedReading is true: http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsMIMEInputStream.cpp#179. In this case, calling ShareData with both strings is fine. So, we should either a) assert that mStartedReading is always true, and then unconditionally ShareData, or b) conditionally do 1 or 2. Let's do the latter to be safe. >+ >+ nsCAutoString cidStr; >+ nsCID cid; >+ if (!ReadParam(aMsg, aIter, &cidStr) || >+ !cid.Parse(cidStr.get())) >+ return PR_FALSE; >+ >+ nsCOMPtr<nsIInputStream> stream = do_CreateInstance(cid); >+ if (!stream) >+ return PR_FALSE; >+ >+ nsCOMPtr<nsIIPCSerializable> serializable = do_QueryInterface(stream); >+ if (!serializable || !serializable->Read(aMsg, aIter)) >+ return PR_FALSE; Use IPC::InputStream to do all this. >+ >+ mData = stream; Note that 'stream' can be NULL, apparently -- http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsMIMEInputStream.cpp#170. This is handled correctly within IPC::InputStream, though, so definitely use it per above. >+ >+ // nsMIMEInputStreamConstructor already appended mHeaderStream & mCLStream Slightly confusing comment. It's actually nsMIMEInputStream::Init() that appended, want to change it? Also might make more sense to move this comment up to where you mutate mHeaderStream/mCLStream. >+ nsresult rv = mStream->AppendStream(mData); >+ if (NS_FAILED(rv)) >+ return rv; Want 'return PR_FALSE' here. I also think we only want to AppendStream if mData is non-NULL. Again, see http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsMIMEInputStream.cpp#171. >+ if (!ReadParam(aMsg, aIter, &mAddContentLength) || >+ !ReadParam(aMsg, aIter, &mStartedReading)) >+ return PR_FALSE; Will need mStartedReading to be serialized before the streams if the ShareData calls are going to depend on it, per above. >+void >+nsMIMEInputStream::Write(IPC::Message *aMsg) >+{ >+#ifdef MOZ_IPC >+ using IPC::WriteParam; >+ >+ WriteParam(aMsg, mHeaders); >+ WriteParam(aMsg, mContentLength); >+ >+ nsCOMPtr<nsIIPCSerializable> serializable = do_QueryInterface(mData); >+ NS_ABORT_IF_FALSE(serializable, "QI to nsIIPCSerializable failed"); >+ >+ nsCOMPtr<nsIClassInfo> classInfo = do_QueryInterface(mData); >+ char cidStr[NSID_LENGTH]; >+ nsCID cid; >+ nsresult rv = classInfo->GetClassIDNoAlloc(&cid); >+ NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv), "All IPDL streams must report a valid class ID"); >+ >+ cid.ToProvidedString(cidStr); >+ WriteParam(aMsg, nsCAutoString(cidStr)); >+ serializable->Write(aMsg); Again -- use IPC::InputStream here. >+ >+ WriteParam(aMsg, mAddContentLength); >+ WriteParam(aMsg, mStartedReading); >+#endif >+} >+ >diff --git a/netwerk/protocol/http/PHttpChannelParams.h b/netwerk/protocol/http/PHttpChannelParams.h >+template<> >+struct ParamTraits<InputStream> >+{ >+ typedef InputStream paramType; >+ >+ static void Write(Message* aMsg, const paramType& aParam) >+ { >+ bool isNull = !aParam.mStream; >+ aMsg->WriteBool(isNull); >+ >+ if (isNull) >+ return; >+ >+ nsCOMPtr<nsIIPCSerializable> serializable = do_QueryInterface(aParam.mStream); >+ NS_ABORT_IF_FALSE(serializable, "QI to nsIIPCSerializable failed"); Hmm. I'm not sure if we should abort here or have a safe fallback. What about streams implemented from script? IPC::URI does this: http://mxr.mozilla.org/mozilla-central/source/netwerk/ipc/NeckoMessageUtils.h#83 Doing that would mean this kind of construct: bool isSerializable = !!serializable; WriteParam(aMsg, isSerializable); if (!serializable) { NS_WARNING("nsIInputStream implementation doesn't support nsIIPCSerializable; falling back to copying data"); nsCString streamString; PRUint32 bytes; aParam.mStream->Available(&bytes); if (bytes > 0) { nsresult rv = NS_ReadInputStreamToString(aParam.mStream, streamString, bytes); NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv), "Can't read input stream into a string!"); } WriteParam(aMsg, streamString); return; } And on the reading side, use NS_NewCStringInputStream(getter_AddRefs(stream), streamString). I'm not really sure if all this is a great idea, though; or if we even care. (Turning random stream types into string streams might be bad!) I wonder if biesi/bz have thoughts here. Let's hold off on this part til we have a sensible answer. >diff --git a/netwerk/test/unit/test_post.js b/netwerk/test/unit/test_post.js >+const BUFFERSIZ = 4096; BUFFERSIZE? >diff --git a/xpcom/io/nsMultiplexInputStream.cpp b/xpcom/io/nsMultiplexInputStream.cpp >+PRBool >+nsMultiplexInputStream::Read(const IPC::Message *aMsg, void **aIter) >+{ >+#ifdef MOZ_IPC >+ using IPC::ReadParam; >+ >+ PRUint32 count; >+ if (!ReadParam(aMsg, aIter, &count)) >+ return PR_FALSE; >+ >+ for (PRUint32 i = 0; i < count; i++) { >+ nsCAutoString cidStr; >+ nsCID cid; >+ if (!ReadParam(aMsg, aIter, &cidStr) || >+ !cid.Parse(cidStr.get())) >+ return PR_FALSE; >+ >+ nsCOMPtr<nsIInputStream> stream = do_CreateInstance(cid); >+ if (!stream) >+ return PR_FALSE; >+ >+ nsCOMPtr<nsIIPCSerializable> serializable = do_QueryInterface(stream); >+ if (!serializable || !serializable->Read(aMsg, aIter)) >+ return PR_FALSE; IPC::InputStream again? >+ >+ nsresult rv = AppendStream(stream); >+ if (NS_FAILED(rv)) >+ return rv; I think you want 'return PR_FALSE' here. >+void >+nsMultiplexInputStream::Write(IPC::Message *aMsg) >+{ >+#ifdef MOZ_IPC >+ using IPC::WriteParam; >+ >+ PRUint32 count; >+ GetCount(&count); Can just use mStreams.Count() here... >+ WriteParam(aMsg, count); >+ >+ for (PRUint32 i = 0; i < count; i++) { >+ nsIInputStream *stream; >+ GetStream(i, &stream); ... and mStreams.ObjectAt(i) here. Faster. >+ >+ nsCOMPtr<nsIIPCSerializable> serializable = do_QueryInterface(stream); >+ NS_ABORT_IF_FALSE(serializable, "QI to nsIIPCSerializable failed"); >+ >+ nsCOMPtr<nsIClassInfo> classInfo = do_QueryInterface(stream); >+ char cidStr[NSID_LENGTH]; >+ nsCID cid; >+ nsresult rv = classInfo->GetClassIDNoAlloc(&cid); >+ NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv), "All IPDL streams must report a valid class ID"); >+ >+ cid.ToProvidedString(cidStr); >+ WriteParam(aMsg, nsCAutoString(cidStr)); >+ serializable->Write(aMsg); IPC::InputStream. >+ NS_RELEASE(stream); Won't need the release if you use ObjectAt. Otherwise looks good. Do you think you can update this within the next couple weeks, for beta 2? If not, we can get someone to pick this up and finish it off; it's pretty close. Thanks for the work!

biesi, bz -- question for you from my review comment above: should we fall back to serializing a stream as an nsStringInputStream if it doesn't support nsIIPCSerializable? For instance, IPC:URI does something similar; it serializes by spec & charset. The use case here would be streams implemented by script. I'm not sure if this is a bad idea or not, but the alternative is aborting.

Should all the nsIIPCSerializable::Read implementations be return bool instead of PRBool?

What are the options to serializing as a string stream? That is, what else could we do instead? Seems like the only other option is to fail the message send or kill the child process?

Since we're in Write, which has no failure mode, we'd have to abort. (Or serialize as an empty stream, and lose data.)

Per discussion yesterday, I'd note that this would not apply to any in-tree stream implementations -- all those (as long as they're nonscriptable) will implement nsIIPCSerializable. It's just extensions that write stream impls in script that would be affected.

Comment on attachment 480844 [details] [diff] [review] 4, comment 31 Perfect, r=dwitte. jduell, want to land it?

Comment on attachment 480844 [details] [diff] [review] 4, comment 31 This is dying in many places on tryserver. netwerk/test/httpserver/test/test_body_length.js | test failed: TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/tryserver_fedora64-debug_test-xpcshell/build/xpcshell/tests/netwerk/test/httpserver/test/head_utils.js | testArray[0].initChannel(ch) failed: [Exception... "Cannot modify properties of a WrappedNative" nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)" location: "JS frame :: /home/cltbld/talos-slave/tryserver_fedora64-debug_test-xpcshell/build/xpcshell/tests/netwerk/test/httpserver/test/test_body_length.js :: init_content_length :: line 89" data: no] TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_getshortcutoruri.js | Exception thrown - [Exception... "Cannot modify properties of a WrappedNative" nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)" location: "JS frame :: chrome://browser/content/browser.js :: getPostDataStream :: line 7141" data: no] There are also a *lot* of layout tests failing, but w/o any useful errors. The getPostDataStream error at least looks like a good place to start looking. Nit: > var teststring2 = "--" + BOUNDARY + "\r\n"; FYI, the closing BOUNDARY should also have a '--' appended to it, to comply with the spec. See http://www.w3.org/TR/html401/interact/forms.html I don't think it makes any difference for your test, though, since you're not parsing it with a standard lib or anything, just strcmp'ing it.

FYI: the layout tests are failing with ###!!! [Parent][RPCChannel] Error: Channel error: cannot send/recv which probably means the child process has died? It's not clear to me which process the tests are running in. Some of the tests: REFTEST TEST-UNEXPECTED-FAIL | data:text/plain, | REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora_test-opengl/build/reftest/tests/layout/reftests/reftest-sanity/no-root.html | REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora_test-opengl/build/reftest/tests/layout/reftests/reftest-sanity/blank.html | REFTEST TEST-UNEXPECTED-FAIL | about:blank | REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora_test-opengl/build/reftest/tests/layout/reftests/reftest-sanity/blank.html | REFTEST TEST-UNEXPECTED-FAIL | about:blank | REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora_test-opengl/build/reftest/tests/layout/reftests/reftest-sanity/invalidation.html | REFTEST TEST-UNEXPECTED-FAIL | data:text/plain, | REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora_test-opengl/build/reftest/tests/modules/libpr0n/test/reftest/colordepth.html | REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora_test-opengl/build/reftest/tests/modules/libpr0n/test/reftest/pngsuite-corrupted/wrapper.html?x00n0g01.png | REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora_test-opengl/build/reftest/tests/modules/libpr0n/test/reftest/pngsuite-corrupted/wrapper.html?xcrn0g04.png | REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora_test-opengl/build/reftest/tests/modules/libpr0n/test/reftest/pngsuite-corrupted/wrapper.html?xlfn0g04.png | REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora_test-opengl/build/reftest/tests/layout/reftests/bugs/283686-1.html | REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora_test-opengl/build/reftest/tests/layout/reftests/bugs/283686-3.html | REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora_test-opengl/build/reftest/tests/layout/reftests/bugs/371483-1.html | REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora_test-opengl/build/reftest/tests/layout/reftests/bugs/383035-1.html | REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora_test-opengl/build/reftest/tests/layout/reftests/bugs/383035-2.html | REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora_test-opengl/build/reftest/tests/layout/reftests/bugs/384762-1.html | REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora_test-opengl/build/reftest/tests/layout/reftests/bugs/385533-1.html

var strStream = Cc["@mozilla.org/io/string-input-stream;1"]. createInstance(Ci.nsIStringInputStream); strStream.data = "123"; causes the test failure strStream.setData("123", "123".length); is okay

I added the nit in comment 39. I replaced stringInputStream.data with stringInputStream.setData and I didn't see the test failures in comment 39 and 40.

(In reply to comment #42) > I replaced stringInputStream.data with stringInputStream.setData and I didn't > see the test failures in comment 39 and 40. What if you keep .data= but replace nsIStringInputStream with nsISupportsCString in the createInstance call?

Comment on attachment 481607 [details] [diff] [review] 5 Bug 324058 means that the existing .data calls should have already called nsISupportsCString.data, so biesi's suggestion in comment 43 won't fix things. I think we have a regression here with the nsIClassInfo logic from 324058 being broken somehow, possibly by this patch modifying nsStringInputStream to inherit from nsIClassInfo. (Why did we need that?) Biesi thinks we should get rid of setData (bug 602724), and we want to make sure we're not breaking anything else out there, so let's figure out why .data broke. > +static NS_DEFINE_CID(kStringInputStreamCID, NS_STRINGINPUTSTREAM_CID); Why do we need to define the CID here? Can't we just include nsIStringInputStream.h?

(In reply to comment #43) > What if you keep .data= but replace nsIStringInputStream with > nsISupportsCString in the createInstance call? Most of the tests (xpcshell, reftests) ran fine. I did not see the failures in comment 39 & comment 40. (In reply to comment #44) > I think we have a regression here with the nsIClassInfo logic from 324058 being > broken somehow, possibly by this patch modifying nsStringInputStream to inherit > from nsIClassInfo. (Why did we need that?) The ParamTraits serialization code writes/reads the class cid. > > +static NS_DEFINE_CID(kStringInputStreamCID, NS_STRINGINPUTSTREAM_CID); > > Why do we need to define the CID here? Can't we just include > nsIStringInputStream.h? I need the nsCID-type variable for the GetClassIDNoAlloc function.

test_post.js uses .data instead of setData, and includes the BOUNDARY NIT (comment 39). nsStringInputStream does not inherit from nsIClassInfo anymore (comment 44).

(In reply to comment #46) > nsStringInputStream does not inherit from nsIClassInfo anymore (comment 44). Most classes don't inherit from nsIClassInfo. Nonetheless they support it. See http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsStringStream.cpp#124, note the _CI

Comment on attachment 482351 [details] [diff] [review] 5 Looks good, and the errors are gone from tryserver. (There are other errors, but seem to be unrelated orange). Let's land and see if it sticks.

Arrg, please use this for check-in. 5 has an unnecessary white space here: >--- a/xpcom/io/nsStringStream.cpp >+++ b/xpcom/io/nsStringStream.cpp >@@ -1,9 +1,9 @@ >-/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ >+ /* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ Sorry about that.

Comment on attachment 482937 [details] [diff] [review] 6, check-in please Sorry, I just looked the patch some more, and have a few more questions: So you've changed nsStringInputStream to no longer inherit from nsIClassInfo. But nsFileStreams, nsBufferedInputStream, nsMIMEInputStream, nsMultiplexInputStream still inherit from it. I assume they should all be changed to just use NS_IMPL_QUERY_INTERFACE5_CI? From biesi's comment it sounds like we generally don't/shouldn't inherit from nsIClassInfo. I assume we can also get rid of the NS_DEFINE_CID for these classes, too. Testing: - It would be good to at least manually test this with fennec with a very large file (>50 MB) - the xpcshell test doesn't test nsMIMEInputStream. Is it possible to add? Thanks.

> But nsFileStreams, nsBufferedInputStream, nsMIMEInputStream, > nsMultiplexInputStream still inherit from it. I tried before uploading 5 and got confused with the macros. Do you have any suggestions for NS_IMPL_ISUPPORTS_INHERITED? nsBuffered and nsFile use the macro.

We should use the NS_IMPL_CLASSINFO, NS_IMPL_QUERY_INTERFACEN_CI and NS_IMPL_CI_INTERFACE_GETTERN macros: http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsStringStream.cpp#122 Thanks to biesi for pointing out their existence. :) This will save you from implementing all the CI methods for each class, and won't change how you get the CID in the serializers. Would you like to roll a new patch with those changes? If you can, please post just the changes to your patch (push a new patch onto your mq and make the changes there, that's what I do).

With regard to NS_IMPL_ISUPPORTS_INHERITED: you'll have to manually split out the NS_IMPL_QUERYINTERFACEN_CI macro. In other words, copy this: http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsIClassInfoImpl.h#136 but without the NS_INTERFACE_MAP_ENTRY_AMBIGUOUS line. Also, when you write out the NS_IMPL_CI_INTERFACE_GETTERN macro (http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsStringStream.cpp#129), make sure you explicitly write out the interface(s) that nsBufferedStream and nsFileStream implement. For example, for nsBufferedStream it's just nsISeekableStream. Make sense?

1. nsFileStreams, nsBufferedInputStream, nsMIMEInputStream, nsMultiplexInputStream does not inherit from nsIClassInfo. 2. I have uploaded a 99MB file with fennec. I used an input form (with PHP and IIS) to upload files onto my local dir. 3. test_post.js includes nsIMIMEInputStream.

So I *think* this may be close ready to land. I've run it through try a few times, with various oranges that seem to be random. The only one that worries me is this (on linux and OSX): TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/tryserver_fedora-debug_test-xpcshell/build/xpcshell/tests/netwerk/test/unit/test_file_partial_inputstream.js | test failed (with xpcshell return code: 1), see following log: >>>>>>> ### XPCOM_MEM_LEAK_LOG defined -- logging leaks to /tmp/tmp4CDGzr/runxpcshelltests_leaks.log pldhash: for the table at address 0x8d5dd58, the given entrySize of 48 probably favors chaining over double hashing. nsNativeModuleLoader::LoadModule("/home/cltbld/talos-slave/tryserver_fedora-debug_test-xpcshell/build/firefox/components/libxpcomsample.so") - load FAILED, rv: 80004005, error: /home/cltbld/talos-slave/tryserver_fedora-debug_test-xpcshell/build/firefox/components/libxpcomsample.so: cannot open shared object file: No such file or directory I just tried it on my laptop, and I get an ASSERT fail instead: 0xbfd01b8c "###!!! ASSERTION: failed to delete file: 'NS_SUCCEEDED(rv)', file /home/jduell/central/t/netwerk/base/src/nsFileStreams.cpp, line 277") at /home/jduell/central/t/memory/mozalloc/mozalloc_abort.cpp:75 ... #6 0x011ae23e in nsFileInputStream::Close (this=0xaac4190) at /home/jduell/central/t/netwerk/base/src/nsFileStreams.cpp:277 #7 0x011aeb88 in nsPartialFileInputStream::Read (this=0xaac4190, aBuf=0xaac44a8 "5678901234", aCount=4096, aResult=0xbfd02010) at /home/jduell/central/t/netwerk/base/src/nsFileStreams.cpp:494 If we can fix or verify that this is harmless/random, we should land this.

Comment on attachment 483378 [details] [diff] [review] changes since 6 >diff --git a/netwerk/base/src/nsBufferedStreams.cpp b/netwerk/base/src/nsBufferedStreams.cpp >-static NS_DEFINE_CID(kBufferedInputStreamCID, NS_BUFFEREDINPUTSTREAM_CID); >+NS_IMPL_ADDREF_INHERITED(nsBufferedInputStream, nsBufferedStream) >+NS_IMPL_RELEASE_INHERITED(nsBufferedInputStream, nsBufferedStream) > >-NS_IMPL_ISUPPORTS_INHERITED5(nsBufferedInputStream, >- nsBufferedStream, >+NS_IMPL_CLASSINFO(nsBufferedInputStream, NULL, nsIClassInfo::THREADSAFE, >+ NS_BUFFEREDINPUTSTREAM_CID) >+ >+NS_INTERFACE_MAP_BEGIN(nsBufferedInputStream) >+ NS_INTERFACE_MAP_ENTRY(nsIInputStream) >+ NS_INTERFACE_MAP_ENTRY(nsIBufferedInputStream) >+ NS_INTERFACE_MAP_ENTRY(nsIStreamBufferAccess) >+ NS_INTERFACE_MAP_ENTRY(nsIIPCSerializable) >+ NS_IMPL_QUERY_CLASSINFO(nsBufferedInputStream) >+NS_INTERFACE_MAP_END_INHERITING(nsBufferedStream) >+ >+NS_IMPL_CI_INTERFACE_GETTER4(nsBufferedInputStream, I think you want NS_IMPL_CI_INTERFACE_GETTER5 here, with nsISeekableStream -- nsBufferedStream implements it, so it should be listed in the CI getter. (The way NS_IMPL_ISUPPORTS_INHERITED handles this is by forwarding unknown interface requests to the base class -- nsBufferedStream -- which catches nsISeekableStream and nsISupports QI's. The NS_IMPL_CI_INTERFACE_GETTER macro doesn't know about this, so you have to do it manually.) Same for nsFileInputStream. Rest looks good, r=dwitte with fix. Please upload a combined patch for checkin (you can use 'hg qfold' if you have both patches in your mq).

netwerk/test/unit/test_file_partial_inputstream.js passes on my computer.

Comment on attachment 484225 [details] [diff] [review] 8, comment 56 r=dwitte; pushed to try. Once that's green we can land.

This is crashing on try in test_file_partial_inputstream.js: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1287509626.1287511117.21112.gz Looks like it's crashing in an assert or somesuch, since NS_DebugBreak is on the stack.

Oh, right here: ###!!! ASSERTION: failed to delete file: 'NS_SUCCEEDED(rv)', file /builds/slave/tryserver-linux-debug/build/netwerk/base/src/nsFileStreams.cpp, line 278 Are you running linux?

Found the problem. Patch coming up...

Problem was that we try to delete the file once in Open(), which works fine: then we try to delete it again in Close(), but this time we assert that it succeeded. Which fails because it doesn't exist. I think this is the sanest way to fix it: just clear the flag once we delete it in Open(). I won't expound on how silly I think it is to even be trying that (what if you open two streams and one deletes the file!?), but hey, whatever man.

I landed this, but we got a Windows debug failure: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1287538515.1287540588.9467.gz&fulltext=1 I'll look again tomorrow. I hate this delete-on-close code.

Comment on attachment 484453 [details] [diff] [review] fix delete-on-close test_file_partial_inputstream.js fails after it finishes running when the destructor calls Close() that was already Closed(). Can't we leave this part? - if (mFile && (mBehaviorFlags & DELETE_ON_CLOSE)) { + + if (mBehaviorFlags & DELETE_ON_CLOSE) { rv = mFile->Remove(PR_FALSE); NS_ASSERTION(NS_SUCCEEDED(rv), "failed to delete file"); - // If we don't need to save the file for reopening, free it up - if (!(mBehaviorFlags & REOPEN_ON_REWIND)) { - mFile = nsnull; - } } The test passes on linux and windows.

merged file close patch, fixed bitrot from app cache landing, and sent to tryserver.

Meh--try is still showing one test_file_partial_inputstream.js error, on windows. http://tinderbox.mozilla.org/showlog.cgi?tree=MozillaTry&errorparser=unittest&logfile=1287655678.1287657608.25673.gz&buildtime=1287655678&buildname=WINNT 5.2 tryserver debug test xpcshell&fulltext=1#err0 There are no error messages for any specific tests--just xpcshell returning -2147483645--so I'm not sure what's going on here. Log is not very helpful, and I have no windows box to debug on.

This is the same failure I saw last time. Never fear, I've already got a Windows build spun up and ready to debug. I'll take a look when I get in...

Wait, but attachment 484991 [details] [diff] [review] doesn't incorporate Jae-Seong's suggestion in comment 64. That's why it's still failing... (In reply to comment #64) > test_file_partial_inputstream.js fails after it finishes running when the > destructor calls Close() that was already Closed(). Indeed, if Close() is called twice, that would explain it. > Can't we leave this part? Yeah, that sounds fine. r=me! I'm pushing that change to try now.

Passed and pushed. http://hg.mozilla.org/mozilla-central/rev/b448b7b2e6d5 http://hg.mozilla.org/mozilla-central/rev/a5cbd666cb0a