634666 - File streams perform disk I/O when being initialized

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Alon Zakai (:azakai)]

Steps:

1. Install Fennec.
2. Run Fennec.
3. Close Fennec (actually shut it down).
4. Load again.

That load will be much slower than subsequent loads, apparently related to the spinning "Add-ons for your Fennec" thing, during which the chrome process also stutters and stalls (the spinning animation lags, and you cannot interact with chrome during that time).

I see this on a Galaxy S.

mfinkle says that we read from network on first load, then save to disk. So on second load we read from disk - that might be what is slowing us down. (However we also read from disk on subsequent startups. But perhaps Android caches it in memory at that point?)

See point #4 in https://bugzilla.mozilla.org/show_bug.cgi?id=629513#c21

Comment 1

[Alon Zakai (:azakai)]

I was mistaken in the first comment.

1 On first run (after install), we show about:firstrun. On later startups, we show about:home. The addons spinner is on about:home. So, the second run is when we show about:home for the first time.

2. The spinner makes the slowdown noticeable, *but* it happens on later runs as well. It is just not as obvious when there isn't a spinner that lags. To see it, right after load, pan left and right constantly. The lag will appear after ~2 seconds, and last for ~1 second.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

More clarification:  "mfinkle says that we read from network on first load"

We read from network, but not during the actual load. We delay, using a setTimeout, and read from AMO after the page has safely loaded. We then save this cache to disk.

Subsequent loads will read from cache right away.

Comment 3

[Alon Zakai (:azakai)]

Ok, I have narrowed this down to the session store.

Here is data from 2 runs, doing profiling on the first 5 seconds *after* load finishes:

Runnable|./mobile/components/SessionStore.js:378|main         2.4340 total seconds      2.4340 mean seconds           1 counted
Runnable|./mobile/components/SessionStore.js:378|main         2.0550 total seconds      2.0550 mean seconds           1 counted

Perhaps we should move session store disk ops to a side thread?

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

The ~2 second number is odd in that we usually wait a minimum of 2 seconds before writing out any state:

http://mxr.mozilla.org/mobile-browser/source/components/SessionStore.js#375

Also, when we write out the state, we use NetUtil.asyncCopy, which should use a separate thread, right?

http://mxr.mozilla.org/mobile-browser/source/components/SessionStore.js#470

Comment 5

[Alon Zakai (:azakai)]

The cause is

  ostream.init(aFile, 0x02 | 0x08 | 0x20, 0600, 0);

So we do use NetUtil.asyncCopy, however, something significant happens in the init() which is sadly not asynchronous.

Comment 7

[Alon Zakai (:azakai)]

Requesting blocking as a fennec 2.0 blocker has been duped into this.

Comment 8

[Shawn Wilsher :sdwilsh]

Ugh...that's because the Init method of the safe and unsafe file output streams will stat the file at the very least :(
https://hg.mozilla.org/mozilla-central/annotate/cd63bb369f1c/netwerk/base/src/nsFileStreams.cpp#l653
https://hg.mozilla.org/mozilla-central/annotate/cd63bb369f1c/netwerk/base/src/nsFileStreams.cpp#l563

I wonder if we can avoid doing that until we actually do something with the output stream...

