Closed Bug 961665 Opened 10 years ago Closed 10 years ago

[OS.File] Native implementation of read()

Categories

(Toolkit Graveyard :: OS.File, enhancement)

enhancement
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
mozilla30

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

(Keywords: perf, Whiteboard: [Async:ready])

Attachments

(5 files, 36 obsolete files)

1.07 KB, patch
gps
: review+
Details | Diff | Splinter Review
21.44 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
13.84 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
42.26 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
646 bytes, patch
Yoric
: review+
Details | Diff | Splinter Review
Bug 959130 shows a considerable regression in startup. We found out that the culprit is using OS.File.read() during startup, which requires initializing the OS.File ChromeWorker, which in turns can take ~700ms on a fast machine (!)

Since we want to be able to use OS.File.read during startup, this means that we need an implementation that doesn't need to start the worker, i.e. a native implementation.
Attached patch Native code (obsolete) — Splinter Review
Attachment #8362666 - Flags: review?(nfroyd)
Attached patch 1. Native code (obsolete) — Splinter Review
This code implements a native xpcom version of OS.File.read(), minus compression.
Attachment #8362666 - Attachment is obsolete: true
Attachment #8362666 - Flags: review?(nfroyd)
Attachment #8362890 - Flags: review?(nfroyd)
Attached patch 2. JS Code (obsolete) — Splinter Review
This surprisingly long patch adds the ability to transparently replace the JS back-end for OS.File.read by the native back-end, as well as the features that were necessary for testing.
Attachment #8362892 - Flags: review?(nfroyd)
Attached patch 4. Tests (obsolete) — Splinter Review
Attachment #8362895 - Flags: review?(nfroyd)
I should add that the Windows version hasn't been tested yet. I'm in the progress of building it.
Attachment #8362534 - Attachment filename: native.build → 4. Build and link
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #0)
> Bug 959130 shows a considerable regression in startup. We found out that the
> culprit is using OS.File.read() during startup, which requires initializing
> the OS.File ChromeWorker, which in turns can take ~700ms on a fast machine
> (!)

Bleh.  Do we know why it takes so long to launch the worker?  It looks like there has been no discussion of trying to make that part faster.

> Since we want to be able to use OS.File.read during startup, this means that
> we need an implementation that doesn't need to start the worker, i.e. a
> native implementation.

This part does not follow for me from the problem analysis.  Can you elaborate on why maintaining a separate pile of XPCOM code is necessary and useful (even, after a quick skim of the code, user-selectable on a per-read operation basis!) and better than other alternatives?
(In reply to Nathan Froyd (:froydnj) from comment #7)
> (In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from
> comment #0)
> > Bug 959130 shows a considerable regression in startup. We found out that the
> > culprit is using OS.File.read() during startup, which requires initializing
> > the OS.File ChromeWorker, which in turns can take ~700ms on a fast machine
> > (!)
> 
> Bleh.  Do we know why it takes so long to launch the worker?  It looks like
> there has been no discussion of trying to make that part faster.

~400-500 ms seem to be due to the bytecode cache (bug 960986). But even if we fix that issue, that's still 200ms + JS code, which is simply not acceptable during startup.


> > Since we want to be able to use OS.File.read during startup, this means that
> > we need an implementation that doesn't need to start the worker, i.e. a
> > native implementation.
> 
> This part does not follow for me from the problem analysis.  Can you
> elaborate on why maintaining a separate pile of XPCOM code is necessary and
> useful (even, after a quick skim of the code, user-selectable on a per-read
> operation basis!) and better than other alternatives?

Note: I'm not sure that the user-selectable basis is really useful. I meant it mostly for testing purposes. I have strictly no issue with removing it.

So, let's see what alternatives we have:

1. Make ChromeWorkers + OS.File start in 10 ms;
  * Doesn't look feasible (npb suspects that 50 ms would be a lower bound to initialize just the runtime);
2. Fallback to main thread I/O;
  * Bleh;
3. Use the existing tools (FileUtils, NetUtils, nsIFile) during startup:
  * Sources of main thread I/O;
  * In a number of cases, simply slow;
  * Not quite as nice to use as OS.File;
  * When we write code, we're not always sure whether it will be called during startup.
4. New pile of native code sitting side-by-side with OS.File, with an independent API.
  * What's the point of having two JS APIs for the same thing?
5. Replace bits and pieces of osfile_async_front.jsm with something faster based on existing stuff.
  * Single API, no client code refactoring needed.
  * The problems of 1. or 2.
6. Replace bits and pieces of osfile_async_front.jsm with (new) native code.
  * Single API, no client code refactoring needed.
  * Paves the way towards a OS.File for C++ (which would be useful, too).

So, I'd go for 6. Did I miss something?
Attached patch 4. Cumulative patch for Windows (obsolete) — Splinter Review
Fixing a few typos on the Windows version.
Attached patch 1. Native code, v2 (obsolete) — Splinter Review
Folding in the Windows fixes.
Attachment #8362890 - Attachment is obsolete: true
Attachment #8363014 - Attachment is obsolete: true
Attachment #8362890 - Flags: review?(nfroyd)
Attachment #8363016 - Flags: review?(nfroyd)
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #8)
> (In reply to Nathan Froyd (:froydnj) from comment #7)
> > (In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from
> > comment #0)
> > > Bug 959130 shows a considerable regression in startup. We found out that the
> > > culprit is using OS.File.read() during startup, which requires initializing
> > > the OS.File ChromeWorker, which in turns can take ~700ms on a fast machine
> > > (!)
> > 
> > Bleh.  Do we know why it takes so long to launch the worker?  It looks like
> > there has been no discussion of trying to make that part faster.
> 
> ~400-500 ms seem to be due to the bytecode cache (bug 960986). But even if
> we fix that issue, that's still 200ms + JS code, which is simply not
> acceptable during startup.

Have you timed the startup of what you are proposing to use instead?

> > > Since we want to be able to use OS.File.read during startup, this means that
> > > we need an implementation that doesn't need to start the worker, i.e. a
> > > native implementation.
> > 
> > This part does not follow for me from the problem analysis.  Can you
> > elaborate on why maintaining a separate pile of XPCOM code is necessary and
> > useful (even, after a quick skim of the code, user-selectable on a per-read
> > operation basis!) and better than other alternatives?
> 
> Note: I'm not sure that the user-selectable basis is really useful. I meant
> it mostly for testing purposes. I have strictly no issue with removing it.
> 
> So, let's see what alternatives we have:
> 
> 1. Make ChromeWorkers + OS.File start in 10 ms;
>   * Doesn't look feasible (npb suspects that 50 ms would be a lower bound to
> initialize just the runtime);

OK, that is probably fair.

> 5. Replace bits and pieces of osfile_async_front.jsm with something faster
> based on existing stuff.
>   * Single API, no client code refactoring needed.
>   * The problems of 1. or 2.
> 6. Replace bits and pieces of osfile_async_front.jsm with (new) native code.
>   * Single API, no client code refactoring needed.
>   * Paves the way towards a OS.File for C++ (which would be useful, too).

What is the difference between these two options?  The native code is already based on existing stuff, yes?  Or is the "existing stuff" things like NetUtil?
Comment on attachment 8363016 [details] [diff] [review]
1. Native code, v2

Review of attachment 8363016 [details] [diff] [review]:
-----------------------------------------------------------------

Preliminary comments.  I'm still thinking about how we can avoid all this--or why not just use nsIFile bits for the actual Read() part?

::: toolkit/components/osfile/NativeOSFileInternals.cpp
@@ +58,5 @@
> +}
> +
> +// Guard object for autoclosing file descriptors
> +
> +struct ScopedCloseFile {

I feel like we must have at least three different versions of this floating around in the tree.

@@ +125,5 @@
> +const int SYSERROR_INVALID_ARGUMENT = ERROR_NOT_SUPPORTED;
> +const int SYSERROR_FILE_TOO_LARGE = ERROR_FILE_TOO_LARGE;
> +
> +
> +typedef DWORD ssize_t;

This is incorrect for the Win64 case.  (I thought our configure script defined ssize_t at one point...)

@@ +271,5 @@
> +  }
> + private:
> +  // The callbacks. Maintained as nsRefPtr as they are generally
> +  // xpconnect values, which cannot be manipulated with nsCOMPtr
> +  // off the main thread.

Um.  What makes the AddRef and Release operations that nsRefPtr does any safer than the ones nsCOMPtr does?

@@ +308,5 @@
> +  }
> + private:
> +  // The callbacks. Maintained as nsRefPtr as they are generally
> +  // xpconnect values, which cannot be manipulated with nsCOMPtr
> +  // off the main thread.

Same comments apply here.

@@ +408,5 @@
> +    ScopedDeletePtr<nsString> resultString(new nsString());
> +    resultString->SetLength(maxChars);
> +
> +    int32_t encodedLength = buffer->Length();
> +    if (encodedLength != (int64_t)buffer->Length()) {

This is an odd way to test for |encodedLength < 0|.

@@ +417,5 @@
> +                          resultString->BeginWriting(), &maxChars);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));
> +    resultString->SetLength(maxChars);
> +
> +    buffer.dispose(); // We don't need this data anymore

