566577 - e10s: Serialize nsIFileInputStream

Status: RESOLVED WONTFIX

(Core :: Networking: HTTP, defect)

Description

[Jae-Seong Lee-Russo]

From my understanding, I need to access PRFileDesc* mFD to get the file name because nsCOMPtr<nsIFile> mFile is not there all the time.  My questions are, how do I access mFD & how do I get the file name from PRFileDesc?

Comment 1

[Boris Zbarsky [:bzbarsky]]

We could fix things so that we always have an mFile, no?  We certainly do for HTTP form uploads right now (since those are OPEN_ON_REWIND, iirc).

Comment 2

[Jae-Seong Lee-Russo]

Attachment: patch, 0

Comment 3

[Jason Duell]

Comment on attachment 447539 [details] [diff] [review]
patch, 0

> We could fix things so that we always have an mFile, no?

So this patch just assumes the file is there, correct?  Is there something else we need to do to make sure that works?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Is there something else we need to do to make sure that works?

Change nsFileInputStream::Open to always set mFile?

Comment 5

[Jason Duell]

As per bug 564553 comment 6, we can get rid of the buffered stream (is IsNeckoChild() only: leave them for chrome requests) by changing NS_NewPostDataStream() in nsNetUtil.h to take an extra arg (or maybe an enum to replace the isFile arg, with notFile, unbuffered, and buffered values).

Comment 6

[Jason Duell]

(while we're at it, can we remove the "nsIIOService *unused = nsnull" argument, or are the repeated incantations of that in nsNetUtil.h actually serving some purpose?  The nsNetUtil.h API isn't frozen in any way, is it?

Comment 7

[Jae-Seong Lee-Russo]

Attachment: patch, 1

1. Serialize/Deserialize nsIFileInputStream*
2. Change nsFileStreams.cpp to always set mFile
3. comment 5

Comment 8

[Jae-Seong Lee-Russo]

Comment on attachment 448202 [details] [diff] [review]
patch, 1

This patch fails to build after Bug 536273 landed.

Comment 9

[Jae-Seong Lee-Russo]

Attachment: patch, 2

This patch contains a test that changes https://bug566305.bugzilla.mozilla.org/attachment.cgi?id=448358

Comment 10

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

Comment on attachment 448451 [details] [diff] [review]
patch, 2

+++ b/ipc/glue/IPCMessageUtils.h
+template<>
+struct ParamTraits<nsAutoString> : ParamTraits<nsString>

Is this useful? Why not serialize/deserialize nsStrings?

+++ b/netwerk/base/public/nsIFileStreams.idl

this needs a new IID

+++ b/netwerk/base/public/nsNetUtil.h
+typedef enum {
+    UploadStream_notFile = 0,
+    UploadStream_unbuffered,
+    UploadStream_buffered
+} UploadStreamType;

This is C++, not C. Please make this:

enum UploadStreamType {
...
};

Also, necko enums are fairly inconsistent, but please either name these as eUploadStream_NotFile or UPLOADSTREAM_NOTFILE, etc.

I think the names would also be easier to understand if they were eUploadStream_File_Buffered, eUploadStream_File_Unbuffered, eUploadStream_NoFile

+                } else
+                    NS_ADDREF(*result = fileStream);

please put braces around the else-body if you put them around the then-body

+++ b/netwerk/base/src/nsFileStreams.cpp
     // If the file will be reopened on rewind, save the info to open the file
     if (mBehaviorFlags & REOPEN_ON_REWIND) {
-        mFile = aFile;
         mIOFlags = aIOFlags;
         mPerm = aPerm;
     }

at this point I think you can just remove the if and always set mIOFlags/mPerm, that's just two integers.

+    *aFile = mFile;
+    NS_IF_ADDREF(*aFile);

make that NS_ADDREF, you are nullchecking mFile above.

+++ b/netwerk/protocol/http/src/PHttpChannelParams.h
+    nsCOMPtr<nsILocalFile> file =
+      do_CreateInstance("@mozilla.org/file/local;1", &rv);

You can simplify this a bit by using NS_NewLocalFile.

+    rv = stream->Init(file, -1, -1, nsIFileInputStream::CLOSE_ON_EOF |
+                                    nsIFileInputStream::REOPEN_ON_REWIND);

Hmm, don't you want to serialize the flags too?

Comment 11

[Jae-Seong Lee-Russo]

Attachment: patch, 3

Comment 12

[Jae-Seong Lee-Russo]

Attachment: test

Please review test_FileInputStream (the test_String part is reviewed in 566305) or the whole thing if you have time.  I want to make sure the test is correct before adding test_MultiplexInputStream.

Comment 13

[Doug Turner (:dougt)]

Comment on attachment 458609 [details] [diff] [review]
test

applied and ran 7 tests without failure.  looks fine.

Comment 14

[Doug Turner (:dougt)]

Comment on attachment 451206 [details] [diff] [review]
patch, 3

I do not like adding this enum everywhere. I would rather just have a new NS_NewPostDataStream method and safe us from polluting the namespace.

Comment 15

[Jae-Seong Lee-Russo]

Comment on attachment 451206 [details] [diff] [review]
patch, 3

Bug 564553 made a different approach; so this patch is unnecessary and Bug 566305 needs to be backed out.

Comment 16

[[e-mail]]

Without reading the patch in detail, I'm not sure exactly what you mean by "unnecessary". Should we WONTFIX this, or is this something we still want even after bug 564553 lands?

Comment 17

[Jae-Seong Lee-Russo]

Will be fixed by bug 564553