FWIW, I don't think this blocks 2.0; we've shipped with this before (although I'd love to see it fixed)

Comment 9

[Alon Zakai (:azakai)]

Hmm, I am seeing that the time is spent in PR_Open() in nsLocalFile::OpenNSPRFileDesc. I am not sure how the the safe and unsafe file output streams fit into the picture here? Does PR_Open call them?

Comment 10

[Alon Zakai (:azakai)]

Nevermind, I got that backwards, they call OpenNSPRFileDesc which calls PR_Open, which is what I was looking at.

So, looks like it would be good to delay the PR_Open so it runs in a background thread.

Comment 11

[Shawn Wilsher :sdwilsh]

(In reply to comment #10)
> So, looks like it would be good to delay the PR_Open so it runs in a background
> thread.
We probably want to defer that work until we call ::Write for both classes.

Comment 12

[Alon Zakai (:azakai)]

Just out of curiosity, I dumped the time we spend doing PR_Open on all files. We can end up spending a lot of time in not just the session store, but others. Here is data from 3 first runs (each after a fresh install):


I/Gecko   ( 6004): |Opening Unix file '/data/data/org.mozilla.fennec/files/mozilla/a9m6gidu.default/autocomplete.json' took 2.47300 seconds, mainThread == 1

...
I/Gecko   ( 6004): |Opening Unix file '/data/data/org.mozilla.fennec/files/mozilla/a9m6gidu.default/sessionstore.js' took 0.00200 seconds, mainThread == 1


---

I/Gecko   ( 6058): |Opening Unix file '/data/data/org.mozilla.fennec/files/mozilla/1p2x8f58.default/autocomplete.json' took 0.13200 seconds, mainThread == 1

...
I/Gecko   ( 6058): |Opening Unix file '/data/data/org.mozilla.fennec/files/mozilla/1p2x8f58.default/sessionstore.js' took 1.32000 seconds, mainThread == 1


---

I/Gecko   ( 6108): |Opening Unix file '/data/data/org.mozilla.fennec/files/mozilla/6bhc8090.default/autocomplete.json' took 0.00600 seconds, mainThread == 1

...
I/Gecko   ( 6108): |Opening Unix file '/data/data/org.mozilla.fennec/files/mozilla/6bhc8090.default/sessionstore.js' took 0.48600 seconds, mainThread == 1



So this is unpredictable. Perhaps the filesystem is busy with other stuff for other reasons (maybe even our own stuff, on another thread), and if we are unlucky in our timing, we end up blocking for a while in PR_Open.

Comment 13

[Alon Zakai (:azakai)]

Attachment: potential patch

Patch that defers OpenNSPRFileDesc until we actually write (or flush). Fixes the problem, Fennec remains nice and responsive after startup. (Out of curiosity I checked the time, and opening the file still takes ~1 second like before (but it's now in a background thread))

My only concern here is that we are slightly changing the behavior, in that actually doing PR_Open and creating the file is deferred to when we actually need it. So if you just Init a file, and expect that to tell you if there is a problem with creating it (no parent directory, invalid filename, etc.), then this will no longer work with this approach. The error will appear only when doing an actual write. Also, looking for the file will not find it on the disk yet.

So if there is something in the code that relies on the those things, that might be a problem. (We actually used to have such a thing in Fennec frontend code, but recently removed it.) The fix for such places would be simple (for example, do a flush), but the question is how easy it is to find such things, if they are out there. And also whether this is the right approach.

Alternatively, we can add an option "allow deferred open" somehow in the interface (/in a new interface), or something like that.

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #13)
> 
> Alternatively, we can add an option "allow deferred open" somehow in the
> interface (/in a new interface), or something like that.

To that point, I think this new behavior should be controlled by an additional behavior flag. Perhaps OPEN_ON_WRITE? I also worry about changing behavior across the board this late in the game for both Firefox and Fennec.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

I like the idea of a new behavior flag. It won't even break the interface.

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> I like the idea of a new behavior flag. It won't even break the interface.
...and we should fix everything in the tree that uses this in one go (I think most everything that uses the safe output stream has been made async at this point).

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

As per sdwilsh!

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Alon - Please continue with any patches though. This still blocks Fennec.

Comment 19

[Shawn Wilsher :sdwilsh]

Comment on attachment 513331 [details] [diff] [review]
potential patch

>+++ b/netwerk/base/src/nsFileStreams.cpp
note that nsFileInputStream and nsPartialFileInputStream suffer the same fate here and needs to be fixed.

> NS_IMETHODIMP
> nsFileOutputStream::Init(nsIFile* file, PRInt32 ioFlags, PRInt32 perm,
>                          PRInt32 behaviorFlags)
> {
>-    NS_ENSURE_TRUE(mFD == nsnull, NS_ERROR_ALREADY_INITIALIZED);
>+    NS_ENSURE_TRUE(mFD == nsnull && !mPendingInit, NS_ERROR_ALREADY_INITIALIZED);
Given how often you use a similar check, can I suggest that you make a helper method:
bool IsInitialized() const
{
    return !mFD && !mPendingInit;
}

Additionally, if we want to take this for 2.0 (which I strongly suspect we do), we'll want to go with the approach in comment 15.  Not sure where we want to toss that flag, but I'm sure bz has an opinion.

>+    mPendingInit = PR_TRUE; // No longer a partial Init in motion
note that the comment doesn't seem terribly useful here

>+#define COMPLETE_PARTIAL_INIT \
>+    if (mPendingInit && !mFD) { \
>+        nsresult rv = FinishInit(); \
>+        NS_ENSURE_SUCCESS(rv, rv); \
>+    }
This is generally frowned upon in our codebase.  Please just move the code to each method.

>+++ b/netwerk/base/src/nsFileStreams.h
>+protected:
>+    PRPackedBool mPendingInit : 1;
note that using PRPackedBool here isn't buying us anything.  Just use bool please.

If we want this to land in 2.0, we'll also need to add some tests for this.  I'm not sure if we can do this, but wrapping PR_Open (or whatever other methods we care about) and tracking what thread they run on in a cpp test would be the best way to do this (see https://mxr.mozilla.org/mozilla-central/source/storage/test/test_true_async.cpp for a similar type of test).  Also need to ensure that we have exists tests for this streams.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Yeah, changing default behavior is bad because it introduces possible races.  As things stand, once init() returns you know you have the file open and can write to it; something that you won't know anymore with the lazy-init.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Oh, the question was where to put the flag.  nsIFileOutputStream seems like the obvious place to me (similar to the nsIFileInputStream flags).

Comment 22

[Alon Zakai (:azakai)]

(In reply to comment #21)
> Oh, the question was where to put the flag.  nsIFileOutputStream seems like the
> obvious place to me (similar to the nsIFileInputStream flags).

In comment #19 sdwilsh asks for this to work in both input and output streams. So should the flag be in nsISeekableStream (what nsFileStream, the shared parent of input and output streams, implements)?

Comment 23

[Boris Zbarsky [:bzbarsky]]

This flag doesn't make any sense for general seekable streams.

Comment 24

[Boris Zbarsky [:bzbarsky]]

And for input stream things are even more worrying than for output stream.  You might get totally different data depending on when the open happens, so you would have to be _very_ careful using this flag.

Comment 25

[Shawn Wilsher :sdwilsh]

(In reply to comment #24)
> And for input stream things are even more worrying than for output stream.  You
> might get totally different data depending on when the open happens, so you
> would have to be _very_ careful using this flag.
Could you elaborate?  I'm not sure I understand how things might be different.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Consider the following sequence of operations:

1)  Init a file input stream (stream1) with file X.
2)  Remove file X.
3)  Create a new file named X with different data.

When you read from stream 1, will you get the original data or the new data?  In the old setup, the original (and step 2 would fail to remove the file on Windows).  With the new flag, it depends on how step 1 races against steps 2+3.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Er, sorry depends on how the _first_read_ races against steps 2+3.  And since they're probably on different threads, you have very little control over it.

Comment 28

[Shawn Wilsher :sdwilsh]

Sure, but is that case common?  Regardless, I think we should add that flag for file input streams since these days what we really really want is to do all disk access (even calling PR_Open) off of the GUI thread.  We certainly don't need to make it the default.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> Sure, but is that case common?  

Probably not.  

I'm fine with adding the flag; we just need to clearly document what guarantees are offered without the flag and what guarantees are offered with the flag.

Comment 30

[Alon Zakai (:azakai)]

Attachment: patch

Here is a patch.

I added the flag in a new interface, nsIFileStream, used just for shared flags for input and output streams. Make sense?

Comment 31

[Boris Zbarsky [:bzbarsky]]

I'll try to take a look at the details on Tuesday, but for now I just wanted to say that I don't think we want a new interface just for this.  Please just add the flags to the two existing file stream interfaces.

Comment 32

[Alon Zakai (:azakai)]

Attachment: patch v2

Changed the interfaces as requested.

Comment 33

[Alon Zakai (:azakai)]

Attachment: mobile-browser patch

mobile-browser part.

These three places are the ones that use NetUtil.asyncCopy, AFAICT the new option should work fine in them.

Aside from that, we can also use this in NetUtil.asyncFetch - not sure if the benefit is significant, though (can't see one in my data). Maybe still worth doing though? Seems to be one place in mobile-browser that uses it (in AutoCompleteCache.js).

Comment 34

[Shawn Wilsher :sdwilsh]

Comment on attachment 513591 [details] [diff] [review]
mobile-browser patch

>+++ b/components/AddonUpdateService.js
>     let ostream = Cc["@mozilla.org/network/safe-file-output-stream;1"].createInstance(Ci.nsIFileOutputStream);
>-    ostream.init(aFile, 0x02 | 0x08 | 0x20, 0600, 0);
>+    ostream.init(aFile, 0x02 | 0x08 | 0x20, 0600, ostream.OPEN_WHEN_NEEDED);

>+++ b/components/AutoCompleteCache.js
>     let ostream = Cc["@mozilla.org/network/file-output-stream;1"].createInstance(Ci.nsIFileOutputStream);
>     let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].createInstance(Ci.nsIScriptableUnicodeConverter);
> 
>     try {
>-      ostream.init(this.cacheFile, (MODE_WRONLY | MODE_CREATE | MODE_TRUNCATE), PERMS_FILE, 0);
>+      ostream.init(this.cacheFile, (MODE_WRONLY | MODE_CREATE | MODE_TRUNCATE), PERMS_FILE, ostream.OPEN_WHEN_NEEDED);

>+++ b/components/SessionStore.js
>     // Initialize the file output stream.
>     let ostream = Cc["@mozilla.org/network/safe-file-output-stream;1"].createInstance(Ci.nsIFileOutputStream);
>-    ostream.init(aFile, 0x02 | 0x08 | 0x20, 0600, 0);
>+    ostream.init(aFile, 0x02 | 0x08 | 0x20, 0600, ostream.OPEN_WHEN_NEEDED);
Note that it might be worthwhile for all of these to use FileUtils.openSafeFileOutputStream, although that would require some minor changes there.

Comment 35

[Shawn Wilsher :sdwilsh]

Comment on attachment 513587 [details] [diff] [review]
patch v2

Note: we don't seem to have any tests for the normal file output stream, only one test for the safe file output stream (dealing with unicode and added in bug 629291), and no tests for file input streams.  A number of tests do use them though, so we /may/ have sufficient coverage there.  At the very least, we need to add tests to make sure that basic functionality still works with our new flag, and ideally I'd like to see tests like I described in comment 19.

>+++ b/netwerk/base/public/nsIFileStreams.idl
>+    /**
>+     * If this is set, the file will be actually opened (i.e., a call to
>+     * PR_Open done) only when actually needed, that is, when we do an
nit: drop both uses of actually here

>+     * actual read, write, flush, etc. to the file. In other words, we
>+     * defer the actual open to when it is required. This is useful if the
>+     * actual activity happens later, for example on a background thread,
>+     * as it makes the PR_Open call also be done there.
nit: drop both uses of actual here, and suggesting alternate phrasing:
"...if the operation happens on a background thread so the opening and possible stating of the file happens there as well."


>+     * Note that deferring the actual open has the effect of nothing
>+     * changing on disk until something is actually done with the stream.
>+     * For example, just calling Init on a file will not actually try
>+     * to open it for reading, so if there is a problem (invalid filename,
>+     * etc.) then the error will only occur when you actually read.
nit: use @note and suggesting alternate phrasing:
@note using this flag results in the file not being opened during the call to init.  This means that any errors that might happen when this flag is not set would happen when during the first read.

(Note, I'm making this comment specific to input streams.  We should make a comment specific to output streams below too.)

nit: this file appears to use two spaces after a period.  This whole uses just one.

>+    const long OPEN_WHEN_NEEDED = 1<<4;
I think I'd prefer DEFER_OPEN, but bz has the ultimate say here.

>+    /**
>+     * See the same constant in nsIFileInputStream. Note that the
>+     * value here must exactly match the one there.
>+     */
We should add a PR_STATIC_ASSERT in the implementation file that these two constants are the same, and lose the text about it.

>+++ b/netwerk/base/src/nsFileStreams.h
>     nsresult InitWithFileDescriptor(PRFileDesc* fd, nsISupports* parent);
> 
> protected:
>     PRFileDesc*           mFD;
>     nsCOMPtr<nsISupports> mParent; // strong reference to parent nsFileIO,
>                                    // which ensures mFD remains valid.
While we are here...InitWithFileDescriptor and mParent aren't actually used anywhere.  Remove them please so we have a smaller cost of adding more variables to this class.

>     PRBool                mCloseFD;
>+
>+    // Flags describing our behavior.  See the IDL file for possible values.
>+    PRInt32 mBehaviorFlags;
>+
>+    // Whether we have a pending open (see OPEN_WHEN_NEEDED).
>+    PRBool mPendingOpen;
Move this to be below mCloseFD, and make them both PRPackedBools.

>+    // If there is a pending open, do it now. It's important for this to be
>+    // inline since we do it in almost every call in a stream.
>+    nsresult DoPendingOpen()
>+    {
>+        if (!mPendingOpen) return NS_OK;
>+
>+        nsresult rv = DoOpen(mLocalFile, mIoFlags, mPerm, PR_TRUE);
>+        CancelPendingOpen();
>+        return rv;
>+    }
Please don't implement this in the header file.

I haven't had a change to look at the cpp file yet, but out of time now.

Comment 36

[Alon Zakai (:azakai)]

Attachment: wip tests

First version of tests. Not finished yet, not ready for review.

Comment 37

[Alon Zakai (:azakai)]

Attachment: wip m-c applications

WIP applications of deferred open in 3 places in m-c (all the places that use NetUtil.asyncCopy). Not fully tested, not ready for review.

Comment 38

[Alon Zakai (:azakai)]

Thanks for the comments sdwilsh, I'll post an updated patch sometime during the weekend.

Comment 39

[Alon Zakai (:azakai)]

Attachment: Part 1: DEFER_OPEN option

Updated patch following sdwilsh's comments.

Comment 40

[Alon Zakai (:azakai)]

Attachment: Part 2: Tests

Tests.

The existing tests that use nsIFileInputStream and nsIFileOutputStream are modified so that they run twice, once with deferring and once without.

In addition I added a test_DEFER_OPEN test, that specifically checks for the behavior we want: DEFER_OPEN on output streams does not create the file when we do init(), but we still do so without DEFER_OPEN, and DEFER_OPEN on input streams does not return an error on init() if the filename is invalid (but only when we actually read), but we still get the error without DEFER_OPEN. So we check for the behavior of only doing disk operations when needed when DEFER_OPEN is set.

I needed to make some changes to the test file:

1. The same server port was used more than once, and after adding my test other tests in this file started to fail intermittently on tryserver. Using a unique port each time fixes that.

2. The order in which do_execute_soon functions execute is no longer reliable after adding my test (timings are different I guess), so more care is needed with the test index to avoid intermittent problems with the last test.

Passes try ok, and all other tryserver tests as well.

Comment 41

[Alon Zakai (:azakai)]

Attachment: Part 3: m-c applications of DEFER_OPEN

Applications in m-c of DEFER_OPEN. Passes try ok. Not asking for review yet since not sure if desktop want to take this, or wait for after 4.0.

Comment 42

[Alon Zakai (:azakai)]

Attachment: Part 4: m-b applications of DEFER_OPEN

Updated mobile-browser patch. Carried r+.

Comment 43

[Shawn Wilsher :sdwilsh]

Comment on attachment 514096 [details] [diff] [review]
Part 3: m-c applications of DEFER_OPEN

>+++ b/services/sync/modules/util.js
These changes need to be pulled into a seperate patch.  They use their own repo, and will want it to land there.  This code is used by the mobile browser, so you'll want it patched as well.

r=me on everything else

Comment 44

[Shawn Wilsher :sdwilsh]

Comment on attachment 514089 [details] [diff] [review]
Part 1: DEFER_OPEN option

>+++ b/netwerk/base/public/nsIFileStreams.idl
>+     * @note Using this flag results in the file not being opened
>+     * during the call to init.  This means that any errors that might
>+     * happen when this flag is not set would happen when during the first
>+     * read.
nit: text should line up with "Using" and not @note here

>+     * @note Using this flag results in the file not being opened
>+     * during the call to init.  This means that any errors that might
>+     * happen when this flag is not set would happen when during the first
>+     * write, and if the file is to be created, then it will not
>+     * appear on the disk until the first write.
ditto

>+++ b/netwerk/base/src/nsFileStreams.cpp
> nsFileStream::nsFileStream()
>     : mFD(nsnull)
>     , mCloseFD(PR_TRUE)
>+    , mBehaviorFlags(0)
>+    , mPendingOpen(PR_FALSE)
You have the wrong order here (this should be generating a warning too).  Also, mCloseFD needs to be set to PR_FALSE now, and we should set it to PR_TRUE when we actually open the file.

Although...we don't even need mCloseFD anymore.  It was used with InitWithFileDescriptor only.  We can just check mFD now.

> nsFileStream::~nsFileStream()
> {
>     if (mCloseFD)
>         Close();
>+
>+    CancelPendingOpen();
Close() will do this for us.  It also checks mFD, so we don't even need an if here.

>+nsresult
>+nsFileStream::DoOpen(nsILocalFile* aFile, PRInt32 aIoFlags, PRInt32 aPerm,
>+                     PRBool aForce)
s/PRBool/bool/

>+    if (mBehaviorFlags & nsIFileInputStream::DEFER_OPEN && !aForce) {
>+        mPendingOpen = PR_TRUE;
>+        mIoFlags = aIoFlags;
>+        mPerm = aPerm;
>+        mLocalFile = aFile;
We actually want to Clone aFile, otherwise someone could change the nsIFile that we hold a reference to before we open the file (we should test this too).

>+    PRFileDesc* fd;
>+    nsresult rv = aFile->OpenNSPRFileDesc(aIoFlags, aPerm, &fd);
>+    if (NS_FAILED(rv)) return rv;
nit: NS_ENSURE_SUCCESS here

>+nsresult
>+nsFileStream::DoPendingOpen()
>+{
>+    if (!mPendingOpen) return NS_OK;
please put the return on a newline and brace one-line ifs per coding style

>+nsresult
>+nsFileStream::CancelPendingOpen()
>+{
>+    mPendingOpen = PR_FALSE;
>+    mLocalFile = nsnull;
>+
>+    return NS_OK;
>+}
This can just return void instead of an nsresult

> nsFileInputStream::Write(IPC::Message *aMsg)
> {
>+    nsresult rv = DoPendingOpen();
>+    if (!NS_SUCCEEDED(rv))
>+        return;
nit: brace one-line if

> nsFileOutputStream::Create(nsISupports *aOuter, REFNSIID aIID, void **aResult)
> {
>+    PR_STATIC_ASSERT(DEFER_OPEN == nsIFileInputStream::DEFER_OPEN);
You should just put this right at the top of the file after the includes:
PR_STATIC_ASSERT(nsIFileInputStream::DEFER_OPEN == nsIFileOutputStream::DEFER_OPEN);

> }
> 
>+
> NS_IMETHODIMP
> nsSafeFileOutputStream::Write(const char *buf, PRUint32 count, PRUint32 *result)
nit: you added a newline here you don't want

>+++ b/netwerk/base/src/nsFileStreams.h
>+    PRFileDesc* mFD;
>+    PRPackedBool mCloseFD : 1;
This is going away...

>+    // Whether we have a pending open (see DEFER_OPEN).
>+    PRPackedBool mPendingOpen : 1;
...which means this can just be a bool now.

>+    // Opens the file, either right now, or deferred to later if
>+    // DEFER_OPEN is used in this stream.
>+    // @param aForce Does an open right now, even if we do use DEFER_OPEN
If we are going to kind of do java-doc comments, let's go all the way please (formatted like https://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#63)

>+    // If there is a pending open, cancel it and clean it up.
>+    nsresult CancelPendingOpen();
Should be void per comments above.

r=sdwilsh

This patch is full of awesome.

Comment 45

[Alon Zakai (:azakai)]

Oops, it looks like some additional deferring is needed for safe output streams - or else they do some file IO on init, even with DEFER_OPEN. Noticed that when testing on device just now (2 second stall on the main thread... grr).

I'm working on a patch now, should be ready soon. I'll also add in the changes requested in the comments above (thanks!).

Comment 46

[Shawn Wilsher :sdwilsh]

Comment on attachment 514093 [details] [diff] [review]
Part 2: Tests

>+++ b/netwerk/test/unit/test_NetUtil.js
> /**
>  * Reads the contents of a file and returns it as a string.
>  *
>  * @param aFile
>  *        The file to return from.
>  * @return the contents of the file in the form of a string.
>  */
>-function getFileContents(aFile)
>+function getFileContents(aFile, aBehaviorFlags)
Please also update the java-doc comment here.  It should be tagged with [optional] like here: https://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#225

>-function test_async_write_file()
>+function async_write_file(aBehaviorFlags)
...
>-function test_async_write_file_nsISafeOutputStream()
>+function async_write_file_nsISafeOutputStream(aBehaviorFlags)
Both of these need to move up to the "Helper Methods" section since they are no longer actual tests.

>+function async_write_file(aBehaviorFlags)
>     // Check the file contents.
>-    do_check_eq(TEST_DATA, getFileContents(file));
>+    do_check_eq(TEST_DATA, getFileContents(file, aBehaviorFlags));
You don't need to actually do this since it's a synchronous read, right?

>+function async_write_file_nsISafeOutputStream(aBehaviorFlags)
>     // Check the file contents.
>-    do_check_eq(TEST_DATA, getFileContents(file));
>+    do_check_eq(TEST_DATA, getFileContents(file, aBehaviorFlags));
ditto

>+function test_async_write_file() {
>+  [0, Ci.nsIFileOutputStream.DEFER_OPEN].forEach(async_write_file);
>+}
Ah, this is why the issues you described in comment 40 happened.  What you really want here is to keep the old method, and make a new one:
function test_async_write_file()
{
  async_write_file(Ci.nsIFileOutputStream.DEFER_OPEN);
}

>+function test_async_write_file_nsISafeOutputStream() {
ditto here

>+// DEFER_OPEN files should only be opened when actually written to, as
>+// we defer the call to Open to later, and so forth. Note that for writing,
>+// we check for actual writing in test_async_write_file_*, here we just
>+// check that the file is *not* created during init.
>+function test_DEFER_OPEN() {
This test don't really belong on test_NetUtil.js as it doesn't have anything to do with NetUtil.  I guess we want a new test_filestreams.js for these.  You'll also want to test the writing bits of nsIFileOutputStream when it is a safe file output stream (we have a test file for those).

>   // Start the http server, and register our handler.
>   let server = new nsHttpServer();
>   server.registerPathHandler("/test", function(aRequest, aResponse) {
>     aResponse.setStatusLine(aRequest.httpVersion, 200, "OK");
>     aResponse.setHeader("Content-Type", "text/plain", false);
>     aResponse.write(TEST_DATA);
>   });
>-  server.start(4444);
>+  server.start(5555);
> 
>   // Create our URI.
>-  let uri = NetUtil.newURI("http://localhost:4444/test");
>+  let uri = NetUtil.newURI("http://localhost:5555/test");
You shouldn't need to do this with the changes mentioned above.

>   // Start the http server, and register our handler.
>   let server = new nsHttpServer();
>   server.registerPathHandler("/test", function(aRequest, aResponse) {
>     aResponse.setStatusLine(aRequest.httpVersion, 200, "OK");
>     aResponse.setHeader("Content-Type", "text/plain", false);
>     aResponse.write(TEST_DATA);
>   });
>-  server.start(4444);
>+  server.start(6666);
> 
>   // Open our location asynchronously.
>-  NetUtil.asyncFetch("http://localhost:4444/test", function(aInputStream,
>+  NetUtil.asyncFetch("http://localhost:6666/test", function(aInputStream,
>                                                             aResult) {
ditto

>-function test_asyncFetch_with_nsIFile()
>+function asyncFetch_with_nsIFile(aBehaviorFlags)
This needs to move to the "Helper Methods" section too.

>   // Sanity check to make sure the data was written.
>-  do_check_eq(TEST_DATA, getFileContents(file));
>+  do_check_eq(TEST_DATA, getFileContents(file, aBehaviorFlags));
You don't want to pass aBehaviorFlags here

>+function test_asyncFetch_with_nsIFile() {
>+  [0, Ci.nsIFileOutputStream.DEFER_OPEN].forEach(asyncFetch_with_nsIFile);
>+}
Same as above; you want to make to test functions for this so it doesn't break run_next_test

> function run_next_test()
> {
>   if (index < tests.length) {
>     do_test_pending();
> 
>     // Asynchronous test exceptions do not kill the test...
>     do_execute_soon(function() {
>-      try {
>-        print("Running the next test: " + tests[index].name);
>-        tests[index++]();
>-      }
>-      catch (e) {
>-        do_throw(e);
>+      if (index < tests.length) {
>+        var test = tests[index++];
>+        try {
>+          print("Running the next test: " + test.name);
>+          test();
>+        }
>+        catch (e) {
>+          do_throw(e);
>+        }
You shouldn't have to do this with the changes mentioned above.  The problem was that you were getting more than one test running at once (which makes debugging really hard when things fail).  If you feel ambitious, you can copy the run_next_test implementation from https://hg.mozilla.org/mozilla-central/annotate/1da3405c74fd/toolkit/components/places/tests/head_common.js#l650 and rename |tests| to gTests.  You don't have to do that though (we are going to pull that method into head.js for all of xpcshell to use eventually, so I'm slowly trying to standardize on it to make that task easier).

I want to see this one more time, hence the r-, but it's looking good!

Comment 47

[Brad Lassey [:blassey] (use needinfo?)]

sorry for the spam, changing hours estimates requires a comment

Comment 48

[Alon Zakai (:azakai)]

Finishing up a patch now. But two things:

> >+function async_write_file(aBehaviorFlags)
> >     // Check the file contents.
> >-    do_check_eq(TEST_DATA, getFileContents(file));
> >+    do_check_eq(TEST_DATA, getFileContents(file, aBehaviorFlags));
> You don't need to actually do this since it's a synchronous read, right?

Not in particular, however, I do think there is value in testing it once without DEFER_OPEN and once with - just to check for breakage even in synchronous operations when using DEFER_OPEN.

> You'll
> also want to test the writing bits of nsIFileOutputStream when it is a safe
> file output stream (we have a test file for those).

test_NetUtils.js does test writing of nsISafeFileOutputStream, in async_write_file_nsISafeOutputStream?

Comment 49

[Alon Zakai (:azakai)]

Attachment: Part 1: DEFER_OPEN option v2

Updated patch. Changes:

1. sdwilsh's comments.
2. Additional code to entirely defer safe output streams.

Comment 50

[Shawn Wilsher :sdwilsh]

(In reply to comment #48)
> Not in particular, however, I do think there is value in testing it once
> without DEFER_OPEN and once with - just to check for breakage even in
> synchronous operations when using DEFER_OPEN.
I would really rather test that in a different file.  I'm a big fan of tests being simple and contained so that when they fail they are easy to debug.  Big tests that test the world get you test coverage, but when they break, they take a lot longer to debug usually.

> test_NetUtils.js does test writing of nsISafeFileOutputStream, in
> async_write_file_nsISafeOutputStream?
We should get some synchronous writing tests too.

Comment 51

[Alon Zakai (:azakai)]

Attachment: Part 2: Tests v2

Updated tests following comments.

Comment 52

[Alon Zakai (:azakai)]

Attachment: Part 2: Tests v3

Updated tests, all comments.

Comment 53

[Alon Zakai (:azakai)]

Attachment: Part 5: services/sync application of DEFER_OPEN

Patch for services/sync, split out following comment #43.

Anybody know who I should ask to review this? I don't see anything in the module owners list.

Comment 54

[Philipp von Weitershausen [:philikon]]

Comment on attachment 514644 [details] [diff] [review]
Part 5: services/sync application of DEFER_OPEN

DEFER_COUNT won't be available on 3.5/3.6 which Sync still supports for now. Suggest doing fos.DEFER_OPEN || 0.

Comment 55

[Alon Zakai (:azakai)]

Attachment: Part 5: services/sync application of DEFER_OPEN v2

That sounds good, here's an updated patch.

Comment 56

[Philipp von Weitershausen [:philikon]]

Comment on attachment 514895 [details] [diff] [review]
Part 5: services/sync application of DEFER_OPEN v2

This should go to fx-sync. We'll then catch it in our next merge. I can land it there for you if you want.

Comment 57

[Alon Zakai (:azakai)]

(In reply to comment #56)
> Comment on attachment 514895 [details] [diff] [review]
> Part 5: services/sync application of DEFER_OPEN v2
> 
> This should go to fx-sync. We'll then catch it in our next merge. I can land it
> there for you if you want.

That would be great, thanks. The only thing this needs to wait on is to make sure that bz is ok with naming the constant DEFER_OPEN. Otherwise this can land before any of the other stuff here.

Comment 58

[Shawn Wilsher :sdwilsh]

Comment on attachment 514627 [details] [diff] [review]
Part 1: DEFER_OPEN option v2

>+++ b/netwerk/base/src/nsFileStreams.cpp
> nsFileStream::nsFileStream()
>     : mFD(nsnull)
>-    , mCloseFD(PR_TRUE)
>+    , mBehaviorFlags(0)
>+    , mPendingOpen(PR_FALSE)
nit: false

>+nsresult
>+nsFileStream::MaybeOpen(nsILocalFile* aFile, PRInt32 aIoFlags, PRInt32 aPerm)
>+        nsCOMPtr<nsILocalFile> localFile = do_QueryInterface(file, &rv);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+        mOpenParams.mLocalFile = localFile;
Just do mOpenParams.mLocalFile = do_QueryInterface(file);
NS_ENSURE_TRUE(mOpenParams.mLocalFile, NS_ERROR_UNEXPECTED);

>+        mPendingOpen = PR_TRUE;
nit: true

+nsresult
+nsFileStream::DoOpen()
+{
Please NS_PRECONDITION here that mOpenParams.mLocalFile is non-null

>+void
>+nsFileStream::CancelPendingOpen()
>+{
>+    mPendingOpen = PR_FALSE;
nit: false

>+nsresult
>+nsSafeFileOutputStream::DoOpen()
>+{    
>+    nsCOMPtr<nsILocalFile> file = mOpenParams.mLocalFile;
>+    mOpenParams.mLocalFile = nsnull;
You really just want to do file.swap(mOpenParams.mLocalFile) here.  Also, please add a comment about why we throw away mLocalFile here (as far as I can tell, it's because we need to follow symlinks, which is explained lower, but having a comment here would be incredibly useful).

>+++ b/netwerk/base/src/nsFileStreams.h
>+    /**
>+     * Whether we have a pending open (see DEFER_OPEN in the IDL file).
>+     */
>+    PRBool mPendingOpen;
nit: bool

>+    struct OpenParams {
>+        nsCOMPtr<nsILocalFile> mLocalFile;
>+        PRInt32 mIoFlags;
>+        PRInt32 mPerm;
>+    };
these shouldn't have an mPrefix

r=sdwilsh

Comment 59

[Alon Zakai (:azakai)]

Attachment: Part 1: DEFER_OPEN option v3

Changes for sdwilsh's last comments.

Comment 60

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 514970 [details] [diff] [review]
Part 1: DEFER_OPEN option v3

>@@ -76,16 +76,31 @@ interface nsIFileInputStream : nsIInputS
>+     * If this is set, the file will be opened (i.e., a call to
>+     * PR_Open done) only when needed, that is, when we do an
>+     * actual read, write, flush, etc. to the file.

Instead of "etc", you should probably list the exact set of operations that will cause the PR_Open.

So something like ".... only when one of the following functions is called: ...."

At least list the nsIInputStream functions that will cause the open call here.

>  In other words, we
>+     * defer the actual open to when it is required.  This is useful if
>+     * the operation happens on a background thread so the opening and
>+     * possible |stat|ing of the file happens there as well

Ditch the "In other words" sentence.

>+     * @note Using this flag results in the file not being opened
>+     *       during the call to init.  This means that any errors that might
>+     *       happen when this flag is not set would happen when during the
>+     *       first read.

s/when during/during/.

Also, worth noting that this means the file does not get locked during init() and might disappear before we try to read for it.

> interface nsIFileOutputStream : nsIOutputStream
>+    /**
>+     * See the same constant in nsIFileInputStream.

This should document what will cause the file to be opened.

>+     *
>+     * @note Using this flag results in the file not being opened
>+     *       during the call to init.  This means that any errors that might
>+     *       happen when this flag is not set would happen when during the

s/when during/during/

>+     *       first write, and if the file is to be created, then it will not
>+     *       appear on the disk until the first write.
>+     */
>+    const long DEFER_OPEN = 1<<4;

Why isn't this |1 << 0|?  Just so MaybeOpen() can be shared between the two streams?  I'd prefer that we just pass the "defer" boolean to MaybeOpen from the two callsites instead.

>+++ b/netwerk/base/src/nsFileStreams.cpp
>+    , mPendingOpen(false)

I'd call this mDeferredOpen.  Or mHavePendingOpen, perhaps?

>+nsFileStream::DoOpen()

This should null out mOpenParams.localFile before returning; otherwise we'll keep it alive in the not-deferred case, since nothing nulls it out in that case.

> nsFileInputStream::Read(const IPC::Message *aMsg, void **aIter)
> {
>+    nsresult rv = DoPendingOpen();
>+    NS_ENSURE_SUCCESS(rv, rv);
>+

This change makes no sense.  Please revert it?

> void
> nsFileInputStream::Write(IPC::Message *aMsg)
> {
>+    nsresult rv = DoPendingOpen();
>+    if (!NS_SUCCEEDED(rv)) {
>+        return;
>+    }
>+

Likewise.

> nsSafeFileOutputStream::Write(const char *buf, PRUint32 count, PRUint32 *result)
> {
>-    nsresult rv = nsFileOutputStream::Write(buf, count, result);
>+    nsresult rv = DoPendingOpen();
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    rv = nsFileOutputStream::Write(buf, count, result);

This change seems to be unnecessary, since nsFileOutputStream::Write will DoPendingOpen itself.  Take it out?

>+++ b/netwerk/base/src/nsFileStreams.h
>+     * Variables for the DEFER_OPEN case, that store data we need

Nix the comma there.  Or perhaps just rewrite as:

  Data we need to do a deferred open.

>+     * Potentially opens the file, either right now, or deferred to later if
>+     * DEFER_OPEN is used in this stream.
>+     */
>+    nsresult MaybeOpen(nsILocalFile* aFile, PRInt32 aIoFlags, PRInt32 aPerm);

How about:

  Either open the file, or if DEFER_OPEN was passed to init
  store the information needed to do the open later.

>+     * Open the file. This is called either from MaybeOpen (during Init)
>+     * or from DoPendingOpen (if DEFER_OPEN is used in this stream).

s/in/when initializing/

>+     * The default behavior in DoOpen is to open the file and save the

s/in/of/

>+     * If there is a pending open, do it now. It's important for this to be
>+     * inline since we do it in almost every call in a stream.
>+     */
>+    inline nsresult DoPendingOpen();

If you actually care about this being inline, you should put the impl in the header (after the class decl, say).  Otherwise compilers may or may not inline it.

Also, s/call in a stream/stream API call/

>+    /**
>+     * If there is a pending open, cancel it and clean it up.

s/ and clean it up//, I think.

>+++ b/xpcom/io/nsLocalFileUnix.cpp

You don't want to land this stuff.  Take it out.

r=me with the above nits picked.  Please post the patch with the nits picked in the bug before pushing, though.

Comment 61

[Shawn Wilsher :sdwilsh]

(In reply to comment #60)
> If you actually care about this being inline, you should put the impl in the
> header (after the class decl, say).  Otherwise compilers may or may not inline
> it.
We shouldn't care about it being inline though.  The disk touching is going to dominate the time here, so inlining is just silly I think.

Comment 62

[Alon Zakai (:azakai)]

> >+++ b/netwerk/base/src/nsFileStreams.cpp
> > nsFileInputStream::Read(const IPC::Message *aMsg, void **aIter)
> > {
> >+    nsresult rv = DoPendingOpen();
> >+    NS_ENSURE_SUCCESS(rv, rv);
> >+
> 
> This change makes no sense.  Please revert it?
> 
> > void
> > nsFileInputStream::Write(IPC::Message *aMsg)
> > {
> >+    nsresult rv = DoPendingOpen();
> >+    if (!NS_SUCCEEDED(rv)) {
> >+        return;
> >+    }
> >+
> 
> Likewise.
> 

Hmm, reading those two methods more carefully, I realize I don't know what is going on. These are IPC-only functions, I guess. Why does Read call Init? And why does an input stream even need a Write method?

I don't know whether this patch will interfere with that or not - in particular, Read will call Init, with behavior flags passed over - would DEFER_OPEN be a problem? If so should we add a runtime check that DEFER_OPEN is not in those flags?

Comment 63

[Boris Zbarsky [:bzbarsky]]

Those methods are used to send objects across the process boundary.  Write serializes the object into a form that can ten be sent to the other process.  Read takes the serialized form and creates a new object.

> Read will call Init, with behavior flags passed over

Uh... how so?  It seems to pass in the mBehaviorFlags that Write wrote out, no?

Comment 64

[Alon Zakai (:azakai)]

I have no idea. All I see so far is that Init is being called in Read, which implies that this object is not used like a 'normal' nsFileInputStream, where we manually call Init. And DEFER_OPEN changes exactly the behavior of what happens during Init. So without understanding the IPC part of this code, all I can say so far is it looks to me like it might be relevant. I will need to spend some time reading the IPC code that uses this.

Comment 65

[Boris Zbarsky [:bzbarsky]]

Alon, the way this is used in IPC is that you create an input stream, initialize it however you want, then call Write() on it, all in process A.  Then in process B you take the output created by Write and call Read() on it.  This creates a new input stream in process B which should be a clone of the original stream you started with, effectively.

Comment 66

[Shawn Wilsher :sdwilsh]

Comment on attachment 514640 [details] [diff] [review]
Part 2: Tests v3

>+++ b/netwerk/test/unit/test_NetUtil.js
>+function async_write_file(aBehaviorFlags)
>+function async_write_file_nsISafeOutputStream(aBehaviorFlags)
>+function asyncFetch_with_nsIFile(aBehaviorFlags)
Please add java-doc style documentation for all three of these since they are helper methods now please.


>+++ b/netwerk/test/unit/test_filestreams.js
global-nit: please brace one-line ifs

>+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is mozilla.org unit test code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ *  Mozilla
>+ * Portions created by the Initial Developer are Copyright (C) 2011
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
nit: https://www.mozilla.org/MPL/boilerplate-1.1/pd-c

>+////////////////////////////////////////////////////////////////////////////////
>+//// Helper Methods
>+
>+/**
>+ * DEFER_OPEN files should only be opened when actually written to, as
>+ * we defer the call to Open to later, and so forth. Note that for writing,
>+ * we check for actual writing in test_NetUtil (async) and in sync_operations
>+ * in this file (sync), whereas in this function we just check that the file
>+ * is *not* created during init.
>+ *
>+ * @param aSafe Whether to use a safe stream or not (only relevant for output streams)
>+ * @param aDeferOpen Whether to check with DEFER_OPEN or not
>+ * @param aTrickDeferredOpen Whether we try to 'trick' deferred opens by changing the file before
>+ *                           the actual open. The stream should have a clone, so this shouldn't confuse it
nit: this isn't the style that is used in network; please copy the formatting that NetUtil does: https://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#63

>+ */
>+function DEFER_OPEN(aSafe, aDeferOpen, aTrickDeferredOpen) {
nit: brace on newline (this applies globally)
nit: the name of this function is odd and shouldn't be in all caps (looks like a const when in use).

I'd actually prefer to pass in the contract id instead using aSafe here too.

>+  if (file.exists())
>+    file.remove(false);
This may fail (especially on windows).  You really want to use createUnique and never try to remove a file whenever possible.

>+  let ostream = Cc["@mozilla.org/network/" + (aSafe ? "safe-" : "") + "file-output-stream;1"].
>+                createInstance(Ci.nsIFileOutputStream);
>+  ostream.init(file, -1, -1, aDeferOpen ? Ci.nsIFileOutputStream.DEFER_OPEN : 0);
>+  do_check_eq(aDeferOpen, !file.exists()); // If defer, should not exist and vice versa
This may not work right either.  On some platforms we have a cache for this.  You need to create a new nsIFile object to actually check it (use clone).

>+  if (aDeferOpen) {
>+    // File should appear when we do write to it.
>+    if (aTrickDeferredOpen)
>+      file.leafName = TRICKY_LEAF_NAME;
>+    ostream.write("data", 4);
>+    if (aTrickDeferredOpen)
>+      file.leafName = LEAF_NAME;
>+    do_check_true(file.exists());
I think this is checking that modifying the nsIFile object passed in doesn't change where the stream writes to, correct?  Could you add a comment explaining at a little better please (it wasn't clear to me at first)?  This is sorta explained in the java-doc comment, but only if you read what @param aTrickDeferredOpen is for.

>+  // Reading
You actually probably want to split this method up into a read method and a write method do to the existence issues I'm mentioning here.

>+  if (file.exists())
>+    file.remove(false);
Again, this isn't reliable.  Use a new file via createUnique.

>+  let istream = Cc["@mozilla.org/network/file-input-stream;1"].
>+                createInstance(Ci.nsIFileInputStream);
>+  var initOk, getOk;
>+  try {
>+    istream.init(file, -1, 0, aDeferOpen ? Ci.nsIFileOutputStream.DEFER_OPEN : 0);
>+    initOk = true;
>+  }
>+  catch(e) {
>+    initOk = false;
>+  }
>+  try {
>+    let fstream = Cc["@mozilla.org/network/file-input-stream;1"].
>+                  createInstance(Ci.nsIFileInputStream);
>+    fstream.init(aFile, -1, 0, 0);
>+    getOk = true;
>+  }
>+  catch(e) {
>+    getOk = false;
>+  }
>+  do_check_true( (aDeferOpen && initOk && !getOk) ||
>+                 (!aDeferOpen && !initOk && !getOk) );
>+  istream.close();
I'm not even sure what this is trying to do.  Can you please add a comment explaining this?

>+// We test async operations in test_NetUtil.js, and here test for simple sync
>+// operations on input streams.
>+function sync_operations(aBehaviorFlags)
nit: proper java-doc comments please

>+////////////////////////////////////////////////////////////////////////////////
>+//// Tests
>+
>+function make_defer_open_tests() {
>+  function make_test(aSafe, aDeferOpen, aTrickDeferredOpen) {
>+    var temp;
>+    return eval("temp = function defer_test_" + aSafe + "_" + aDeferOpen + "_" + aTrickDeferredOpen + "() {" +
>+                "  return DEFER_OPEN(aSafe, aDeferOpen, aTrickDeferredOpen);" +
>+                "}");
>+    return ret;
>+  }
>+  return [0,1,2,3,4,5,6,7].map(function(i) {
>+    return make_test(!!(i & 1), !!(i & 2), !!(i & 4))
>+  });
>+}
I understand that you are trying to make it simple to test all cases here, but this would be rather difficult to debug.  I'd much rather see a method with a descriptive name for each of these here.

>+function run_next_test()
>+{
>+  if (index < tests.length) {
>+    do_test_pending();
>+
>+    // Asynchronous test exceptions do not kill the test...
>+    do_execute_soon(function() {
>+      try {
>+        print("Running the next test: " + tests[index].name);
>+        tests[index++]();
>+      }
>+      catch (e) {
>+        do_throw(e);
>+      }
>+    });
>+  }
>+
>+  do_test_finished();
>+}
None of these tests are actually async, so you could just iterate the tests array.  However, if you want to keep it async, please use run_next_test from here instead of the one you copied from test_NetUtil: https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/head_common.js#650

r- only because of the existence issues.  I don't want to add more intermittent orange to the tree.

Comment 67

[Alon Zakai (:azakai)]

(In reply to comment #65)
> Alon, the way this is used in IPC is that you create an input stream,
> initialize it however you want, then call Write() on it, all in process A. 
> Then in process B you take the output created by Write and call Read() on it. 
> This creates a new input stream in process B which should be a clone of the
> original stream you started with, effectively.

Thanks, now I see. So there should be no problem with DEFER_OPEN here (and no need for DoPendingOpen as you said).

Will post an updated patch shortly.

Comment 68

[Alon Zakai (:azakai)]

Attachment: Part 1: DEFER_OPEN option v4

Updated patch. bz, did you want to take another look at this?

Comment 69

[Boris Zbarsky [:bzbarsky]]

I wanted to give it a skim, yes.  Looks good.

Comment 70

[Alon Zakai (:azakai)]

Attachment: Part 2: Tests v4

Updated tests patch.

Comment 71

[Shawn Wilsher :sdwilsh]

Why did you make ensure_unique when nsIFile::createUnique exists?  You should be able to call remove right after creating it so it doesn't actually exist.

Comment 72

[Alon Zakai (:azakai)]

Well, you scared me with file deletion worries. This way avoids file deletion entirely.

If calling createUnique and then deleting is guaranteed to work, then I am not sure why the code from before could fail. It was careful to only delete when it knew the file existed. So unless some other process is messing with the files, I don't understand the danger?

Comment 73

[Shawn Wilsher :sdwilsh]

(In reply to comment #72)
> Well, you scared me with file deletion worries. This way avoids file deletion
> entirely.
> 
> If calling createUnique and then deleting is guaranteed to work, then I am not
> sure why the code from before could fail. It was careful to only delete when it
> knew the file existed. So unless some other process is messing with the files,
> I don't understand the danger?
I've never seen an intermittent orange because we create and then delete.  However, once you start writing anything to the file (which we were doing before), deleting doesn't always seem to work.

Comment 74

[Alon Zakai (:azakai)]

(In reply to comment #73)
> (In reply to comment #72)
> > Well, you scared me with file deletion worries. This way avoids file deletion
> > entirely.
> > 
> > If calling createUnique and then deleting is guaranteed to work, then I am not
> > sure why the code from before could fail. It was careful to only delete when it
> > knew the file existed. So unless some other process is messing with the files,
> > I don't understand the danger?
> I've never seen an intermittent orange because we create and then delete. 
> However, once you start writing anything to the file (which we were doing
> before), deleting doesn't always seem to work.

I would guess that is a Windows issue (not allowing open files to be deleted)? I ran into it in this bug, but only because I wasn't properly closing the file streams before I tried to delete them, which was easy to fix.

Anyhow, would you like me to rewrite this patch with createUnique and then delete?

Comment 75

[Shawn Wilsher :sdwilsh]

(In reply to comment #74)
> I would guess that is a Windows issue (not allowing open files to be deleted)?
> I ran into it in this bug, but only because I wasn't properly closing the file
> streams before I tried to delete them, which was easy to fix.
Yeah, windows is usually the issue here.  Sometimes even closing it doesn't seem to help (at least in the case of SQLite files).

> Anyhow, would you like me to rewrite this patch with createUnique and then
> delete?
At the very least, we shouldn't be doing Math.random(); just increment the number.  Your call on what approach you want to do.

Comment 76

[Alon Zakai (:azakai)]

Attachment: Part 2: Tests v5

Ok, here's a patch with incrementing instead of randomness.

Comment 77

[Shawn Wilsher :sdwilsh]

Sorry for the delay on review here.  I'll review the test changes this evening (EST).

Comment 78

[Shawn Wilsher :sdwilsh]

Comment on attachment 515790 [details] [diff] [review]
Part 2: Tests v5

>+++ b/netwerk/test/unit/test_NetUtil.js
>+function async_write_file(aDeferOpen)
>+function async_write_file_nsISafeOutputStream(aDeferOpen)
I missed this before, but these two methods are identical with the exception of a different contract id for the safe file output stream.  We should only have one helper method here that takes a contract id and aDeferOpen.

>+++ b/netwerk/test/unit/test_filestreams.js
>+function test_access()
>+{
>+  check_access("@mozilla.org/network/file-output-stream;1", false, false);
>+}
>+
>+function test_access_trick()
>+{
>+  check_access("@mozilla.org/network/file-output-stream;1", false, true);
>+}
>+
>+function test_access_defer()
>+{
>+  check_access("@mozilla.org/network/file-output-stream;1", true, false);
>+}
>+
>+function test_access_defer_trick()
>+{
>+  check_access("@mozilla.org/network/file-output-stream;1", true, true);
>+}
>+
>+function test_access_safe()
>+{
>+  check_access("@mozilla.org/network/safe-file-output-stream;1", false, false);
>+}
>+
>+function test_access_safe_trick()
>+{
>+  check_access("@mozilla.org/network/safe-file-output-stream;1", false, true);
>+}
>+
>+function test_access_safe_defer()
>+{
>+  check_access("@mozilla.org/network/safe-file-output-stream;1", true, false);
>+}
>+
>+function test_access_safe_defer_trick()
>+{
>+  check_access("@mozilla.org/network/safe-file-output-stream;1", true, true);
>+}
constify the contract ids at the top of the file please

r=sdwilsh

Comment 79

[Shawn Wilsher :sdwilsh]

I also want to add that it'd probably be a good idea to get approval on this change before landing it.  This does touch code that Firefox uses, and given how close it is to hitting RC, a little extra caution won't hurt.

Comment 80

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 515790 [details] [diff] [review]
Part 2: Tests v5

requesting approval to land these patches (event though they're blocking fennec already), per sdwilsh's request

Comment 81

[Mike Beltzner [:beltzner, not reading bugmail]]

Resetting blocking-fennec2.0 to "?"

This is a pretty deep set of changes, and the Firefox drivers are not happy with the idea of taking them so late in the game. Does this really block Fennec or can we try to get it later?

Basically, there's no way we can take *this* set of patches for our RC.

Comment 82

[Alon Zakai (:azakai)]

I fully understand Firefox not wanting to take this set of patches.

For Fennec, I would argue that we should take this - the UI locking up for a few seconds a short while after startup, on Galaxy S devices (and shorter lockups on other devices), is bad.

Perhaps we can somehow resolve this with branching, so it only lands for Fennec 4.0 but not Firefox 4.0?

Comment 83

[Shawn Wilsher :sdwilsh]

We will certainly want this for Firefox 5, fwiw.

Comment 84

[Mike Beltzner [:beltzner, not reading bugmail]]

Alon: that would be great, but Stuart claims that this requires desktop testing :/

He and I are discussing a way to see this happen.

Comment 85

[Brad Lassey [:blassey] (use needinfo?)]

as discussed, this still blocks fennec

Comment 86

[Alon Zakai (:azakai)]

Attachment: Part 2: Tests v6

Updated tests patch, carried r+.

Comment 87

[Robert Sayre]

Stuart and I discussed this patch on IRC.

We decided that we can take this patch as an ifdef, so it can be reverted very quickly, and without a lot of branch mechanics. Once a patchset is posted with both versions of the code, I'll approve it.

Comment 88

[Shawn Wilsher :sdwilsh]

(In reply to comment #87)
> We decided that we can take this patch as an ifdef, so it can be reverted very
> quickly, and without a lot of branch mechanics. Once a patchset is posted with
> both versions of the code, I'll approve it.
And I guess we'll need to take the firefox changes to JS files in a new bug then too.

Comment 89

[Stuart Parmenter]

Talked to sayrer -- we're OK to land this on m-c now.  We won't pull this in to desktop's 4.0 (or 4.0.1 (if necessary)).

Comment 90

[Alon Zakai (:azakai)]

Attachment: combined m-c patch

Combined m-c patch that I will check in in a bit.

Comment 91

[Alon Zakai (:azakai)]

m-c part: http://hg.mozilla.org/mozilla-central/rev/204bb4873d1b
m-b part: http://hg.mozilla.org/mobile-browser/rev/a59a3d731222

So all done except for the services part. philikon, have you pushed that already?

Comment 92

[Philipp von Weitershausen [:philikon]]

Pushed part 5 to fx-sync: https://hg.mozilla.org/services/fx-sync/rev/e3f4f3f5c958

This will make its way to m-c when we merge our other Fennec blockers. At that point this bug can be resolved, right?

Comment 93

[Alon Zakai (:azakai)]

Yes, we can resolve this bug when that merge happens.

Comment 94

[Philipp von Weitershausen [:philikon]]

Merged part 5 to services-central: http://hg.mozilla.org/services/services-central/rev/bd8f30e6eb18

Comment 95

[Philipp von Weitershausen [:philikon]]

Merged part 5 to m-c: http://hg.mozilla.org/mozilla-central/rev/bd8f30e6eb18

Comment 96

[Philipp von Weitershausen [:philikon]]

I don't believe there are any STRs for QA, so marking as [qa-].