Sure, but you might as well let the destructor do your work for you.
(In reply to Nathan Froyd (:froydnj) from comment #11)
> Have you timed the startup of what you are proposing to use instead?

Not yet. I'll try and benchmark this today or tomorrow.

> > 5. Replace bits and pieces of osfile_async_front.jsm with something faster
> > based on existing stuff.
> >   * Single API, no client code refactoring needed.
> >   * The problems of 1. or 2.
> > 6. Replace bits and pieces of osfile_async_front.jsm with (new) native code.
> >   * Single API, no client code refactoring needed.
> >   * Paves the way towards a OS.File for C++ (which would be useful, too).
> 
> What is the difference between these two options?  The native code is
> already based on existing stuff, yes?  Or is the "existing stuff" things
> like NetUtil?

Indeed, I meant NetUtil.
(In reply to Nathan Froyd (:froydnj) from comment #12)
> > +struct ScopedCloseFile {
> 
> I feel like we must have at least three different versions of this floating
> around in the tree.

Unfortunately, they don't seem to be exported.

> @@ +125,5 @@
> > +const int SYSERROR_INVALID_ARGUMENT = ERROR_NOT_SUPPORTED;
> > +const int SYSERROR_FILE_TOO_LARGE = ERROR_FILE_TOO_LARGE;
> > +
> > +
> > +typedef DWORD ssize_t;
> 
> This is incorrect for the Win64 case.  (I thought our configure script
> defined ssize_t at one point...)

Currently, it doesn't. I'll try and look for a workaround.

> @@ +271,5 @@
> > +  }
> > + private:
> > +  // The callbacks. Maintained as nsRefPtr as they are generally
> > +  // xpconnect values, which cannot be manipulated with nsCOMPtr
> > +  // off the main thread.
> 
> Um.  What makes the AddRef and Release operations that nsRefPtr does any
> safer than the ones nsCOMPtr does?

nsCOMPtr calls QI (during assignation, iirc), which causes an assertion failure with xpconnect values.

> 
> @@ +408,5 @@
> > +    ScopedDeletePtr<nsString> resultString(new nsString());
> > +    resultString->SetLength(maxChars);
> > +
> > +    int32_t encodedLength = buffer->Length();
> > +    if (encodedLength != (int64_t)buffer->Length()) {
> 
> This is an odd way to test for |encodedLength < 0|.

Good point :)

> @@ +417,5 @@
> > +                          resultString->BeginWriting(), &maxChars);
> > +    MOZ_ASSERT(NS_SUCCEEDED(rv));
> > +    resultString->SetLength(maxChars);
> > +
> > +    buffer.dispose(); // We don't need this data anymore
> 
> Sure, but you might as well let the destructor do your work for you.

Sure, if you prefer.
(In reply to Nathan Froyd (:froydnj) from comment #12)
> Comment on attachment 8363016 [details] [diff] [review]
> 1. Native code, v2
> 
> Review of attachment 8363016 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Preliminary comments.  I'm still thinking about how we can avoid all
> this--or why not just use nsIFile bits for the actual Read() part?

Ah, I forgot to answer this.
We could use nsIFile for Read(). This would simplify Read() a bit and complicate error-handling a bit, as I'd have to map XPCOM error values to their errno/GetLastError() values to ensure that they remain compatible with OS.File.Error. All in all, I decided to go for platform-specific code as this was more in line with the rest of OS.File and since I hoped it would make consistency easier to check.
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #13)
> (In reply to Nathan Froyd (:froydnj) from comment #11)
> > Have you timed the startup of what you are proposing to use instead?
> 
> Not yet. I'll try and benchmark this today or tomorrow.
> 

Mmmh... My numbers seem to differ greatly from Tim's.

Protocol:
- running the test 1000 times on xpcshell;
- kill the OS.File worker;
- ChromeWorker: measure duration between postMessage and the first lines of JS;
- native: measure the duration between Dispatch() and Run();
- measures taken on a 2 years old MacBook Pro.

Note that there are wide variations between runs in the debug version.

Build 1: --enable-debug --disable-optimize --enable-profiling --enable-logging
- ChromeWorker launch: 40ms to 219ms (wide variations), OS.File ready (all toplevel code executed): ~420ms (that's surprisingly stable)
- Native launch: 80µs to 200µs

Build 2: --disable-debug --enable-optimize --disable-profiling --disable-logging
- ChromeWorker launch: 15ms, OS.File ready: 35ms
- Native launch: 16µs

I suspect that the difference is xpcshell vs. real Firefox.
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #16)
> (In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from
> comment #13)
> > (In reply to Nathan Froyd (:froydnj) from comment #11)
> > > Have you timed the startup of what you are proposing to use instead?
> > 
> > Not yet. I'll try and benchmark this today or tomorrow.
> > 
> 
> Mmmh... My numbers seem to differ greatly from Tim's.
> 
> Protocol:
> - running the test 1000 times on xpcshell;
> - kill the OS.File worker;
> - ChromeWorker: measure duration between postMessage and the first lines of
> JS;
> - native: measure the duration between Dispatch() and Run();
> - measures taken on a 2 years old MacBook Pro.
> 
> Note that there are wide variations between runs in the debug version.
> 
> Build 1: --enable-debug --disable-optimize --enable-profiling
> --enable-logging
> - ChromeWorker launch: 40ms to 219ms (wide variations), OS.File ready (all
> toplevel code executed): ~420ms (that's surprisingly stable)
> - Native launch: 80µs to 200µs
> 
> Build 2: --disable-debug --enable-optimize --disable-profiling
> --disable-logging
> - ChromeWorker launch: 15ms, OS.File ready: 35ms
> - Native launch: 16µs
> 
> I suspect that the difference is xpcshell vs. real Firefox.

That's a *really* big variance between xpcshell and real Firefox.  Try it in Firefox instead?
As mentioned in bug 960986 comment #14 the browser window is slowing down worker startup. If the browser window is parsed and ready before we try to read the session file, then it's blocking the loading of the worker's script files and will lead to said delay. For xpcshell tests this should all be really fast because there is no browser window.
(In reply to Tim Taubert [:ttaubert] from comment #18)
> As mentioned in bug 960986 comment #14 the browser window is slowing down
> worker startup. If the browser window is parsed and ready before we try to
> read the session file, then it's blocking the loading of the worker's script
> files and will lead to said delay. For xpcshell tests this should all be
> really fast because there is no browser window.

OK, that makes sense.  David, can you try replicating Tim's results with Firefox?

Also, if push comes to shove, I think doing these modifications to OS.File is nicer than bug 959958.
Flags: needinfo?(dteller)
(In reply to Nathan Froyd (:froydnj) from comment #19)
> OK, that makes sense.  David, can you try replicating Tim's results with
> Firefox?

I'll try and benchmark both the native version and the ChromeWorker version in Firefox.

> Also, if push comes to shove, I think doing these modifications to OS.File
> is nicer than bug 959958.

Did you mean 959598?
Flags: needinfo?(dteller)
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #20)
> > Also, if push comes to shove, I think doing these modifications to OS.File
> > is nicer than bug 959958.
> 
> Did you mean 959598?

Doh, of course.
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #20)
> I'll try and benchmark both the native version and the ChromeWorker version
> in Firefox.

On my Nightly --enable-optimize --disable-debug, when measuring the time it takes to start the OS.File worker, I seem to see:
- 20-50 ms until the ChromeWorker starts evaluating;
- 70-450 ms until the ChromeWorker stops evaluation;
- 15-50 µs for the native version.
You should generally avoid using "string"/"wstring" in IDL used by script - they are just generally bad (embedded nulls cause truncation, "wstring" can't share the string, "string" mangles non-ASCII, etc.). Not huge problems in this case but using AString is an easy change.
Attached file 2. JS Code, v2 (obsolete) —
Attachment #8362892 - Attachment is obsolete: true
Attachment #8362892 - Flags: review?(nfroyd)
Attachment #8365471 - Flags: review?(nfroyd)
Attached patch 1. Native code, v3 (obsolete) — Splinter Review
Applied misc feedback.
Attachment #8363016 - Attachment is obsolete: true
Attachment #8363016 - Flags: review?(nfroyd)
Attachment #8365472 - Flags: review?(nfroyd)
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #22)
> (In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from
> comment #20)
> > I'll try and benchmark both the native version and the ChromeWorker version
> > in Firefox.
> 
> On my Nightly --enable-optimize --disable-debug, when measuring the time it
> takes to start the OS.File worker, I seem to see:
> - 20-50 ms until the ChromeWorker starts evaluating;
> - 70-450 ms until the ChromeWorker stops evaluation;
> - 15-50 µs for the native version.

So, we compared notes with Tim, and it turns out that we're simply not benchmarking the same worker. Still, things are bad on both the OS.File worker and on the SessionWorker, and the native OS.File.read() can solve both issues in one go.

As a side-note, I had times that lasted up to 1100ms until the ChromeWorker stops evaluating.
Flags: needinfo?(nfroyd)
Splitting part 2 to make reviewing easier. Also, taking the opportunity to clean up that code.
Attachment #8365471 - Attachment is obsolete: true
Attachment #8365471 - Flags: review?(nfroyd)
Attachment #8367302 - Flags: review?(nfroyd)
Attachment #8362895 - Attachment description: 3. Tests → 4. Tests
Attachment #8367302 - Flags: review?(nfroyd) → review+
Comment on attachment 8365472 [details] [diff] [review]
1. Native code, v3

Review of attachment 8365472 [details] [diff] [review]:
-----------------------------------------------------------------

This patch doesn't actually build any of the code that it provides.  You should fix that.

I guess this is going in the right direction.  Would still like to see if we can squeeze any speedup out of workers, either on the JS side with sharing, or with startup cache, etc. tricks.

::: toolkit/components/osfile/INativeOSFileInternals.idl
@@ +44,5 @@
> +{
> +  /**
> +   * @param operation The name of the failed operation.
> +   * @param status The status number of the operation, as per OS.File.Error
> +   * specifications (e.g. errno under Unix, GetLastError under Windows

Nit: end this with |).|.

@@ +63,5 @@
> +   * @param path The path to the file to read.
> +   * @param options An object that may contain some of the following fields
> +   * - {number} bytes The maximal number of bytes to read.
> +   * - {string} encoding If provided, return the result as a string, decoded
> +   *   using this encoding. Otherwise, return the result as an ArrayBuffer.

I guess by "return" here, you mean "the value passed to the success callback"?  We should make that clearer.

::: toolkit/components/osfile/NativeOSFileInternals.cpp
@@ +128,5 @@
> +
> +const int SYSERROR_INVALID_ARGUMENT = ERROR_NOT_SUPPORTED;
> +const int SYSERROR_FILE_TOO_LARGE = ERROR_FILE_TOO_LARGE;
> +
> +typedef DWORD bytes_t;

This is still suspect for Win64 (DWORD is 32 bits everywhere), though I guess the current implementation limits things to files of <= 2GB anyway.

@@ +147,5 @@
> +  AbstractResult(PRTime aExecutionDuration)
> +    : mExecutionDuration(aExecutionDuration)
> +  { }
> +  NS_IMETHOD GetExecutionDurationMS(uint32_t *aExecutionDurationMS)
> +  {

Don't declare this NS_IMETHOD, otherwise you're introducing virtual methods unnecessarily.  Just nsresult will do.  Or you might consider making this inherit from INativeOSFileResult, like nsRunnable inherits from nsIRunnable and sets up all the common Runnable methods.

@@ +199,5 @@
> +/**
> + * Return a result as a Uint8Array
> + *
> + * In this implementation, attribute |result| is a Uint8Array. Each call
> + * to |result| causes one bopy of the buffer.

"causes one copy"

@@ +202,5 @@
> + * In this implementation, attribute |result| is a Uint8Array. Each call
> + * to |result| causes one bopy of the buffer.
> + */
> +class TypedArrayResult MOZ_FINAL: public INativeOSFileResult,
> +                                   public AbstractResult

Nit: alignment.

@@ +209,5 @@
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +  NS_DECL_INATIVEOSFILERESULT
> +
> +  TypedArrayResult(PRTime aExecutionDuration,
> +                    nsCString* aResult)

Nit: argument alignment.  (Probably don't need to wrap here.)

@@ +275,5 @@
> + private:
> +  // The callbacks. Maintained as nsRefPtr as they are generally
> +  // xpconnect values, which cannot be manipulated with nsCOMPtr
> +  // off the main thread.
> +  nsRefPtr<INativeOSFileSuccessCallback> mOnSuccess;

Why store a success callback that never gets used?

@@ +313,5 @@
> +  // The callbacks. Maintained as nsRefPtr as they are generally
> +  // xpconnect values, which cannot be manipulated with nsCOMPtr
> +  // off the main thread.
> +  nsRefPtr<INativeOSFileSuccessCallback> mOnSuccess;
> +  nsRefPtr<INativeOSFileErrorCallback> mOnError;

Why store an error callback that never gets used?

@@ +355,5 @@
> +    return NS_OK;
> +  }
> +private:
> +  already_AddRefed<INativeOSFileSuccessCallback> mOnSuccess;
> +  already_AddRefed<INativeOSFileErrorCallback> mOnError;

It is really weird to see already_AddRefed *members*...can we just make these nsRefPtr and use .forget() in the appropriate places?

@@ +389,5 @@
> +
> +    if (!mEncoding.Length()) {
> +      // No encoding, return as an ArrayBuffer
> +      nsCOMPtr<INativeOSFileResult> result = new TypedArrayResult(PR_Now() - start,
> +                                                                  buffer.forget());

Nit: just use nsRefPtr here; pretty sure the right thing happens with casting already_AddRefed values.

@@ +413,5 @@
> +
> +    int32_t encodedLength = buffer->Length();
> +    if (encodedLength < 0) {
> +      return Fail(NS_LITERAL_CSTRING("arithmetics"), SYSERROR_FILE_TOO_LARGE);
> +    }

Might as well move this check above allocating resultString, so we don't allocate unnecessarily.  Even farther up is appropriate, beyond checking for a valid encodingName.

Also, "encodedLength" is not the right name for this.

@@ +423,5 @@
> +
> +    buffer.dispose(); // We don't need this data anymore
> +
> +    nsCOMPtr<INativeOSFileResult> result = new StringResult(PR_Now() - start,
> +                                                            resultString.forget());

Nit: nsRefPtr here.

@@ +439,5 @@
> +  NS_METHOD Read(nsACString& aBuffer)
> +  {
> +    MOZ_ASSERT(!NS_IsMainThread());
> +
> +#if defined(XP_UNIX)

I think using nsIFile and friends here, even with mappings from XPCOM error codes to platform error codes, would be much simpler in the long run.  The mapping seems a little onerous right now because we only have one operation implemented, but later operations will require minimal (if any) additions to the mapping, whereas separate implementations mean we have to write everything twice all the time.

@@ +461,5 @@
> +      size = mBytes;
> +    }
> +
> +    aBuffer.SetLength(size);
> +    uint32_t total_read = 0;

This should be off_t.

@@ +476,5 @@
> +    file.dispose();
> +#elif defined(XP_WIN)
> +
> +    ScopedCloseFile file(CreateFileW(mPath.get(), GENERIC_READ,
> +                             FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,

Your alignment for args here is peculiar.

@@ +499,5 @@
> +    }
> +
> +    aBuffer.SetLength(size);
> +    DWORD total_read = 0;
> +    DWORD numberOfBytesJustRead;

Go ahead and initialize this so the compiler can see that it always has a value.
Attachment #8365472 - Flags: review?(nfroyd) → feedback+
Comment on attachment 8362895 [details] [diff] [review]
4. Tests

Review of attachment 8362895 [details] [diff] [review]:
-----------------------------------------------------------------

The native option needs to be hidden behind a pref.

Also, don't we have many more tests for read?  Those should be exercised in native mode, too.
Attachment #8362895 - Flags: review?(nfroyd) → feedback+
Comment on attachment 8367304 [details] [diff] [review]
3. Calling the native back-end instead of the JS back-end whenever applicable

Review of attachment 8367304 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +825,5 @@
> +    options = clone(options, ["outExecutionDuration"]);
> +    options.bytes = bytes;
> +  }
> +
> +  if (options.compression || ("native" in options && !options["native"]) ) {

This switching stuff can be useful for testing, but let's hide it behind a pref and set that pref during testing.

::: toolkit/components/osfile/modules/osfile_shared_front.jsm
@@ +336,5 @@
>        return buffer;
>      }
> +    let decoder;
> +    try {
> +      decoder = new TextDecoder(options.encoding);

This is a new feature, correct?
Attachment #8367304 - Flags: review?(nfroyd) → feedback+
Flags: needinfo?(nfroyd)
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #26)
> (In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from
> comment #22)
> > (In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from
> > comment #20)
> > > I'll try and benchmark both the native version and the ChromeWorker version
> > > in Firefox.
> > 
> > On my Nightly --enable-optimize --disable-debug, when measuring the time it
> > takes to start the OS.File worker, I seem to see:
> > - 20-50 ms until the ChromeWorker starts evaluating;
> > - 70-450 ms until the ChromeWorker stops evaluation;
> > - 15-50 µs for the native version.
> 
> So, we compared notes with Tim, and it turns out that we're simply not
> benchmarking the same worker. Still, things are bad on both the OS.File
> worker and on the SessionWorker, and the native OS.File.read() can solve
> both issues in one go.
> 
> As a side-note, I had times that lasted up to 1100ms until the ChromeWorker
> stops evaluating.

Did you profile to see where the time is going?  What platform is this on?

If you didn't profile, do you have a testcase that I can attempt profiling with?
Flags: needinfo?(dteller)
(In reply to Nathan Froyd (:froydnj) from comment #32)
> Did you profile to see where the time is going?  What platform is this on?

I built with --disable-profiling to get (hopefully) realistic results, so no, I have not attempted to profile this just yet. It is my understanding that Tim did profile his own benchmark, though, and came up with the conclusion that the time was spent in the XUL cache.

> If you didn't profile, do you have a testcase that I can attempt profiling
> with?

The test case is as follows:
- (using a version of Scheduler._updateTelemetry patched to store the results in object OS.File in addition to adding them to the histograms);
- launch Firefox with a clean profile;
- open the console, import osfile.jsm, look at the results stored in object OS.File.
Flags: needinfo?(dteller) → needinfo?(ttaubert)
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #33)
> (In reply to Nathan Froyd (:froydnj) from comment #32)
> > Did you profile to see where the time is going?  What platform is this on?
> 
> I built with --disable-profiling to get (hopefully) realistic results, so
> no, I have not attempted to profile this just yet. It is my understanding
> that Tim did profile his own benchmark, though, and came up with the
> conclusion that the time was spent in the XUL cache.

You can read the whole story in bug 960986.
Flags: needinfo?(ttaubert)
(In reply to Nathan Froyd (:froydnj) from comment #29)
> ::: toolkit/components/osfile/NativeOSFileInternals.cpp
> @@ +128,5 @@
> > +
> > +const int SYSERROR_INVALID_ARGUMENT = ERROR_NOT_SUPPORTED;
> > +const int SYSERROR_FILE_TOO_LARGE = ERROR_FILE_TOO_LARGE;
> > +
> > +typedef DWORD bytes_t;
> 
> This is still suspect for Win64 (DWORD is 32 bits everywhere), though I
> guess the current implementation limits things to files of <= 2GB anyway.

Well, MSDN says DWORD.

> @@ +275,5 @@
> > + private:
> > +  // The callbacks. Maintained as nsRefPtr as they are generally
> > +  // xpconnect values, which cannot be manipulated with nsCOMPtr
> > +  // off the main thread.
> > +  nsRefPtr<INativeOSFileSuccessCallback> mOnSuccess;
> 
> Why store a success callback that never gets used?

Because this is the best way I know to ensure that it is Released on the main thread. Otherwise, I could NS_ProxyRelease it, if you prefer.

> @@ +313,5 @@
> > +  // The callbacks. Maintained as nsRefPtr as they are generally
> > +  // xpconnect values, which cannot be manipulated with nsCOMPtr
> > +  // off the main thread.
> > +  nsRefPtr<INativeOSFileSuccessCallback> mOnSuccess;
> > +  nsRefPtr<INativeOSFileErrorCallback> mOnError;
> 
> Why store an error callback that never gets used?

As above.

> @@ +355,5 @@
> > +    return NS_OK;
> > +  }
> > +private:
> > +  already_AddRefed<INativeOSFileSuccessCallback> mOnSuccess;
> > +  already_AddRefed<INativeOSFileErrorCallback> mOnError;
> 
> It is really weird to see already_AddRefed *members*...can we just make
> these nsRefPtr and use .forget() in the appropriate places?

I suppose that we can. However, calling Release() off the main thread on these fields doesn't make sense and should cause crashes, so it's a bit weird to have a nsRefPtr. What do you think?

> @@ +439,5 @@
> > +  NS_METHOD Read(nsACString& aBuffer)
> > +  {
> > +    MOZ_ASSERT(!NS_IsMainThread());
> > +
> > +#if defined(XP_UNIX)
> 
> I think using nsIFile and friends here, even with mappings from XPCOM error
> codes to platform error codes, would be much simpler in the long run.  The
> mapping seems a little onerous right now because we only have one operation
> implemented, but later operations will require minimal (if any) additions to
> the mapping, whereas separate implementations mean we have to write
> everything twice all the time.

I can try, but I'm not quite sure that there even is a conversion from XPCOM error codes to platform error codes. This might mean that I'll need to patch xpcom at some point in the future to ensure that error codes are correct.

(applied the rest of the feedback)
Attached patch 1. Native code, v4 (obsolete) — Splinter Review
Moved to a nsIFile-based implementation.
Attachment #8362534 - Attachment is obsolete: true
Attachment #8365472 - Attachment is obsolete: true
Attachment #8367886 - Flags: review?(nfroyd)
Moved native stuff to a new module, hidden the choice between native implementation and JS implementation behind a preference, now handling xpcom errors at JS-level.

> > +    let decoder;
> > +    try {
> > +      decoder = new TextDecoder(options.encoding);
> 
> This is a new feature, correct?

Indeed, this is a new feature. I am planning to make use of it immediately, but if you wish, I can postpone it to a followup bug.
Attachment #8367304 - Attachment is obsolete: true
Attachment #8367887 - Attachment is patch: true
Attachment #8367887 - Attachment mime type: application/x-javascript → text/plain
Attachment #8367887 - Flags: review?(nfroyd)
Attached patch 4. Tests, v2 (obsolete) — Splinter Review
More tests. Also, moved tests from mochi to xpcom.
Attachment #8362895 - Attachment is obsolete: true
Attachment #8367889 - Flags: review?(nfroyd)
Attached patch 4. Tests, v3 (obsolete) — Splinter Review
Adding forgotten file.
Attachment #8367889 - Attachment is obsolete: true
Attachment #8367889 - Flags: review?(nfroyd)
Attachment #8367897 - Flags: review?(nfroyd)
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #35)
> (In reply to Nathan Froyd (:froydnj) from comment #29)
> > ::: toolkit/components/osfile/NativeOSFileInternals.cpp
> > @@ +128,5 @@
> > > +
> > > +const int SYSERROR_INVALID_ARGUMENT = ERROR_NOT_SUPPORTED;
> > > +const int SYSERROR_FILE_TOO_LARGE = ERROR_FILE_TOO_LARGE;
> > > +
> > > +typedef DWORD bytes_t;
> > 
> > This is still suspect for Win64 (DWORD is 32 bits everywhere), though I
> > guess the current implementation limits things to files of <= 2GB anyway.
> 
> Well, MSDN says DWORD.

This page:

http://msdn.microsoft.com/en-us/library/windows/desktop/aa383751%28v=vs.85%29.aspx

says that SSIZE_T is a typedef for LONG_PTR, which is 64-bit on Win64 and 32-bit on Win32.  (Also, LONG_PTR is the worst name ever for an integer type, and doubly worst because long on Win64 is still only 32 bits...)  Searching seems to indicate that SSIZE_T is used for ssize_t.

> > @@ +275,5 @@
> > > + private:
> > > +  // The callbacks. Maintained as nsRefPtr as they are generally
> > > +  // xpconnect values, which cannot be manipulated with nsCOMPtr
> > > +  // off the main thread.
> > > +  nsRefPtr<INativeOSFileSuccessCallback> mOnSuccess;
> > 
> > Why store a success callback that never gets used?
> 
> Because this is the best way I know to ensure that it is Released on the
> main thread. Otherwise, I could NS_ProxyRelease it, if you prefer.

Aha.  A comment to that effect would be good.

> > @@ +313,5 @@
> > > +  // The callbacks. Maintained as nsRefPtr as they are generally
> > > +  // xpconnect values, which cannot be manipulated with nsCOMPtr
> > > +  // off the main thread.
> > > +  nsRefPtr<INativeOSFileSuccessCallback> mOnSuccess;
> > > +  nsRefPtr<INativeOSFileErrorCallback> mOnError;
> > 
> > Why store an error callback that never gets used?
> 
> As above.

As above.

> > @@ +355,5 @@
> > > +    return NS_OK;
> > > +  }
> > > +private:
> > > +  already_AddRefed<INativeOSFileSuccessCallback> mOnSuccess;
> > > +  already_AddRefed<INativeOSFileErrorCallback> mOnError;
> > 
> > It is really weird to see already_AddRefed *members*...can we just make
> > these nsRefPtr and use .forget() in the appropriate places?
> 
> I suppose that we can. However, calling Release() off the main thread on
> these fields doesn't make sense and should cause crashes, so it's a bit
> weird to have a nsRefPtr. What do you think?

I think it's OK to have an nsRefPtr so long as you use .forget() on them in Succeed() and Fail().  That way we're not Release()'ing them off the main thread.

> > @@ +439,5 @@
> > > +  NS_METHOD Read(nsACString& aBuffer)
> > > +  {
> > > +    MOZ_ASSERT(!NS_IsMainThread());
> > > +
> > > +#if defined(XP_UNIX)
> > 
> > I think using nsIFile and friends here, even with mappings from XPCOM error
> > codes to platform error codes, would be much simpler in the long run.  The
> > mapping seems a little onerous right now because we only have one operation
> > implemented, but later operations will require minimal (if any) additions to
> > the mapping, whereas separate implementations mean we have to write
> > everything twice all the time.
> 
> I can try, but I'm not quite sure that there even is a conversion from XPCOM
> error codes to platform error codes. This might mean that I'll need to patch
> xpcom at some point in the future to ensure that error codes are correct.

Let's cross that bridge when we come to it.  Read and perhaps write are really the interesting operations that might need to be done natively, I think; everything else should be fine in JS.  At least, that's the ideal...
Attached patch 1. Native code, v5 (obsolete) — Splinter Review
Applied latest feedback.
Attachment #8367886 - Attachment is obsolete: true
Attachment #8367886 - Flags: review?(nfroyd)
Attachment #8367922 - Flags: review?(nfroyd)
Comment on attachment 8367922 [details] [diff] [review]
1. Native code, v5

Review of attachment 8367922 [details] [diff] [review]:
-----------------------------------------------------------------

Hey, it builds!  You will need a build peer to examine the moz.build changes, but they look fine to me.

This all looks OK, but the comments at the end suggest we are going to need a boatload of tests for this.

::: toolkit/components/osfile/NativeOSFileInternals.cpp
@@ +97,5 @@
> +  StringResult(PRTime aExecutionDuration, nsString* aResult)
> +    : AbstractResult(aExecutionDuration)
> +    , mResult(aResult)
> +  { }
> +  NS_IMETHOD GetResult(JSContext* cx, JS::MutableHandleValue aResult);

MOZ_OVERRIDE at the end.

@@ +124,5 @@
> +  TypedArrayResult(PRTime aExecutionDuration, nsCString* aResult)
> +    : AbstractResult(aExecutionDuration)
> +    , mResult(aResult)
> +  { }
> +  NS_IMETHOD GetResult(JSContext* cx, JS::MutableHandleValue aResult);

MOZ_OVERRIDE at the end.

@@ +259,5 @@
> +  nsresult Fail(const nsACString& aOperation, nsresult aStatus) {
> +#if defined(DEBUG)
> +    MOZ_ASSERT(!mResolved);
> +    mResolved = true;
> +#endif // defined(DEBUG)

Pull the entire conditional out into a private SetResolved method and call it from Fail() and Succeed().

@@ +324,5 @@
> +
> +    if (!mEncoding.Length()) {
> +      // No encoding, return as an ArrayBuffer
> +      nsCOMPtr<INativeOSFileResult> result = new TypedArrayResult(PR_Now() - start,
> +                                                                  buffer.forget());

nsRefPtr<TypedArrayResult>

@@ +357,5 @@
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));
> +    resultString->SetLength(maxChars);
> +
> +    nsRefPtr<INativeOSFileResult> result = new StringResult(PR_Now() - start,
> +                                                            resultString.forget());

nsRefPtr<StringResult>

@@ +373,5 @@
> +  NS_METHOD Read(nsACString& aBuffer)
> +  {
> +    MOZ_ASSERT(!NS_IsMainThread());
> +
> +

Nit: delete extraneous line.

@@ +417,5 @@
> +        Fail(NS_LITERAL_CSTRING("nsIFileInputStream::Read"), rv);
> +        return NS_ERROR_FAILURE;
> +      }
> +      total_read += just_read;
> +    } while (just_read != 0);

total_read != available would end the loop one iteration sooner, fwiw.  Would also let you stick just_read inside the loop instead.

Oh, but perhaps we can legitimately return 0 bytes read with NS_SUCCESS.  Bleh.  Keep it how it is.

@@ +424,5 @@
> +    if (mCompression.Length()) {
> +      // Not implemented
> +      Fail(NS_LITERAL_CSTRING("Decompression"), NS_ERROR_INVALID_ARG);
> +      return NS_ERROR_FAILURE;
> +    }

Check IsEmpty() here instead of Length().  Failing should really be done earlier for this case, but ideally nobody will be calling this outside of JS, which does the necessary checks.  I guess we can leave it here for now.

@@ +449,5 @@
> +{
> +  // Extract options
> +  nsCString encoding = NS_LITERAL_CSTRING("");
> +  nsCString compression = NS_LITERAL_CSTRING("");
> +  uint32_t bytes = (uint32_t)(-1);

Or just UINT32_MAX.

@@ +457,5 @@
> +
> +    // Extract encoding
> +    JS::Rooted<JS::Value> valEncoding(cx);
> +    if (JS_GetProperty(cx, options, "encoding", &valEncoding)
> +        && valEncoding.isString()) {

I think we should be asserting the typed-ness of all of these options.  So we should return NS_ERROR_FAILURE if !valEncoding.isString() and so on throughout.  Assuming I'm reading correctly, we "type-check" the options in the JS version by using them as their expected types and complaining at the point of usage.  Tests should show discrepancies here.  And if we don't have tests for this, we should write some.

(Though the JS versions probably perform coercion of arguments that the C++ version doesn't.  Bleh.  Perhaps we should strictly enforce types in the JS versions too?)
Attachment #8367922 - Flags: review?(nfroyd) → review+
Comment on attachment 8367887 [details] [diff] [review]
3. Calling the native back-end instead of the JS back-end whenever applicable, v2

Review of attachment 8367887 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the Cu.import business straightened out or answered well.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +850,5 @@
> +    options.bytes = bytes;
> +  }
> +
> +  if (options.compression || !nativeWheneverAvailable) {
> +    // Cannot use native implementation yet

Let's phrase this positively: // We must use the JavaScript implementation.

@@ +862,5 @@
> +        return new Uint8Array(data.buffer, data.byteOffset, data.byteLength);
> +      });
> +  }
> +
> +  // Otherwise, use the native implementation

Periods end sentences.

::: toolkit/components/osfile/modules/osfile_native.jsm
@@ +22,5 @@
> +} else {
> +  throw new Error("I am neither under Windows nor under a Posix system");
> +}
> +let {Promise} = Cu.import("resource://gre/modules/Promise.jsm", {});
> +let {XPCOMUtils} = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});

Is there a reason you're not Cu.import'ing into |this|, for all of these?

@@ +61,5 @@
> +
> +/**
> + * An OS.File error launched from XPCOM.
> + */
> +function NativeError(operation, status, stack) {

I like.
Attachment #8367887 - Flags: review?(nfroyd) → review+
Comment on attachment 8367897 [details] [diff] [review]
4. Tests, v3

Review of attachment 8367897 [details] [diff] [review]:
-----------------------------------------------------------------

Nothing wrong here, we just need more tests to ensure consistency of interface between JS OS.File and native OS.File.

::: toolkit/components/osfile/tests/xpcshell/test_read_write.js
@@ +30,5 @@
> +  do_get_profile();
> +  SHARED_PATH = OS.Path.join(OS.Constants.Path.profileDir, "test_osfile_read.tmp");
> +});
> +
> +// Check reading/writing with encodings

This should go in test_encodings.js.

@@ +128,5 @@
> +  yield test_with_options({}, "Not renaming, not flushing");
> +  yield test_with_options({flush: true}, "Not renaming, flushing");
> +});
> +
> +// Test that errors raise the correct exceptions

I think these could stand to be their own test_errors.js file, of which will become much larger based on the comments in part 1.  Or test_encodings.js.

add_test_pair should probably go in head.js, then.
Attachment #8367897 - Flags: review?(nfroyd) → feedback+
Attached patch 1. Native code, v6 (obsolete) — Splinter Review
Applied feedback.
Attachment #8367922 - Attachment is obsolete: true
Attachment #8368511 - Flags: review+
Unbitrotten.
Attachment #8367302 - Attachment is obsolete: true
Attachment #8368512 - Flags: review+
(In reply to Nathan Froyd (:froydnj) from comment #43)
> ::: toolkit/components/osfile/modules/osfile_native.jsm
> @@ +22,5 @@
> > +} else {
> > +  throw new Error("I am neither under Windows nor under a Posix system");
> > +}
> > +let {Promise} = Cu.import("resource://gre/modules/Promise.jsm", {});
> > +let {XPCOMUtils} = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
> 
> Is there a reason you're not Cu.import'ing into |this|, for all of these?

No strong reason.
Now that I know that it's possible to have an import that doesn't magically introduce symbols, but rather something that goes through the usual let-binding, I prefer doing this. IMHO, it's easier on the eye, plus IDEs have an easier time with it.

If you don't want it, I can move back to magic style.
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #47)
> (In reply to Nathan Froyd (:froydnj) from comment #43)
> > ::: toolkit/components/osfile/modules/osfile_native.jsm
> > @@ +22,5 @@
> > > +} else {
> > > +  throw new Error("I am neither under Windows nor under a Posix system");
> > > +}
> > > +let {Promise} = Cu.import("resource://gre/modules/Promise.jsm", {});
> > > +let {XPCOMUtils} = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
> > 
> > Is there a reason you're not Cu.import'ing into |this|, for all of these?
> 
> No strong reason.
> Now that I know that it's possible to have an import that doesn't magically
> introduce symbols, but rather something that goes through the usual
> let-binding, I prefer doing this. IMHO, it's easier on the eye, plus IDEs
> have an easier time with it.

This is in the eye of the beholder.

> If you don't want it, I can move back to magic style.

I was mostly curious what this does on B2G, but I suppose that the same XPCOMUtils object is returned, it's just a question of where you access it from: the temporary object you passed in or the global.
Applied feedback, improved type error checking for both implementations of OS.File.read.
Attachment #8367887 - Attachment is obsolete: true
Attachment #8368518 - Flags: review+
Attached patch 4. Tests, v4 (obsolete) — Splinter Review
Moooar tests. Plus reorganized as requested.
Attachment #8367897 - Attachment is obsolete: true
Attachment #8368519 - Flags: review?(nfroyd)
Comment on attachment 8368519 [details] [diff] [review]
4. Tests, v4

Review of attachment 8368519 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the error-checking!

::: toolkit/components/osfile/tests/xpcshell/test_read_write.js
@@ +2,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +let {utils: Cu} = Components;

Might as well take this out, since it is in head.js.
Attachment #8368519 - Flags: review?(nfroyd) → review+
Comment on attachment 8368512 [details] [diff] [review]
2. JS - Split Scheduler.post in two, v2

This patch has moved to bug 957123.
Attachment #8368512 - Attachment is obsolete: true
Note: The patch causes some mysterious regressions, which I'm currently investigating. Some of them seem to be due to tests that make weird assumptions about the scheduling of I/O (i.e. assume that interleaving between nsIFile I/O and OS.File I/O is stable).
Blocks: 970495
Attached patch 1. Native code, v7 (obsolete) — Splinter Review
A "few" changes to the native component since the previous version:
- we do not need to copy the contents of the ArrayBuffer/JSString on the main thread anymore, which should be a good bonus for reading 50Mb sessionstore.js files - this change was much more involved than I expected, and required me to visit the CycleCollector;
- we now read using nspr, rather than nsIFile;
- several fixes to corner-case memory usage.
Attachment #8368511 - Attachment is obsolete: true
Attachment #8375548 - Flags: review?(nfroyd)
Attachment #8375548 - Flags: review?(bzbarsky)
No meaningful changes here.
Attachment #8368518 - Attachment is obsolete: true
Attachment #8375549 - Flags: review+
Attached patch 3. Tests, v5 (obsolete) — Splinter Review
No meaningful changes here.
Attachment #8368519 - Attachment is obsolete: true
Attachment #8375550 - Flags: review+
Comment on attachment 8375548 [details] [diff] [review]
1. Native code, v7

Review of attachment 8375548 [details] [diff] [review]:
-----------------------------------------------------------------

You need a build peer for the moz.build changes.  bz knows many things, but he is not a build peer. :)

You are doing a lot of JSAPI manipulation (xpcpublic.h?!).  Maybe we should be building this as chrome-only webidl?

::: toolkit/components/osfile/NativeOSFileInternals.cpp
@@ +5,5 @@
> +/**
> + * Native implementation of some OS.File operations
> + */
> +
> +#include <nsString.h>

Please use "" for non-system headers.  Please double-check all of these.

@@ +155,5 @@
> +   *
> +   * @param aStartDate The instant at which the operation was
> +   * requested.  Used to collect Telemetry statistics.
> +   */
> +  AbstractResult(PRTime aStartDate)

We should be using TimeStamp here and throughout instead.

@@ +213,5 @@
> +NS_IMPL_CYCLE_COLLECTION_CLASS(AbstractResult)
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(AbstractResult)
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +  NS_INTERFACE_MAP_ENTRY(INativeOSFileResult)

Nit: I think it is typical to go from more specific to less specific, so reverse these please.

@@ +249,5 @@
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +/**
> + * Return a result as a string

Periods at the end of sentences.  Here and many other places.

@@ +291,5 @@
> +StringResult::GetResult(JSContext* cx, JS::MutableHandleValue aResult)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (GetCachedResult().isUndefined()) {

WYDT about something like:

AbstractResult::GetResult(...)
{
  if (GetCachedResult().isUndefined()) {
    nsresult rv = GetCacheableResult(cx, mCachedResult);
    if (NS_FAILED(rv)) {
      return rv;
    }
  }
  aResult.set(GetCachedResult());
  return NS_OK;
}

where GetCacheableResult is a protected virtual function?  This way you're not duplicating all the GetCachedResult logic.  Admittedly slower, but probably not too bad.  Classes can then implement GetResult directly if they don't want caching.

@@ +421,5 @@
> +  NS_METHOD Run() {
> +    MOZ_ASSERT(NS_IsMainThread());
> +    (void)mOnError->Complete(mOperation, mStatus, mOSError);
> +
> +    // Ensure that the callbacks are released on the main thread.

So I see this idiom here and in a couple of other places around the tree, and I don't really understand it.  Are you just being extra-paranoid that even though Run() is on the main thread, that ~$EVENT() won't be run on the main thread?  I think that level of paranoia is unjustified.  Please just let the event destructor do its work normally, and remove this stuff.

@@ +422,5 @@
> +    MOZ_ASSERT(NS_IsMainThread());
> +    (void)mOnError->Complete(mOperation, mStatus, mOSError);
> +
> +    // Ensure that the callbacks are released on the main thread.
> +    nsRefPtr<INativeOSFileSuccessCallback> onSuccess = nullptr;

The default constructor of nsRefPtr is to initialize with nullptr, no need to do it twice.

@@ +474,5 @@
> +    (void)mOnSuccess->Complete(mResult);
> +
> +    // Ensure that the callbacks are released on the main thread.
> +    nsRefPtr<INativeOSFileSuccessCallback> onSuccess = nullptr;
> +    nsRefPtr<INativeOSFileErrorCallback> onError = nullptr;

Here too.  And note that you forgot to do this for mResult. ;)

@@ +638,5 @@
> +    } while (just_read != 0);
> +
> +    // Compression
> +    if (!mCompression.IsEmpty()) {
> +      // Not implemented

We should be checking this up front in ::Read().

@@ +675,5 @@
> +    , mResult(new TypedArrayResult(PR_Now()))
> +  { }
> +
> +  ~DoReadToTypedArrayEvent() {
> +    // If AbstraactReadEvent::Run() has bailed out, we may need to cleanup

Nit typo: AbstractReadEvent.

@@ +678,5 @@
> +  ~DoReadToTypedArrayEvent() {
> +    // If AbstraactReadEvent::Run() has bailed out, we may need to cleanup
> +    // mResult, which is main-thread only data
> +    nsCOMPtr<nsIThread> main = do_GetMainThread();
> +    (void)NS_ProxyRelease(main, mResult);

Probably only worth doing this if mResult, since mResult would be nullptr after the .forget().

@@ +713,5 @@
> +  ~DoReadToStringEvent() {
> +    // If AbstraactReadEvent::Run() has bailed out, we may need to cleanup
> +    // mResult, which is main-thread only data
> +    nsCOMPtr<nsIThread> main = do_GetMainThread();
> +    (void)NS_ProxyRelease(main, mResult);

Likewise here.

@@ +809,5 @@
> +    JS::Rooted<JS::Value> valBytes(cx);
> +    if (JS_GetProperty(cx, options, "bytes", &valBytes)
> +        && valBytes.isNumber()) {
> +      bytes = valBytes.toInt32();
> +    }

Need to yell here if !valBytes.isNumber().

::: toolkit/components/osfile/NativeOSFileInternals.h
@@ +8,5 @@
> +#include "INativeOSFileInternals.h"
> +
> +namespace mozilla {
> +
> +class NativeOSFileInternalsService MOZ_FINAL: public INativeOSFileInternalsService {

Nit: space between MOZ_FINAL and :.

@@ +13,5 @@
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_INATIVEOSFILEINTERNALSSERVICE
> +private:
> +  void operator=(const NativeOSFileInternalsService* other) MOZ_DELETE;

MOZ_DELETE probably requires mozilla/Attributes.h, you should #include that.
Attachment #8375548 - Flags: review?(nfroyd) → feedback+
> Are you just being extra-paranoid that even though
> Run() is on the main thread, that ~$EVENT() won't be run on the main thread?

I have more review comments coming up, but this needs dealing with before David goes changing code.

He _should_ be paranoid about it.  We've had a number of bugs in worker code where the worker thread does an NS_DispatchToMainThread, then loses its timeslice before its nsRefPtr for the runnable can go out of scope, then the main thread runs the event, and then the worker thread runs its timeslice.  That results in the destructor of the runnable running on the worker thread.  We absolutely need to handle that case correctly here.
(In reply to Boris Zbarsky [:bz] from comment #59)
> > Are you just being extra-paranoid that even though
> > Run() is on the main thread, that ~$EVENT() won't be run on the main thread?
> 
> I have more review comments coming up, but this needs dealing with before
> David goes changing code.

As a side-note, I have been able to reproduce cases in which the destructor is called on either thread. So I'll stay paranoid :)
(In reply to Boris Zbarsky [:bz] from comment #59)
> > Are you just being extra-paranoid that even though
> > Run() is on the main thread, that ~$EVENT() won't be run on the main thread?
>
> He _should_ be paranoid about it.  We've had a number of bugs in worker code
> where the worker thread does an NS_DispatchToMainThread, then loses its
> timeslice before its nsRefPtr for the runnable can go out of scope, then the
> main thread runs the event, and then the worker thread runs its timeslice. 
> That results in the destructor of the runnable running on the worker thread.
> We absolutely need to handle that case correctly here.

I love threading.  I also keep forgetting that high levels of paranoia are completely justified when threading is involved.

smaug and khuey and I have been having similar discussion elsewhere in the context of already_AddRefed.  Maybe we should have an OffThreadRefPtr or something with appropriate assertions...
Comment on attachment 8375548 [details] [diff] [review]
1. Native code, v7

You probably need to file a comm-central bug to add this to their package-manifest.in files.  :(

>+++ b/toolkit/components/osfile/INativeOSFileInternals.idl
>+[scriptable, function, uuid(08B4CF29-3D65-4E79-B522-A694C322ED07)]
>+interface INativeOSFileResult: nsISupports

"function" doesn't make sense there, since this is an object the C++ will provide, right?

Also, I'd think "mozINativeOSFileResult" or "nsINativeOSFileResult" would be preferred.  That part is worth getting sr from a toolkit person for.  This part applies to all the interfaces defined in this file.

>+   * Duration of the off dispatch to the IO thread, in milliseconds.

Perhaps "delay between when the operation was requested on the main thread and when it began running on the I/O thread"?

>+  readonly attribute unsigned long dispatchDurationMS;

Is there a reason to restrict to an integer number of ms here?  DOMHighResTimeStamp (or "double" if you don't want to include domstubs.idl) may be a better fit here.

>+  readonly attribute unsigned long executionDurationMS;

Likewise.

Do we want to also expose the time between the operation completing on the background thread and the callback getting invoked on the main thread?

>+   * @param operation The name of the failed operation.

Is there a fixed list of these?  Are these names something we generate internally, or are they based in some way on what was passed to read()?  Are these names ASCII or UTF-8 or something else?

>+   * @param path The path to the file to read.

Absolute path, right?  In what encoding?

>+++ b/toolkit/components/osfile/NativeOSFileInternals.cpp
>+#include <nsString.h>

This, and other non-system headers, should be in "", not <>, I believe.

>+  const static void release(type ptr) {

That const looks pretty odd.  I realize the API docs for Scoped do that too, but it still looks odd.  ;)

>+ * Base class for results passed to the callbacks.
...
>+ * when we transfer data allocate on another thread into JS. Note that

s/allocate/allocated/

>+class AbstractResult: public INativeOSFileResult {
>+  AbstractResult(PRTime aStartDate)

Is there a reason we're using PRTime here instead of mozilla::TimeStamp/TimeDuration?  The latter has the benefit of having a monotonic clock that's actually useful for actually measuring intervals, and it seems like intervals are what you're primarily after here... I think TimeStamp/TimeDuration would be a better fit.  As a bonus, there are nice methods for asking for the number of ms in a TimeDuration.

>+protected:
>+  JS::Heap<JS::Value>& GetCachedResult() {

Probably simpler to just have this return a JS::Value, actually.

>+  void SetCachedResult(JS::Heap<JS::Value>& result) {

This should take a "JS::Handle<JS::Value> result".

>+AbstractResult::GetResult(JSContext*, JS::MutableHandleValue)
>+{
>+  return NS_ERROR_NOT_IMPLEMENTED;

You could just not NS_DECL_INATIVEOSFILERESULT in this class and leave GetResult pure virtual instead (but manually declare the two duration getters).  I think that would be clearer and harder to mess up if someone adds a new subclass.

>+class StringResult MOZ_FINAL: public AbstractResult
>+  NS_DECL_ISUPPORTS_INHERITED

Why do you need this?  If you're not implementing any additional interfaces, seems like you could just inherit nsISupports stuff completely from AbstractResult.

>+   * @param aContents The string to pass to JavaScript. Ownership of the
>+   * string and its contents is passed to StringResult. The string must
>+   * be UTF-8.

An nsString is always made up of 16-bit code points.  Do you mean it must be valid UTF-16?

>+  ScopedDeletePtr<nsString> mContents;

Is there a reason this is not just using an nsString member?  That seems like it would save an extra heap allocation...

>+StringResult::GetResult(JSContext* cx, JS::MutableHandleValue aResult)

>+    JS::Rooted<JS::Value> rootedResult(cx, JS::UndefinedValue());

JS::UndefinedValue is the default for a Rooted<Value>, so no need to do it explicitly here.

>+    mContents.forget();

This leaks the mContents object (but not its string data), no?  All the more reason to not use a ScopedDeletePtr here.

>+    JS::Heap<JS::Value> result(rootedResult);
>+    SetCachedResult(result);

  SetCachedResult(rootedResult);

>+class TypedArrayResult MOZ_FINAL: public AbstractResult
>+  NS_DECL_ISUPPORTS_INHERITED

Again, shouldn't be needed.

>+TypedArrayResult::GetResult(JSContext* cx, JS::MutableHandle<JS::Value> aResult)
>+  // ArrayBuffers with the same underlying C buffer. As this would be
>+  // very unsafe, we need to cache the result once we have it.

Also, it would be completely messed up API from a JS perspective to have .result return a new value each time.

>+    ArrayBufferContents contents = mContents.get();

Making this a const ref would avoid the extra copying...

>+    if (!arrayBuffer.get()) {

Please drop the .get().

>+    JS::Heap<JSObject*> result(

This should either be a raw JSObject* or a JS::Rooted<JSObject*>.  The latter is probably clearer/safer.

>+      JS_NewUint8ArrayWithBuffer(cx, arrayBuffer,
>+                                 0, mContents.get().nbytes));

Why not "contents.nbytes" for that last argument?

>+    if (!result.get()) {

No need for .get().

>+    JS::Heap<JS::Value> valResult;
>+    valResult.setObject(*result);
>+    SetCachedResult(valResult);

  SetCachedResult(JS::ObjectValue(*result));

>+class ErrorEvent MOZ_FINAL: public nsRunnable {

Need to document the aOSError constructor argument.

> Also, we pass the callbacks as
>+   * alread_AddRefed to ensure that we do not manipulate main-thread
>+   * only refcounters off the main thread.

Another option is to pass these as nsRefPtr<whatever>& and swap() into our members.  I _think_ what you have now is ok, though.

>+  ErrorEvent(already_AddRefed<INativeOSFileSuccessCallback> aOnSuccess,

This needs to assert which thread it's called on.

>+    { }
>+  NS_METHOD Run() {

Blank line in between those, please.

>+    // Ensure that the callbacks are released on the main thread.
>+    nsRefPtr<INativeOSFileSuccessCallback> onSuccess = nullptr;
>+    nsRefPtr<INativeOSFileErrorCallback> onError = nullptr;
>+    nsRefPtr<AbstractResult> discardedResult = nullptr;
>+    mOnSuccess.swap(onSuccess);
>+    mOnError.swap(onError);
>+    mDiscardedResult.swap(discardedResult);

That seems like a very roundabout way of saying:

  mOnSuccess = nullptr;
  mOnError = nullptr;
  mDiscardedResult = nullptr;

>+class SuccessEvent MOZ_FINAL: public nsRunnable {

Similar comments here (assert thread in ctor, simplify the cleanup in Run).

>+  NS_METHOD Run() {

Why do we not need to null out mResult here?  Seems to me like we do...

>+class AbstractDoEvent: public nsRunnable {

Constructor should assert which thread it's running on.

>+  void Fail(const nsACString& aOperation,
>+    (void)NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);

If dispatch fails, I would almost prefer we event.forget() (leaking it) rather than release stuff on the wrong threads...

>+  void Succeed(already_AddRefed<INativeOSFileResult> aResult) {
>+    (void)NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);

And here.

>+class AbstractReadEvent: public AbstractDoEvent {
>+  NS_METHOD Run() MOZ_OVERRIDE {

Might as well assert not-mainthread here.

>+  NS_METHOD Read(ScopedArrayBufferContents& aBuffer)

Is there a reason for this to be NS_METHOD as opposed to "virtual nsresult" or just non-virtual?

>+      Fail(NS_LITERAL_CSTRING("open"), nullptr, NS_OK, PR_GetOSError());

Why PR_GetOSError(), by the way, as opposed to PR_GetError()?

Also, should we consider sending an error nsresult even if we're also providing an nspr-level error?

>+    uint32_t bytes = std::min((uint32_t)stat.size, mBytes);

The API didn't have anything to say about this only being ok for files under 2^32 bytes...  I think we should be using 64-bit byte counts as much as we can.  So PRFileInfo64, PR_GetOpenFileInfo64, etc. And probably explicitly reporting an error here if the result is too big to fit in a typed array, since _that_ is the only current 32-bit limitation.  And even that will get removed in the not too far off future, as I understand.

What I don't think we want to be doing is doing a 1-byte read on a 2^32+1 byte file....

Note that PR_Read will fail if your "bytes" is > 2^31, so the last argument to that function needs to be clamped to not be too big no matter what.

>+    } while (just_read != 0);

After this, maybe assert that bytes == total_read?

>+    if (!mCompression.IsEmpty()) {

Would be good to fail this before we start reading.

>+class DoReadToTypedArrayEvent MOZ_FINAL: public AbstractReadEvent {
>+    // If AbstraactReadEvent::Run() has bailed out, we may need to cleanup

One too many 'a' in Abstract.

>+  void AfterRead(PRTime dispatchDate,
>+                 ScopedArrayBufferContents& aBuffer) MOZ_OVERRIDE {

aDispatchData?

>+class DoReadToStringEvent MOZ_FINAL: public AbstractReadEvent {
>+  void AfterRead(PRTime dispatchDate,
>+                 ScopedArrayBufferContents& aBuffer) MOZ_OVERRIDE {

And here.

>+    if (!dom::EncodingUtils::FindEncodingForLabel(mEncoding, encodingName)) {
>+      Fail(NS_LITERAL_CSTRING("Decode"), mResult.forget(), NS_ERROR_INVALID_ARG);

We could do this up-front on the main thread too, before reading all the data, right?

>+    nsCOMPtr<nsIUnicodeDecoder> decoder = dom::EncodingUtils::DecoderForEncoding(encodingName);

I assume that this is threadsafe, right?

>+    nsresult rv = decoder->GetMaxLength(sourceChars, sourceBytes, &maxChars);

Hrm.  Doing large files here would be pretty tough.  We should probably just make sure we error out (earlier, before reading the data) in the string case if the file is too big.

>+    if (NS_FAILED(rv)) {
>+      Fail(NS_LITERAL_CSTRING("GetMaxLength"), mResult.forget(), NS_ERROR_INVALID_ARG);

Why not the actual rv that was returned?

>+    if (sourceBytes < 0) {

How can that happen, exactly, short of overflow?  We should be detecting overflow in a better way.

On the other hand, checking maxChars....

>+    resultString->SetLength(maxChars);

Should check the result of that call (i.e. what the string length is now) to make sure it actually worked.  Otherwise we'll scribble on memory.

>+NativeOSFileInternalsService::Read(const nsACString& aPath,
>+  nsCString encoding = NS_LITERAL_CSTRING("");
>+  nsCString compression = NS_LITERAL_CSTRING("");

Empty string is the default value anyway; don't need initializers.

>+  if (!aOptions.isPrimitive()) {

So if "5" is passed in we want to silently ignore it, rather than throw?

If we're willing to throw on primitives, this would work a lot better as a WebIDL dictionary (or xpidl, but the webidl ones are nicer), I suspect.  I haven't bothered reviewing the JSAPI usage here in detail, on the assumption that this code will change.

>+  if (encoding.IsEmpty()) {

That's not quite the same as "provided"...  Worth documenting that what really matters is whether ToString(encoding) is "".  Though if you go the dictionary route, I assume that undefined will also map to ""...

>+++ b/toolkit/components/osfile/NativeOSFileInternals.h
>+  void operator=(const NativeOSFileInternalsService* other) MOZ_DELETE;

Please document why this is needed.

I'd appreciate an interdiff for the next round of review...
Attachment #8375548 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky [:bz] from comment #62)
> Comment on attachment 8375548 [details] [diff] [review]
> 1. Native code, v7
> 
> You probably need to file a comm-central bug to add this to their
> package-manifest.in files.  :(

Will do.

> Also, I'd think "mozINativeOSFileResult" or "nsINativeOSFileResult" would be
> preferred.  That part is worth getting sr from a toolkit person for.  This
> part applies to all the interfaces defined in this file.

I'm a toolkit person, do I count? :)

Asking on dev-platform.

> Is there a reason to restrict to an integer number of ms here?  DOMHighResTimeStamp (or "double" if you don't want to include domstubs.idl) may be a better fit here.

Well, DOMHighResTimeStamp only makes sense if I have a window and a document and I have neither. I can switch to double, though.

> 
> >+  readonly attribute unsigned long executionDurationMS;
> 
> Likewise.
> 
> Do we want to also expose the time between the operation completing on the
> background thread and the callback getting invoked on the main thread?

I don't need this information at the moment, I figure I can always add it later if necessary.

> >+   * @param operation The name of the failed operation.
> 
> Is there a fixed list of these?  Are these names something we generate
> internally, or are they based in some way on what was passed to read()?  Are
> these names ASCII or UTF-8 or something else?

There is no fixed list, these are purely to help debugging. These names are ASCII.

> >+   * @param path The path to the file to read.
> 
> Absolute path, right?  In what encoding?

Absolute path, indeed. As PR_Open uses native encoding, moving to PR_OpenUTF16 to avoid the issue.

> >+++ b/toolkit/components/osfile/NativeOSFileInternals.cpp
> >+  const static void release(type ptr) {
> 
> That const looks pretty odd.  I realize the API docs for Scoped do that too,
> but it still looks odd.  ;)

Well, I'm keeping consistent with Scoped :)

> >+AbstractResult::GetResult(JSContext*, JS::MutableHandleValue)
> >+{
> >+  return NS_ERROR_NOT_IMPLEMENTED;
> 
> You could just not NS_DECL_INATIVEOSFILERESULT in this class and leave
> GetResult pure virtual instead (but manually declare the two duration
> getters).  I think that would be clearer and harder to mess up if someone
> adds a new subclass.

I decided to follow froydnj's suggestion and rather leave GetCacheableResult pure virtual.

> >+   * @param aContents The string to pass to JavaScript. Ownership of the
> >+   * string and its contents is passed to StringResult. The string must
> >+   * be UTF-8.
> 
> An nsString is always made up of 16-bit code points.  Do you mean it must be
> valid UTF-16?

Indeed, that's what I meant.

> >+  ScopedDeletePtr<nsString> mContents;
> 
> Is there a reason this is not just using an nsString member?  That seems
> like it would save an extra heap allocation...

Ah, right. Since xpcshell will handle stealing the underlying buffer, I don't need to manage stealing through a scoped pointer.

> > Also, we pass the callbacks as
> >+   * alread_AddRefed to ensure that we do not manipulate main-thread
> >+   * only refcounters off the main thread.
> 
> Another option is to pass these as nsRefPtr<whatever>& and swap() into our
> members.  I _think_ what you have now is ok, though.

I tend to believe that already_AddRefed is slightly nicer.
I could try and introduce some RAII objects with type-based guarantees that we do not addref/release off the main thread, if you want.

> >+  void Fail(const nsACString& aOperation,
> >+    (void)NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
> 
> If dispatch fails, I would almost prefer we event.forget() (leaking it)
> rather than release stuff on the wrong threads...

Let's NS_ProxyRelease it in case of error.

> >+      Fail(NS_LITERAL_CSTRING("open"), nullptr, NS_OK, PR_GetOSError());
> 
> Why PR_GetOSError(), by the way, as opposed to PR_GetError()?
> 
> Also, should we consider sending an error nsresult even if we're also
> providing an nspr-level error?

The rest of OS.File uses platform-specific OS-level errors, so I'm trying to stick to this as much as I can. However, you are right, let's just get rid of the nsresult.

> 
> >+    uint32_t bytes = std::min((uint32_t)stat.size, mBytes);
> 
> The API didn't have anything to say about this only being ok for files under
> 2^32 bytes...  I think we should be using 64-bit byte counts as much as we
> can.  So PRFileInfo64, PR_GetOpenFileInfo64, etc. And probably explicitly
> reporting an error here if the result is too big to fit in a typed array,
> since _that_ is the only current 32-bit limitation.  And even that will get
> removed in the not too far off future, as I understand.
> 
> What I don't think we want to be doing is doing a 1-byte read on a 2^32+1
> byte file....
> 
> Note that PR_Read will fail if your "bytes" is > 2^31, so the last argument
> to that function needs to be clamped to not be too big no matter what.
> 
> >+    } while (just_read != 0);
> 
> After this, maybe assert that bytes == total_read?

We could theoretically have a race condition with another process truncating the file between PR_GetOpenFileInfo64 and PR_Read, which would result in bytes != total_read, so I'm not going to assert, but I'll raise an error in case this happens.

> And here.
> 
> >+    if (!dom::EncodingUtils::FindEncodingForLabel(mEncoding, encodingName)) {
> >+      Fail(NS_LITERAL_CSTRING("Decode"), mResult.forget(), NS_ERROR_INVALID_ARG);
> 
> We could do this up-front on the main thread too, before reading all the
> data, right?

We certainly could. I preferred moving as much work as possible off the main thread.

> >+    nsCOMPtr<nsIUnicodeDecoder> decoder = dom::EncodingUtils::DecoderForEncoding(encodingName);
> 
> I assume that this is threadsafe, right?

Documentation (or macros) don't seem to mention anything about thread-safety, but this function is used in the Web Workers implementation of TextDecoder, so I assume taht it is.


> >+    nsresult rv = decoder->GetMaxLength(sourceChars, sourceBytes, &maxChars);
> 
> Hrm.  Doing large files here would be pretty tough.  We should probably just
> make sure we error out (earlier, before reading the data) in the string case
> if the file is too big.

What kind of bounds do you have in mind?

> >+    if (sourceBytes < 0) {
> 
> How can that happen, exactly, short of overflow?  We should be detecting
> overflow in a better way.
> 
> On the other hand, checking maxChars....

I was thinking of overflow, indeed, but you are right, I got confused in the arguments.

> >+  if (!aOptions.isPrimitive()) {
> 
> So if "5" is passed in we want to silently ignore it, rather than throw?
> 
> If we're willing to throw on primitives, this would work a lot better as a
> WebIDL dictionary (or xpidl, but the webidl ones are nicer), I suspect.  I
> haven't bothered reviewing the JSAPI usage here in detail, on the assumption
> that this code will change.

Sounds good to me. Do you have any examples?

> >+++ b/toolkit/components/osfile/NativeOSFileInternals.h
> >+  void operator=(const NativeOSFileInternalsService* other) MOZ_DELETE;
> 
> Please document why this is needed.

I'm actually not completely sure that it is. Some reviewer insisted that it was a good precaution for another service, so I went along with the review.

> I'd appreciate an interdiff for the next round of review...

Will do.
(In reply to Nathan Froyd (:froydnj) from comment #58)
> You need a build peer for the moz.build changes.  bz knows many things, but
> he is not a build peer. :)
> 
> You are doing a lot of JSAPI manipulation (xpcpublic.h?!).

xpcpublic.h seems to be the best way to transfer a string to JS without copy (source: bz)

> Maybe we should
> be building this as chrome-only webidl?

I'm willing to learn the frightful webidl, if need arises. Why should I pick one over the other?


> ::: toolkit/components/osfile/NativeOSFileInternals.h
> @@ +8,5 @@
> > +#include "INativeOSFileInternals.h"
> > +
> > +namespace mozilla {
> > +
> > +class NativeOSFileInternalsService MOZ_FINAL: public INativeOSFileInternalsService {
> 
> Nit: space between MOZ_FINAL and :.

But, but, but, I thought we programmed in English?
Attached patch 1a. Native code interdiff (obsolete) — Splinter Review
Applied the feedback, here is the cumulative difference.
Bz, is this what you wanted or do you prefer an actual interdiff?
Attachment #8377193 - Flags: review?(nfroyd)
Attachment #8377193 - Flags: review?(bzbarsky)
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #63)
> > >+   * @param path The path to the file to read.
> > 
> > Absolute path, right?  In what encoding?
> 
> Absolute path, indeed. As PR_Open uses native encoding, moving to
> PR_OpenUTF16 to avoid the issue.

Except I can't, PR_OpenUTF16 doesn't seem to be built. Right now, I'm using native encoding, which is consistent with JS-based OS.File. We might need to fix both at once at some point.
Attached patch 1a. Native code interdiff, v2 (obsolete) — Splinter Review
Small tweaks:
- added a (very) short description of the error constant macros;
- added an off-main thread check that the encoding exists before any actual I/O.
Attachment #8377193 - Attachment is obsolete: true
Attachment #8377193 - Flags: review?(nfroyd)
Attachment #8377193 - Flags: review?(bzbarsky)
Attachment #8377524 - Flags: review?(nfroyd)
Attachment #8377524 - Flags: review?(bzbarsky)
> I'm a toolkit person, do I count? :)

Yes.  ;)

> DOMHighResTimeStamp only makes sense if I have a window and a document

Sort of.  The DOM specs suck and use it for both document-relative times and generic time intervals.  I've tried to talk them out of the latter, but failed.  In any case, double is good enough; from JS you can't tell the difference.

> I don't need this information at the moment

OK.  I wasn't sure what exactly we were trying to time when timing these various delays; the time for the entire operation to look "done" on main thread would obviously include the mainthread event loop delay.

> There is no fixed list, these are purely to help debugging. These names are
> ASCII.

OK.  Just document that, please?

> I could try and introduce some RAII objects with type-based guarantees that we
> do not addref/release off the main thread, if you want.

Let's not worry about that for now.

> Let's NS_ProxyRelease it in case of error.

That seems reasonable (though chances are it'll also leak under the hood, if the main thread is not accepting events).

> I preferred moving as much work as possible off the main thread.

OK.  Should probably document in the IDL how invalid encoding labels are treated.

> What kind of bounds do you have in mind?

Well, GetMaxLength can't possibly work right for input lengths larger than INT32_MAX, right?  I really hope it handles integer overflow correctly for lengths close to that and returns an error...

> Sounds good to me. Do you have any examples?

For WebIDL dictionaries, something like WifiCommandOptions might be a reasonable example.

> Some reviewer insisted that it was a good precaution for another service

It seems like a good precaution if the operator= takes a const ref.  But doing this for an assignment operator taking a const ptr is just odd and seems pointless to me...

> I'm willing to learn the frightful webidl

Why is it frightful?  Genuine question, feel free to answer it in email.

The main benefit of using webidl over xpidl is that it does a better job of mapping modern JS concepts to C++ objects so you end up not needing as much manual JSAPI, typically.

> Bz, is this what you wanted or do you prefer an actual interdiff?

It's what I wanted, if it's the diff between "the code that would have resulted if the patch I was reviewing were applied" and "the code that will result if this new patch I want you to review is applied".  At first glance, looks like that's what you posted, so good.  :)
Oh, and the main benefit of using xpidl over webidl is that it plays nicer with things like getting objects via getService and whatnot...
Comment on attachment 8377524 [details] [diff] [review]
1a. Native code interdiff, v2

Review of attachment 8377524 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is closer, but I think another go would be good.  I feel like I caught a few more things this time; bz probably has other suggestions.

::: toolkit/components/osfile/NativeOSFileInternals.cpp
@@ +18,5 @@
> +
> +#include "nsINativeOSFileInternals.h"
> +#include "NativeOSFileInternals.h"
> +
> +

Nit: extra space.

@@ +31,5 @@
> +
> +#include "prio.h"
> +#include "prerror.h"
> +
> +

Here too.

@@ +36,5 @@
> +#include "jsapi.h"
> +#include "jsfriendapi.h"
> +#include "js/Utility.h"
> +#include "xpcpublic.h"
> +

And here.

@@ +136,5 @@
>  };
>  
>  ///////// Results of OS.File operations
>  
> +// Platform specific constants

Please explain briefly why you're using these, rather than mapping from nsresult or similar.

@@ +365,5 @@
>  
> +  // The memory of contents has been allocated on a thread that
> +  // doesn't have a JSRuntime, hence without a context. Now that we
> +  // have a context, attach the memory to where it belongs.
> +  JS_updateMallocCounter(cx, contents.nbytes);

Technically, it's not correct to adjust this counter until we have successfully allocated the array buffer, correct?

@@ +379,5 @@
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +  JS::Heap<JS::Value> valResult;
> +  valResult.setObject(*result);
> +  mContents.forget();

Hm, should we be doing this earlier once we hand off contents.header to JS?  Otherwise allocating the Uint8Array could fail, and we'd try to free the buffer, then JS would try to free the buffer (?), and we would have issues.

@@ +529,5 @@
> +      // Last ditch attempt to release on the main thread - event is
> +      // not thread-safe, so letting the pointer go out of scope would
> +      // cause a crash.
> +      nsCOMPtr<nsIThread> main = do_GetMainThread();
> +      NS_ProxyRelease(main, event);

If you are going to NS_ProxyRelease, you should do something like:

nsIRunnable* rawEvent = nullptr;
event.swap(rawEvent);
NS_ProxyRelease(main, rawEvent);

as NS_ProxyRelease will DTRT and leak the object if the event can't be dispatched.

@@ +547,5 @@
> +      // Last ditch attempt to release on the main thread - event is
> +      // not thread-safe, so letting the pointer go out of scope would
> +      // cause a crash.
> +      nsCOMPtr<nsIThread> main = do_GetMainThread();
> +      NS_ProxyRelease(main, event);

Likewise.

@@ +629,4 @@
>    {
>      MOZ_ASSERT(!NS_IsMainThread());
>  
> +    PRFileDesc* file = PR_OpenFile(NS_ConvertUTF16toUTF8(mPath).get(),

PR_OpenFileUTF16?

@@ +652,5 @@
>      uint64_t total_read = 0;
>      int32_t just_read = 0;
>      char* dest_chars = reinterpret_cast<char*>(aBuffer.rwget().data);
>      do {
> +      int64_t to_read = bytes - just_read;

I don't think just_read is the right thing here.  If somebody's growing the file underneath you, you're going to buffer overflow.

@@ +655,5 @@
>      do {
> +      int64_t to_read = bytes - just_read;
> +      if (to_read < 0 || to_read > UINT32_MAX) {
> +        to_read = UINT32_MAX;
> +      }

PR_Read takes a signed amount to read, not unsigned.  This would not DTRT for large files.

@@ +663,4 @@
>          return NS_ERROR_FAILURE;
>        }
>        total_read += just_read;
> +    } while (just_read != 0 && total_read <= bytes);

I think <= here is wrong, because you don't want to:

1. Read |bytes|
2. Somebody appends content
3. Read again, but you're now scribbling over memory at the end of your allocated buffer.

@@ +719,2 @@
>      nsCOMPtr<nsIThread> main = do_GetMainThread();
>      (void)NS_ProxyRelease(main, mResult);

Likewise on NS_ProxyRelease here.

@@ +756,2 @@
>      nsCOMPtr<nsIThread> main = do_GetMainThread();
>      (void)NS_ProxyRelease(main, mResult);

Likewise on NS_ProxyRelease here.

@@ +859,5 @@
>      JS::Rooted<JS::Value> valBytes(cx);
> +    if (JS_GetProperty(cx, options, "bytes", &valBytes)) {
> +      if (!valBytes.isNumber()) {
> +        return NS_ERROR_INVALID_ARG;
> +      }

Super-nit: can you fix the other argument-checking cases to do this early-return style?

::: toolkit/components/osfile/nsINativeOSFileInternals.idl
@@ +9,5 @@
>  
>  /**
>   * The result of a successful asynchronous operation.
>   */
> +[scriptable, uuid(08B4CF29-3D65-4E79-B522-A694C322ED07)]

Probably super-nit here, but can you [builtinclass] all of these, so we don't get anybody attempting to implement them in JS?

@@ +57,2 @@
>     */
> +  void complete(in ACString operation, in long OSstatus);

The JS glue is going to have to be updated for this change.
Attachment #8377524 - Flags: review?(nfroyd) → feedback+
(In reply to Nathan Froyd (:froydnj) from comment #70)
> @@ +529,5 @@
> > +      // Last ditch attempt to release on the main thread - event is
> > +      // not thread-safe, so letting the pointer go out of scope would
> > +      // cause a crash.
> > +      nsCOMPtr<nsIThread> main = do_GetMainThread();
> > +      NS_ProxyRelease(main, event);
> 
> If you are going to NS_ProxyRelease, you should do something like:
> 
> nsIRunnable* rawEvent = nullptr;
> event.swap(rawEvent);
> NS_ProxyRelease(main, rawEvent);
> 
> as NS_ProxyRelease will DTRT and leak the object if the event can't be
> dispatched.

Argh, ignore me, I totally overlooked the NS_ProxyRelease smart pointer overloads.
(In reply to Nathan Froyd (:froydnj) from comment #70)
> @@ +629,4 @@
> >    {
> >      MOZ_ASSERT(!NS_IsMainThread());
> >  
> > +    PRFileDesc* file = PR_OpenFile(NS_ConvertUTF16toUTF8(mPath).get(),
> 
> PR_OpenFileUTF16?

Unfortunately, http://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/io/prfile.c#740 suggests that the symbol is not built – and ld agrees.

> @@ +57,2 @@
> >     */
> > +  void complete(in ACString operation, in long OSstatus);
> 
> The JS glue is going to have to be updated for this change.

Yeah, but since it's a pretty trivial change (removing NativeError and two lines of code), I figured that it wasn't worth requesting a review atm.
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #72)
> (In reply to Nathan Froyd (:froydnj) from comment #70)
> > @@ +629,4 @@
> > >    {
> > >      MOZ_ASSERT(!NS_IsMainThread());
> > >  
> > > +    PRFileDesc* file = PR_OpenFile(NS_ConvertUTF16toUTF8(mPath).get(),
> > 
> > PR_OpenFileUTF16?
> 
> Unfortunately,
> http://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/io/prfile.c#740
> suggests that the symbol is not built – and ld agrees.

Ah, I had assumed that was what the MOZ_UNICODE define was for at the top of the file.  But I failed to consider that the implementation would have to be compiled in too.  If the MOZ_UNICODE bit isn't useful for anything else, please remove.
Attached patch 1a. Native code interdiff, v3 (obsolete) — Splinter Review
Applied Nathan's feedback (minus NS_ProxyRelease).
I haven't attempted to switch to WebIDL because of this answer:
http://ask.mozilla.org/question/144/xpidl-or-webidl/?answer=149#post-id-149

Given that OS.File is typically used before we create any window, it looks like this wouldn't be the right usage scenario. Do you concur, bz?
Attachment #8377524 - Attachment is obsolete: true
Attachment #8377524 - Flags: review?(bzbarsky)
Attachment #8378216 - Flags: review?(nfroyd)
Attachment #8378216 - Flags: review?(bzbarsky)
I think medium-term we will in fact make it possible to use WebIDL easily in non-window contexts, and even now we support all sorts of WebIDL objects (c.f. TextEncoder) there...  But let's not worry about converting working code for the moment.

I'll try to get to this patch in the next few hours.
Comment on attachment 8378216 [details] [diff] [review]
1a. Native code interdiff, v3

I'm really sorry about the review lag here.  :(

>+++ b/toolkit/components/osfile/NativeOSFileInternals.cpp
>+#if defined(XP_UNIX)
>+#elif defined(XP_WIN)

Please add an #else with an #error "Need to define error constants for this platform" or some such?

>+  virtual nsresult GetCacheableResult(JSContext *cx, JS::MutableHandleValue aResult) = 0;

This should probably be protected, not public, right?  Both here and in the subclasses.

>+  JS::Rooted<JS::Value> rootedResult(cx);

You don't really need this temporary.  You can just do:

  if (!xpc::StringToJsval(cx, mContents, aResult)) {
    return NS_ERROR_FAILURE;
  }
  return NS_OK;

>+TypedArrayResult::GetCacheableResult(JSContext* cx, JS::MutableHandle<JS::Value> aResult)
>+  ArrayBufferContents contents(mContents.get());

Please document that this can't be a const ref because you have to copy, since you plan to use contents.nbytes after calling mContents.forget().

It would be better, perhaps, if we did the mContents.forget() call after the JS_NewUint8ArrayWithBuffer but before checking whether "result" is null.

>+  if (!arrayBuffer.get()) {

The get() here should go away.

>+  JS_updateMallocCounter(cx, contents.nbytes);

Probably worth doing this after doing the actual JS_NewUint8ArrayWithBuffer call and checking that it succeeded.

>+  JS::Heap<JS::Value> valResult;
>+  valResult.setObject(*result);
>+  aResult.set(valResult);

This should just be:

  aResult.setObject(*result);

> class AbstractDoEvent: public nsRunnable {
>   void Fail(const nsACString& aOperation,
>+      // Last ditch attempt to release on the main thread - event is
>+      // not thread-safe

More precisely, some of its members are not threadsafe.

>+  void Succeed(already_AddRefed<nsINativeOSFileResult> aResult) {

Same in this method.

> class AbstractReadEvent: public AbstractDoEvent {
>+  nsresult Read(ScopedArrayBufferContents& aBuffer)
>+    PRFileDesc* file = PR_OpenFile(NS_ConvertUTF16toUTF8(mPath).get(),

Why OpenFile instead of Open?  If there's a reason, please document it.

>     if (!aBuffer.Allocate(bytes)) {

We need to bail out here if bytes doesn't fit in uint32_t, since that's what Allocate takes.  Otherwise we can overflow, under-allocate, and then write the file contents to memory we don't own...  Once we can have array buffers with bigger lengths, we can revisit this code, I guess.  :(

>       just_read = PR_Read(file, dest_chars + total_read, bytes - total_read);

That last arg needs to be std::min(uint64_t(PR_INT32_MAX), bytes - total_read), I believe. 

>+class DoReadToStringEvent MOZ_FINAL : public AbstractReadEvent {
>+  void AfterRead(TimeStamp aDispatchDate,
>     int32_t sourceBytes = aBuffer.get().nbytes;

This is assuming nbytes in fact fits in int32_t, not just uint32_t.  If it doesn't, need to error out.

>+NativeOSFileInternalsService::Read(const nsAString& aPath,

OK, if we're sticking with the manual JSAPI usage...

>   if (!aOptions.isPrimitive()) {

If you mean aOptions.isObject(), just test that.

>     if (JS_GetProperty(cx, options, "encoding", &valEncoding)

If JS_GetProperty returned false, an exception was thrown.  You need to immediately return an error nsresult without performing any more JSAPI calls on cx.  This applies in several places.

>+      encoding = NS_ConvertUTF16toUTF8(JS_GetStringCharsZ(cx, strEncoding));

  CopyUTF16toUTF8(JS_GetStringCharsZ(cx, strEncoding), encoding);

should avoid the extra object ctor/dtor.

>+      if (!valBytes.isNumber()) {

So if someone passes in "12" instead of 12 by accident (e.g. they read it from some API like an input.value or whatnot) we'll throw.  Is that by design?

>       bytes = valBytes.toInt32();

You don't know that valBytes is an integer value.  All you know is it's a number.  It might not be an integer at all, or it might be 2^32 or bigger (which would not fit in an int32 value).

I really do think you should use a dictionary here so as to avoid the manual JSAPI usage...

>+++ b/toolkit/components/osfile/NativeOSFileInternals.h
>+  // Avoid accidental use of built-in operator=
>   void operator=(const NativeOSFileInternalsService* other) MOZ_DELETE;

The built-in takes a const ref, not a const pointer.  So if you're trying to avoid it, this needs to take a const ref.

>+++ b/toolkit/components/osfile/nsINativeOSFileInternals.idl
>+#include "domstubs.idl"

Isn't needed if you're not using DOMHighResTimeStamp.

r=me with the above fixed, but I'd like to make sure I read over whatever you end up doing with the JSAPI bits in NativeOSFileInternalsService::Read before you land this.
Attachment #8378216 - Flags: review?(bzbarsky) → review+
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #72)
> > > +    PRFileDesc* file = PR_OpenFile(NS_ConvertUTF16toUTF8(mPath).get(),

This is totally wrong on Windows. All non-ASCII characters will be broken.
And on non-English Windows, it's too easy for user profile folder names to have non-ASCII characters. And you are going to use this to read sessionstore.js at startup. In short, Firefox will fail to start on localized versions of Windows with this patch.

> Unfortunately,
> http://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/io/prfile.c#740
> suggests that the symbol is not built – and ld agrees.

It's the reason why nsLocalFileWin has an own UTF-16-aware variant of PR_Open.
(In reply to Masatoshi Kimura [:emk] from comment #77)
> > Unfortunately,
> > http://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/io/prfile.c#740
> > suggests that the symbol is not built – and ld agrees.
> 
> It's the reason why nsLocalFileWin has an own UTF-16-aware variant of
> PR_Open.

Ok, good to know. I'll need to fix that.
(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #76)
> >+  JS_updateMallocCounter(cx, contents.nbytes);
> 
> Probably worth doing this after doing the actual JS_NewUint8ArrayWithBuffer
> call and checking that it succeeded.

Done. Speaking of which I wanted to double-check with you: should this be
  contents.nbytes
or
  contents.nbytes + (contents.data - contents.header)
?

> > class AbstractReadEvent: public AbstractDoEvent {
> >+  nsresult Read(ScopedArrayBufferContents& aBuffer)
> >+    PRFileDesc* file = PR_OpenFile(NS_ConvertUTF16toUTF8(mPath).get(),
> 
> Why OpenFile instead of Open?  If there's a reason, please document it.

Well, PR_Open is PR_OpenFile. I can't see any reason to prefer one over the other.

> >       just_read = PR_Read(file, dest_chars + total_read, bytes - total_read);
> 
> That last arg needs to be std::min(uint64_t(PR_INT32_MAX), bytes -
> total_read), I believe. 

And suddenly, I remember why I disliked C++ implicit arithmetics conversions all these years.

> >+      if (!valBytes.isNumber()) {
> 
> So if someone passes in "12" instead of 12 by accident (e.g. they read it
> from some API like an input.value or whatnot) we'll throw.  Is that by
> design?

For the moment, yes. We might wish to relax it at a later stage, but that's how non-native OS.File works.

> >       bytes = valBytes.toInt32();
> 
> You don't know that valBytes is an integer value.  All you know is it's a
> number.  It might not be an integer at all, or it might be 2^32 or bigger
> (which would not fit in an int32 value).
> 
> I really do think you should use a dictionary here so as to avoid the manual
> JSAPI usage...

So, if I follow correctly 
1. create dom/webidl/NativeOSFileInternals.webidl defining dictionary NativeOSFileReadOptions;
2. extend Bindings.conf (not sure what I need to add for a dictionary);
3. add this .webidl to the moz.build;
4. include something (not sure which file) from NativeOSFileInternals.cpp;
5. replace ~35 lines of not-very-nice C++ code by ~10 lines of nicer C++ code.

I can't find out what I need to add in 2. for a .webidl that is not involved in the DOM and doesn't define anything meaningful besides a dictionary.

If you believe that this is worth pursuing, I'll try and do it. In the meantime, I have applied your feedback, so I'll be attaching a new version with manual JSAPI usage.
Attached patch 1. Native code, v8 (obsolete) — Splinter Review
Attaching the folded version.
Comment on attachment 8379670 [details] [diff] [review]
1. Native code, v8

Review of attachment 8379670 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/NativeOSFileInternals.cpp
@@ +132,5 @@
> +///////// Cross-platform issues
> +
> +#if defined(XP_WIN)
> +static nsresult
> +::OpenFile(const nsAFlatString &path, int osflags, int mode,

This patch didn't compile because static functions are invisible from outside the translation unit.
You will have to remove |static| on the side of xpcom.
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #79)
> > I really do think you should use a dictionary here so as to avoid the manual
> > JSAPI usage...
> 
> So, if I follow correctly 
> 1. create dom/webidl/NativeOSFileInternals.webidl defining dictionary
> NativeOSFileReadOptions;
> 2. extend Bindings.conf (not sure what I need to add for a dictionary);
> 3. add this .webidl to the moz.build;
> 4. include something (not sure which file) from NativeOSFileInternals.cpp;
> 5. replace ~35 lines of not-very-nice C++ code by ~10 lines of nicer C++
> code.
> 
> I can't find out what I need to add in 2. for a .webidl that is not involved
> in the DOM and doesn't define anything meaningful besides a dictionary.

You want to add the new .webidl to dom/webidl/moz.build.  If everything works correctly, there should be a mozilla/dom/NativeOSFileReadDictionary.h or somesuch that you can #include and call the bits found therein.

I don't think you need anything else, except maybe noting that the interface shouldn't be registered?

> If you believe that this is worth pursuing, I'll try and do it.

Less JSAPI usage is a good thing.
Comment on attachment 8378216 [details] [diff] [review]
1a. Native code interdiff, v3

Canceling review until we get things sorted.
Attachment #8378216 - Flags: review?(nfroyd)
>  contents.nbytes + (contents.data - contents.header)

I wouldn't worry about the latter; it's just a few bytes.  The point of updating the couner is so our GC heuristics trigger GC as needed, and unless we plan to have millions (or at least hundreds of thousands) of calls to this API between GCs (so reading a huge number of very small files?) we should be fine.

> Well, PR_Open is PR_OpenFile.

The documentation says the handle "mode" differently, with PR_OpenFile seemingly doing more work.  But we don't care about the mode, right?

>2. extend Bindings.conf (not sure what I need to add for a dictionary);

Nothing.  You don't need to change Bindings.conf

>4. include something (not sure which file) from NativeOSFileInternals.cpp;

mozilla/dom/NativeOSFileInternalsBinding.h

>5. replace ~35 lines of not-very-nice C++ code by ~10 lines of nicer C++ code.

Yes.  But note that a dictionary would not keep the semantics you have now.  Eg it _would_ coerce "12" to the integer 12.  So if the semantics you have now are important, we just need to fix the JSAPI usage...
Forgot to qref
Attachment #8380611 - Attachment is obsolete: true
Comment on attachment 8380612 [details] [diff] [review]
1c. Native code interdiff, part 3, v2

Here's the latest interdiff.
Attachment #8380612 - Flags: review?(nfroyd)
Attachment #8380612 - Flags: review?(bzbarsky)
Comment on attachment 8379670 [details] [diff] [review]
1. Native code, v8

And here's the full patch, if you prefer.
Attachment #8375548 - Attachment is obsolete: true
Comment on attachment 8380612 [details] [diff] [review]
1c. Native code interdiff, part 3, v2

Review of attachment 8380612 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/NativeOSFileInternals.webidl
@@ +9,5 @@
> +{
> +  /**
> +   * If specified, convert the raw bytes to a string
> +   * with the specified encoding. Otherwise, return
> +   * the raw bytes.

"...as a typed array."?

@@ +14,5 @@
> +   */
> +  DOMString? encoding;
> +
> +  /**
> +   * If specify limit the number of bytes to read.

Maybe something like "If specified, the number of bytes to read.  Otherwise, read the entire file."

::: toolkit/components/osfile/NativeOSFileInternals.cpp
@@ +644,5 @@
> +                    GENERIC_READ,
> +                    FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
> +                    /*Security attributes*/nullptr,
> +                    OPEN_EXISTING,
> +                    FILE_ATTRIBUTE_NORMAL,

FILE_ATTRIBUTE_READONLY?  (I realize JS OS.File passes FILE_ATTRIBUTE_NORMAL here, but it should probably pass FILE_ATTRIBUTE_READONLY in this case.)

@@ +884,4 @@
>      }
>  
> +    if (dict.mEncoding.WasPassed()) {
> +      CopyUTF16toUTF8(dict.mEncoding.Value(), encoding);

This is a lot nicer than the goop that was there before!

@@ -882,5 @@
>      }
>  
> -    // Extract compression
> -    JS::Rooted<JS::Value> valCompression(cx);
> -    if (!JS_LookupProperty(cx, options, "compression", &valCompression)) {

We need to include compression in the dictionary and complain about it here.

@@ +888,4 @@
>      }
>  
> +    if (dict.mBytes.WasPassed()) {
> +      bytes = dict.mBytes.Value().Value();

Assuming I understand bz correctly, this now will accept |{ bytes: "12" }|, so perhaps we need to change the JS to be consistent?  We should add a test for this.
Attachment #8380612 - Flags: review?(nfroyd) → feedback+
(In reply to Nathan Froyd (:froydnj) from comment #90)
> ::: toolkit/components/osfile/NativeOSFileInternals.cpp
> @@ +644,5 @@
> > +                    GENERIC_READ,
> > +                    FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
> > +                    /*Security attributes*/nullptr,
> > +                    OPEN_EXISTING,
> > +                    FILE_ATTRIBUTE_NORMAL,
> 
> FILE_ATTRIBUTE_READONLY?  (I realize JS OS.File passes FILE_ATTRIBUTE_NORMAL
> here, but it should probably pass FILE_ATTRIBUTE_READONLY in this case.)

Did you mean GENERIC_READ? FILE_ATTRIBUTE_READONLY will set the read-only attribute (file mode in Unix term) to a newly created file. It will be ignored if OPEN_EXISTING is specified. I don't think FILE_ATTRIBUTE_READONLY is appropriate here.
Comment on attachment 8380612 [details] [diff] [review]
1c. Native code interdiff, part 3, v2

Review of attachment 8380612 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/NativeOSFileInternals.cpp
@@ +655,5 @@
> +
> +    file = PR_ImportFile((PROSFd)handle);
> +    if (!file) {
> +      Fail(NS_LITERAL_CSTRING("ImportFile"), nullptr, GetLastError());
> +      return NS_ERROR_FAILURE;

|handle| leaks here.
(In reply to Masatoshi Kimura [:emk] from comment #91)
> (In reply to Nathan Froyd (:froydnj) from comment #90)
> > ::: toolkit/components/osfile/NativeOSFileInternals.cpp
> > @@ +644,5 @@
> > > +                    GENERIC_READ,
> > > +                    FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
> > > +                    /*Security attributes*/nullptr,
> > > +                    OPEN_EXISTING,
> > > +                    FILE_ATTRIBUTE_NORMAL,
> > 
> > FILE_ATTRIBUTE_READONLY?  (I realize JS OS.File passes FILE_ATTRIBUTE_NORMAL
> > here, but it should probably pass FILE_ATTRIBUTE_READONLY in this case.)
> 
> Did you mean GENERIC_READ? FILE_ATTRIBUTE_READONLY will set the read-only
> attribute (file mode in Unix term) to a newly created file. It will be
> ignored if OPEN_EXISTING is specified. I don't think FILE_ATTRIBUTE_READONLY
> is appropriate here.

Indeed.  Thanks for pointing that out.

(In reply to Masatoshi Kimura [:emk] from comment #92)
> ::: toolkit/components/osfile/NativeOSFileInternals.cpp
> @@ +655,5 @@
> > +
> > +    file = PR_ImportFile((PROSFd)handle);
> > +    if (!file) {
> > +      Fail(NS_LITERAL_CSTRING("ImportFile"), nullptr, GetLastError());
> > +      return NS_ERROR_FAILURE;
> 
> |handle| leaks here.

Also a good point!
Attached patch 1. Native code, v9 (obsolete) — Splinter Review
Fixed leak. Thanks, emk.
Attachment #8379670 - Attachment is obsolete: true
Comment on attachment 8381365 [details] [diff] [review]
1. Native code, v9

Review of attachment 8381365 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/NativeOSFileInternals.cpp
@@ +655,5 @@
> +
> +    file = PR_ImportFile((PROSFd)handle);
> +    if (!file) {
> +      int error = GetLastError();
> +      (void)CloseHandle();

I don't believe this compiles.
Comment on attachment 8379669 [details] [diff] [review]
1b. Native code interdiff, part 2

>+static nsresult
>+::OpenFile(const nsAFlatString &path, int osflags, int mode,
>+         PRFileDesc **fd);

This needs to be non-static; probably extern?  And with a fixed indent.

>+  nsPromiseFlatString flatPath(path);
>+  PRFileDesc* fd;
>+  nsresult rv = OpenFile(flatPath, osflags, mode, &fd);

Just pass PromiseFlatString(path) to OpenFile() here instead of creating a temporary nsPromiseFlatString.

>+  const ArrayBufferContents contents(mContents.get());

I was thinking more like:

  const ArrayBufferContents& contents = mContents.get();

the point being to make it a reference, not a copy.

>+    if (!JS_LookupProperty(cx, options, "encoding", &valEncoding)) {

JS_GetProperty is really more idiomatic here, I think...

>+    } else if (!valEncoding.isUndefined()) {

No need for else after return.

>+      if (!valBytes.isInt32()) {

You really don't want to enforce isInt32() here, because callers have no control over whether the JS engine will represent their numbers as integers or doubles.  It will depend on exactly where those numbers came from (e.g. if they're the result of an arithmetic operation they might be doubles... or maybe integers, depending on which JIT level we're running in and what optimizations it decided it could do).

What you probably want is something more like checking isNumber() and then calling toNumber(), and then checking mozilla::DoubleIsInt32 or something.

This still has the weird deleted constructor...

r=me with those fixed.
Attachment #8379669 - Flags: review?(bzbarsky) → review+
Comment on attachment 8380612 [details] [diff] [review]
1c. Native code interdiff, part 3, v2

>+  DOMString? encoding;

So this allows passing null but effectively (in the C++ code) treats it as "", not "null", right?  Is that the intent?

>+  unsigned long? bytes;

And this allows passing null and then treats that as a cause to fatally assert in debug builds and probably do something bizarre in opt builds.  I suggest dropping the "?", so null gets converted to 0 here.

Also, is there a reason you're restricting to 32-bit integers in the dictionary?

>+#if defined(XP_WIN)

I can't speak to the correctness of the code in this branch....

>+    nsAutoCString path;
>+    CopyUTF16toUTF8(mPath, path);

  NS_ConvertUTF16toUTF8 path(mPath);

r=me modulo answers to the questions above.
Attachment #8380612 - Flags: review?(bzbarsky) → review+
And assuming someone who knows something about Windows looks at the XP_WIN bits there.
Comment on attachment 8380612 [details] [diff] [review]
1c. Native code interdiff, part 3, v2

>+    HANDLE handle -
>+      ::CreateFileW(mPath.get(),

s/-/=/?
(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #97)
> Comment on attachment 8380612 [details] [diff] [review]
> 1c. Native code interdiff, part 3, v2
> 
> >+  DOMString? encoding;
> 
> So this allows passing null but effectively (in the C++ code) treats it as
> "", not "null", right?  Is that the intent?

Indeed, that's the intent.

> >+  unsigned long? bytes;
> 
> And this allows passing null and then treats that as a cause to fatally
> assert in debug builds and probably do something bizarre in opt builds.  I
> suggest dropping the "?", so null gets converted to 0 here.

That's weird. What's the rationale to assert failure when passing |null| to an Option<> type?

> Also, is there a reason you're restricting to 32-bit integers in the
> dictionary?

Is there a better way to accept JavaScript integers?
> What's the rationale to assert failure when passing |null| to an Option<> type?

The assert happens when the callee does:

  bytes = dict.mBytes.Value().Value();

because Nullable<T>::Value has:

     MOZ_ASSERT(!mIsNull);

The point being that if you're declaring your member as "long?" instead of "long" that means you plan to handle null in some special way.  If you then blindly try to get the value as an integer, but the value is actually null, we'll assert because clearly you're not doing what you wanted to.

Again, if you just want null converted to an integer via ToInteger(), remove the '?'.  If you want to treat it specially, check dict.mBytes.Value().IsNull() and act accordingly.

> Is there a better way to accept JavaScript integers?

"unsigned long long" will allow 64-bit integers on the C++ side (which will correctly handle integer ranges up to 2^53 or so).
Attached patch 1. Native code, v10 (obsolete) — Splinter Review
Applied the feedback.
Requesting review from Paolo on the Windows-specific code.
Attachment #8378216 - Attachment is obsolete: true
Attachment #8379669 - Attachment is obsolete: true
Attachment #8380612 - Attachment is obsolete: true
Attachment #8381365 - Attachment is obsolete: true
Attachment #8388512 - Flags: review?(paolo.mozmail)
So if null is passed, you don't try to set "bytes", but if bytes is "0" you set it back to the default?

In that case, just remove the '?' from the IDL and the corresponding IsNull() and extra Value() on the C++ side.  Null will just convert to 0, and that will be treated as default, which seems like the behavior you're after.
Ah, sorry, that's a leftover of a previous version of the patch in which I had removed the '?'. I will return to treating 0 normally.
Attached patch 1. Native code, v11 (obsolete) — Splinter Review
Attachment #8388512 - Attachment is obsolete: true
Attachment #8388512 - Flags: review?(paolo.mozmail)
Attachment #8388517 - Flags: review?(paolo.mozmail)
Comment on attachment 8388517 [details] [diff] [review]
1. Native code, v11

Review of attachment 8388517 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/NativeOSFileInternals.cpp
@@ +642,5 @@
> +    // general semantics of OS.File.
> +    HANDLE handle =
> +      ::CreateFileW(mPath.get(),
> +                    GENERIC_READ,
> +                    FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,

I guess you need to set FILE_SHARE_READ only, to indicate that other processes cannot modify the file while we're reading it.

Note that PR_OpenFile would specify FILE_SHARE_READ | FILE_SHARE_WRITE, but we can do better here. The PR_OpenFile code existed since the year 1998 at least.

@@ +645,5 @@
> +                    GENERIC_READ,
> +                    FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
> +                    /*Security attributes*/nullptr,
> +                    OPEN_EXISTING,
> +                    FILE_ATTRIBUTE_NORMAL,

Since you're reading the entire file sequentially, you may want to add FILE_FLAG_SEQUENTIAL_SCAN here.

@@ +649,5 @@
> +                    FILE_ATTRIBUTE_NORMAL,
> +                    /*Template file*/ nullptr);
> +
> +    if (handle == INVALID_HANDLE_VALUE) {
> +      Fail(NS_LITERAL_CSTRING("open"), nullptr, GetLastError());

nit: ::GetLastError()

@@ +656,5 @@
> +
> +    file = PR_ImportFile((PROsfd)handle);
> +    if (!file) {
> +      int error = GetLastError();
> +      (void)CloseHandle(handle);

CloseHandle is not required here because PR_ImportFile takes care of closing the file in case of error.

@@ +657,5 @@
> +    file = PR_ImportFile((PROsfd)handle);
> +    if (!file) {
> +      int error = GetLastError();
> +      (void)CloseHandle(handle);
> +      Fail(NS_LITERAL_CSTRING("ImportFile"), nullptr, error);

You should also use PR_GetOSError() in that case. This is consistent with the other calls to NSPR and their error handling in this function.
Attachment #8388517 - Flags: review?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #107)
> Comment on attachment 8388517 [details] [diff] [review]
> 1. Native code, v11
> 
> Review of attachment 8388517 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/NativeOSFileInternals.cpp
> @@ +642,5 @@
> > +    // general semantics of OS.File.
> > +    HANDLE handle =
> > +      ::CreateFileW(mPath.get(),
> > +                    GENERIC_READ,
> > +                    FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
> 
> I guess you need to set FILE_SHARE_READ only, to indicate that other
> processes cannot modify the file while we're reading it.

That would not be consistent with OS.File JavaScript semantics:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_win_front.jsm#237

> @@ +656,5 @@
> > +
> > +    file = PR_ImportFile((PROsfd)handle);
> > +    if (!file) {
> > +      int error = GetLastError();
> > +      (void)CloseHandle(handle);
> 
> CloseHandle is not required here because PR_ImportFile takes care of closing
> the file in case of error.

This is non-obvious, so we should have a comment here indicating the error behavior of PR_ImportFile.
Comment on attachment 8389107 [details] [diff] [review]
1. Fixing typo in CrashManager

Review of attachment 8389107 [details] [diff] [review]:
-----------------------------------------------------------------

So I guess every Nightly user will have a corrupt date due to this. (We'll try to lz4 decompress uncompressed data, which I assume will throw.)

But that's OK.
Attachment #8389107 - Flags: review?(gps) → review+
Attachment #8389107 - Attachment description: 4. Fixing typo in CrashManager → 1. Fixing typo in CrashManager
Attached patch 2. Native code, 1.0 (obsolete) — Splinter Review
Attachment #8388517 - Attachment is obsolete: true
Attachment #8390422 - Flags: review+
Attached patch 4. Tests, 1.0Splinter Review
Attachment #8375550 - Attachment is obsolete: true
Attachment #8390424 - Flags: review+
Backed out for ASAN build bustage and Windows xpcshell failures that were visible in the Try push in comment 114.
https://hg.mozilla.org/integration/fx-team/rev/f547db6d8bc8

https://tbpl.mozilla.org/php/getParsedLog.php?id=36067100&tree=Fx-Team
Whiteboard: [Async:ready][fixed-in-fx-team] → [Async:ready]
The Windows xpcshell error is me being silly.
For the ASAN build, though, I have no clue. Perhaps someone more knowledgeable than me on the build system can figure out what's going on.
Flags: needinfo?(gps)
(In reply to comment #117)
> The Windows xpcshell error is me being silly.
> For the ASAN build, though, I have no clue. Perhaps someone more knowledgeable
> than me on the build system can figure out what's going on.

<https://tbpl.mozilla.org/php/getParsedLog.php?id=36067100&tree=Fx-Team&full=1> is most likely a build system bug, please file it.  In the mean time you should be able to land this with touching CLOBBER.
Filed as bug 983185.
Note that my next Try attempt builds nicely on Asan, without Clobber: https://tbpl.mozilla.org/?tree=Try&rev=bf100d2d6e73
Attachment #8390422 - Attachment is obsolete: true
Attachment #8391118 - Flags: review+
Attached patch 5. ClobberSplinter Review
Clobber, due to bug 983185.
Attachment #8391122 - Flags: review?(gps)
Comment on attachment 8391122 [details] [diff] [review]
5. Clobber

Ah, it seems that we do not request reviews on the CLOBBER file.
Attachment #8391122 - Flags: review?(gps) → review+
Flags: needinfo?(gps)
I landed a trivial cross compilation fixup:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7807eb511741
(In reply to Jacek Caban from comment #129)
> I landed a trivial cross compilation fixup:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7807eb511741

This "trivial cross compilation fixup" broke my standard Firefox build on Windows 8.1, see bug 990333. Please fix it or back it out.
Blocks: 990333
Sorry, it was the changeset before 7807eb511741 (also pushed by Jacek) that caused my bustage. 7807eb511741 did not cause my bustage.
No longer blocks: 990333
Type: defect → enhancement
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.