Fastpath OS.File.writeAtomic in C++.

RESOLVED FIXED in Firefox 58

Status

()

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: jchen, Assigned: milindl, Mentored)

Tracking

({perf})

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: [lang=js][good next bug])

Attachments

(3 attachments)

In my Android startup profile, it took osfile.jsm around 270ms to load, which is a lot higher than most other modules.
Right. OS.File is pretty big. One thing that certainly doesn't help is that it uses many .jsm files, which all require [main thread] I/O.
Would it be possible to lazy-load the individual submodules? A lot of the OS.File usage on startup that I looked at only use OS.Path.join or OS.Constants.Path.profileDir, so loading everything seems a bit excessive.
Theoretically, yes. However, making sure that the lazy load works as expected (i.e. without evaluating immediately stuff best left lazy) might be a little tricky.
Well, I can mentor an effort to make the loading of OS.File lazy. This might be more complicated than it looks, so I'm not marking this as a good first or second bug.

I believe that the right thing to do is to 
1/ create a new function `lazyModuleGetter` that behaves as `XPCOMUtils.lazyModuleGetter` but throws an error if it is called during the initialization of the module;
2/ replace most of the calls to `Cu.import` with calls to `lazyModuleGetter`;
3/ see what breaks and how we can fix it.
Mentor: dteller
Whiteboard: [lang=js][good next bug]
Hi, I'd like to take a stab at this. From what I understood, I would need to replace all the places where `Cu.import` is used. 
This would be replaced with something like `XPCOMUtils.lazyModuleGetter` that can call a function once the module to be lazily imported is actually imported, and we can check if that occurs when the initialization of original module is taking place.

I have a few questions, first, what does initilization of a module mean exactly? Is it done at the point inside the module where all definitions etc are done or is it something else?

Secondly, I was wondering if we could somehow use `aPostLambda` argument of `XPCOMUtils.lazyModuleGetter`, which seems to be called on actual import to actually throw the error and stuff?

Thanks!
Flags: needinfo?(dteller)
Thanks for picking this bug :)

It has been 3 years since I filed that bug and the good news is that there is now a team actually fixing performance issues, including startup. In particular, they have instrumented startup specifically to be able to determine whether a module has any right of being loaded during startup, which would pretty much replace 1/ and 2/ with "remove some files from the whitelist".

The bad news is that I haven't used that whitelist myself, so let's ask Florian, who has.
Flags: needinfo?(dteller) → needinfo?(florian)
Here is a profile of the first osfile.jsm import during startup on a slow netbook: https://perf-html.io/public/6801d5dff8f8e5e1505b145df68b2bf49ba191cb/calltree/?callTreeFilters=prefix-06xW6zSbxW6zScxW6zSdz.vxW6z_k&range=2.9675_4.1006~3.3592_3.5914&search=osfile&thread=0

This should give you information about where the time is spent, and what should be loaded lazily.

Blacklisting specific modules so that an attempt to load them during startup makes tests fail is as easy as adding a line in http://searchfox.org/mozilla-central/source/browser/base/content/test/performance/browser_startup.js
Flags: needinfo?(florian)
Thanks for the useful info!

I put the osfile in the blacklist, and the test failed. However, from what I understand, the test doesn't tell what code actually loaded osfile.
So, since yesterday, I have been trying to use perf profiling and trying to see what comes first. This is what I have come up with after that. it's not on a slow netbook, but relatively things should be same I guess.


profile #1:  https://perf-html.io/public/80274fa1b65e1892eff7c0a011dccaf8e64d863a/calltree/?callTreeFilters=prefix-061P7.9S8S9FCdSbALszO1N0xzoDx4yy9zO1RsCGdCzoALszO1N0xzpDx4yy9zO1N0xzqDx4yy9zO1Gz4yXiAZmxCiFNqDBvyGnPqzPbR0N0xCq&range=12.0585_13.2227~12.2762_12.3314~12.2844_12.3100&search=osfile&thread=0

GMPProvider comes first with an early Cu.import. This I believe is there in florian's profile also.
I then changed this to use XPCOMUtils.defineLazyModuleGetter. The problem is solved, it's not there in the next profile

profile #2: https://perf-html.io/public/008a03a8298ef2f0cd72d7af6eb7f8b308e223a3/calltree/?callTreeFilters=prefix-081Os_8TgThFAhTjEFqB_rN4xB7xLpCKuB_rT4.uT6EFqB_rN4xB8xLpCKuB_rN4xB9xLpCKuB_rN4xFrxFsxLpCKuB_rT4.uT6EFqB_rxzfEFqB_rN4xFuxLpCKuB_rN4xLpEEkEEsSoxJaxLmSqFH7B_rT4.uT6EFqB_rN4xUsxLpCKuB_rT4.uT6EFqB_rN4xV0xLpCKuB_rzI9xV2xEkEFqB_rN4xIuxLpCKuB_rT4.uT6EFqB_rzI9xV2xEkEFqB_rN4xIuxWe&range=0.6447_0.7330~0.6709_0.6905&search=osfile&thread=0

ExtensionPermissions.jsm is the next thing. It doesn't, however, use Cu.import. It has osfile inside defineLazyModuleGetter, and has a function called lazyInit containing a use of `OS` - but that seems to be called during startup.

After this there is a bunch of things which seem to take small amounts of time - 

profile#3: https://perf-html.io/public/050f6f8b62cc8f7bfc4aea499f47b479b3eae71d/calltree/?range=0.2689_1.0083~0.3671_0.6336~0.3762_0.5937&search=osfile&thread=0

this one has a couple of small things.
-> first is GMPProvider again, seems like it is finally needed in the `startup` which calls getter `gmpPath`
-> second is CrashMonitor.jsm. I imported XPCOMUtils and added lazy import for OS.
-> I can't figure out where the last two come from :(

Issues:
-> I can't tell by looking at the profile when startup has ended, I just assume that after the large initial delay on main thread startup is over. 
-> Test still fails
Flags: needinfo?(dteller)
Oh, right, CrashMonitor.jsm does need OS.File and it is needed during startup. So you'll have difficulties getting rid of it.

However, my hope is that you can delay loading of some *parts* of OS.File, by patching the uses of Cu.import inside OS.File. In other words, could you try and blacklist the files that compose OS.File one after the other, to check what breaks?
Assignee: nobody → i.milind.luthra
Flags: needinfo?(dteller) → needinfo?(i.milind.luthra)
What I did:
1. choose a file
2. wherever that file is imported, start using XPCOMUtils.defineLazyModuleGetter
3. add file to test, run test to see what happens
4. keeping the current file as it is (ie lazily loading), repeat for another file, and so on 

Filename: Test results after applying the above changes
ospath_unix: FAIL x1
osfile_shared_allthread: FAIL x2 [both ospath_unix, osfile_shared_allthread]
osfile_unix_allthread: FAIL x3
ospath: FAIL x4
osfile_native: FAIL x5
osfile_async_front: FAIL x5 [ospath_unix now starts passing]

Notes: I did not touch windows related files since I am on a unix system. I also didn't touch the parts with `require`, I think I could use something like lazyRequire maybe? But in almost all cases but one `require` seemed to be unused on my env, so it didn't block my testing efforts.

I also tried looking at the profiles again, seems like CrashMonitor calls `writeAtomic` inside it's `observe` method when it first observes some notification. This is the thing which shows closest to startup (after GMPProvider and some Extensions related stuff).

honestly I am a bit confused, any ideas on which direction to move?
Flags: needinfo?(i.milind.luthra) → needinfo?(dteller)
The `require` part is used on a background thread, so you did right to not touch it. Also, if it works on Unix, it should probably work on Windows, too, but you did right to start with Unix.

You also did right to look at CrashMonitor. Since it calls `writeAtomic` pretty early, and since `writeAtomic` requires most of OS.File, there are good chances that we cannot get rid of loading all of OS.File during early startup.

Unless we fast path writeAtomic, that is.

Let me give you some background. A few years ago, we realized that loading OS.File (and blocking on that load) early during startup was way too slow as there were many things loaded simultaneously, which made that load unacceptably slow. At the time, the only feature of OS.File that was needed during start was `OS.File.read`, so we wrote a C++ version of `OS.File.read` designed to be used only during startup.

Now, if `OS.File.writeAtomic` is the only part of the JS version of OS.File that is still needed during startup, we may want to write a C++ version of that code.

Actually, we'd like to eventually rewrite all of OS.File in C++ (or Rust), so this would probably be useful regardless.

Would you feel up to the task, milindl?

If you wish to look at the implementation of `OS.File.read`, it is here: http://searchfox.org/mozilla-central/source/toolkit/components/osfile/NativeOSFileInternals.cpp .
Flags: needinfo?(dteller) → needinfo?(i.milind.luthra)
> Unless we fast path writeAtomic, that is.
> 
> Let me give you some background. A few years ago, we realized that loading
> OS.File (and blocking on that load) early during startup was way too slow as
> there were many things loaded simultaneously, which made that load
> unacceptably slow. At the time, the only feature of OS.File that was needed
> during start was `OS.File.read`, so we wrote a C++ version of `OS.File.read`
> designed to be used only during startup.
> 
> Now, if `OS.File.writeAtomic` is the only part of the JS version of OS.File
> that is still needed during startup, we may want to write a C++ version of
> that code.
> 
> Actually, we'd like to eventually rewrite all of OS.File in C++ (or Rust),
> so this would probably be useful regardless.
> 
> Would you feel up to the task, milindl?
> 
> If you wish to look at the implementation of `OS.File.read`, it is here:
> http://searchfox.org/mozilla-central/source/toolkit/components/osfile/
> NativeOSFileInternals.cpp .

I am not very familiar with C++ in general, and not familiar at all with C++ in firefox. This seems like a nice activity to try out, I'll see the old code and see if I'll be able to do anything.

Will needinfo you if I come up with anything constructive, let me try this one over the weekend

Thanks!
Flags: needinfo?(i.milind.luthra)
Sounds good. Know that reaching me might be a bit slow during the next three weeks, though, but I will answer, eventually :)
Comment on attachment 8881356 [details]
Bug 1063635 Part 1 - Add native code for OS.File.writeAtomic.

So this is a look at the patch I am making. The basic idea is to take the methods outlined in Bug 961665, and apply it on a write operation. 


There are a lot of "DOUBT:" and "TODO:" comments in my code, at those points where I was stuck and would need help. It compiles on my PC, but I have not actually tested it.

Also, what do you think about rescoping this bug a bit; this seems we're doing Bug 986145 along with this.

Thanks!
Flags: needinfo?(dteller)
Attachment #8881356 - Flags: feedback?(dteller)
Attachment #8881356 - Flags: feedback?(dteller)
Flags: needinfo?(dteller)
Summary: Loading osfile.jsm is slow → Fastpath OS.File.writeAtomic in C++.
Comment on attachment 8881356 [details]
Bug 1063635 Part 1 - Add native code for OS.File.writeAtomic.

https://reviewboard.mozilla.org/r/152534/#review157684

::: toolkit/components/osfile/NativeOSFileInternals.cpp:389
(Diff revision 2)
>  
> +/**
> + * Return a result as an int32_t.
> + *
> + * In this implementation, attribute |result| is a pointer to int32_t.
> + * It is passed to JS without memory copy.

Nit: we don't care about memory copy for an `int32_t` :)

::: toolkit/components/osfile/NativeOSFileInternals.cpp:391
(Diff revision 2)
> + * Return a result as an int32_t.
> + *
> + * In this implementation, attribute |result| is a pointer to int32_t.
> + * It is passed to JS without memory copy.
> + */
> +class Int32_tResult final: public AbstractResult

Note: at this stage, I haven't checked whether this class is necessary. If it is, just get rid of the `_t` in the class name, though.

::: toolkit/components/osfile/NativeOSFileInternals.cpp:408
(Diff revision 2)
> +   * The method uses a UniquePtr to keep ownership of the pointer, and so this
> +   * should not be used elsewhere.
> +   */
> +  void Init(TimeStamp aDispatchDate,
> +            TimeDuration aExecutionDuration,
> +            int32_t* aContents) {

Make it `int32_t` instead of `int32_t*`. I believe that this entire class can live without using pointers to integers.

::: toolkit/components/osfile/NativeOSFileInternals.cpp:901
(Diff revision 2)
>    RefPtr<StringResult> mResult;
>  };
>  
> +/**
> + * An event implenting writing atomically to a file.
> + * DOUBT: should I write an abstract class for this? Seems like not now, since

Well, to follow the JS version, `aBuffer` can be either a `String` or an `ArrayBuffer`. I believe that you will need two classes to handle this.

::: toolkit/components/osfile/NativeOSFileInternals.cpp:961
(Diff revision 2)
> +  {
> +    MOZ_ASSERT(!NS_IsMainThread());
> +
> +    // Current implementation is WIP. It deals with Unix only.
> +    // It also does not deal with |backupTo| as of now.
> +    // Compression is not even included as one of the options.

I'm entirely ok with landing this without compression and keeping compression for a followup bug.

::: toolkit/components/osfile/NativeOSFileInternals.cpp:968
(Diff revision 2)
> +    if (!mBackupTo.IsVoid()) {
> +      Fail(NS_LITERAL_CSTRING("Unsupported backupTo"), nullptr);
> +    }
> +    ScopedPRFileDesc file;
> +    NS_ConvertUTF16toUTF8 path(mPath);
> +    bool fileExists;

Please initialize it to `false`.

::: toolkit/components/osfile/NativeOSFileInternals.cpp:988
(Diff revision 2)
> +    if (!mTmpPath.IsVoid()) {
> +      NS_ConvertUTF16toUTF8 tmpPath(mTmpPath);
> +      file = PR_OpenFile(tmpPath.get(),
> +                         PR_WRONLY | PR_CREATE_FILE,
> +                         // TODO: make sure this is right (the permissions).
> +                         PR_IRWXU);

On Unix, we open as `0600` (`PR_IRUSR | PR_IWUSR`). On Windows... well, permissions are not honored anyway.

::: toolkit/components/osfile/NativeOSFileInternals.cpp:1003
(Diff revision 2)
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    // This is where we cast |mBytes| into a 32 bit int. Till now, it's been
> +    // a uint64_t everywhere.
> +    int32_t bytesWrittenSuccess = PR_Write(file, mBuffer, (PRInt32) mBytes);

Let's use `uint32_t` instead of `PRInt32`.

::: toolkit/components/osfile/NativeOSFileInternals.cpp:1024
(Diff revision 2)
> +    }
> +
> +    // Apply any tmpPath renames.
> +    if (!mTmpPath.IsVoid()) {
> +      // TODO: I use NS_Convert... twice instead of storing it, a bit confused on how to fix this.
> +      NS_ConvertUTF16toUTF8 tmpPath(mTmpPath);

I don't understand the problem. Why don't you call

```c++
NS_ConvertUTF16toUTF8 tmpPath(mTmpPath);
```

at the function's toplevel?

::: toolkit/components/osfile/NativeOSFileInternals.cpp:1062
(Diff revision 2)
> +  const nsString mPath;
> +  const void* mBuffer;
> +  // DOUBT: I don't know what type I should make this, because
> +  // PR_Write limits me to a PRInt32 which is |int|, but logically #bytes
> +  // should be as large a type as possible. So, I am casting this later.
> +  const uint64_t mBytes;

You are right, in theory, this should be a `size_t` (which will be 64 bits on most recent machines). I don't think that there is much of a hurry to let JavaScript write files that are more than 2Gb, though :)

Let's make everything 32 bits for a first version and file a followup bug to (maybe) extend it to 64 bits afterwards. This might make the code a bit more complicated, as it will need to call directly Windows/Unix APIs, rather than NSPR. Not a big difficulty, but not needed for a first version, either.
Comment on attachment 8881356 [details]
Bug 1063635 Part 1 - Add native code for OS.File.writeAtomic.

https://reviewboard.mozilla.org/r/152534/#review157892

Quite promising :)

Uploading a few answers/comments already. I'll continue the review as soon as I find time.
Comment on attachment 8881356 [details]
Bug 1063635 Part 1 - Add native code for OS.File.writeAtomic.

https://reviewboard.mozilla.org/r/152534/#review157684

> Well, to follow the JS version, `aBuffer` can be either a `String` or an `ArrayBuffer`. I believe that you will need two classes to handle this.

Ok, I was basing my design off the osfile_async_front.jsm file's `writeAtomic`, as per which the buffer can be a typed array or a c buffer. I wasn't sure about the latter so I just went ahead, thanks for clarifying.

The initial design I thought of was having 2 subclasses under the newly named `AbstractWriteAtomicEvent`, and in the `BeforeWriteAtomic` method of those classes, do any processing needed to convert the string/typed array into a buffer suitable for PR_Write. 

The issue with that is the need of JSContext to convert a string/typed array into a C buffer. I thought initially of passing the context inside the constructor, but one MDN page for JSAPI says that a JSContext can be used by only one thread, and the constructor and `BeforeWriteAtomic` seem to be run on different threads.

The easy fix to this would be conversion in the public `WriteAtomic` method, but if we do that then the subclasses will not be doing anything extra at all.

So what I wanted help with was ideas for designing this better.

Thanks!
(needinfo? because I am stuck at above issue)
Flags: needinfo?(dteller)
Comment on attachment 8881356 [details]
Bug 1063635 Part 1 - Add native code for OS.File.writeAtomic.

https://reviewboard.mozilla.org/r/152534/#review158482

Does this answer all your question?

::: toolkit/components/osfile/NativeOSFileInternals.cpp:1156
(Diff revisions 1 - 2)
>      return NS_ERROR_INVALID_ARG;
>    }
>    {
>      JS::AutoCheckCannotGC nogc;
>      bool isShared;
>      buffer = JS_GetArrayBufferViewData(bufferObject.get(), &isShared, nogc);

I believe that the operation should fail if the buffer is shared.

::: toolkit/components/osfile/NativeOSFileInternals.cpp:1044
(Diff revision 2)
> +    *aBytesWritten = bytesWrittenSuccess;
> +    return NS_OK;
> +  }
> +
> +protected:
> +  nsresult BeforeWriteAtomic() {

Since there are no subclasses, you probably don't need that method.

::: toolkit/components/osfile/NativeOSFileInternals.cpp:1050
(Diff revision 2)
> +    // DOUBT: unsure what I need to do here, if anything.
> +    MOZ_ASSERT(!NS_IsMainThread());
> +    return NS_OK;
> +  }
> +
> +  nsresult AfterWriteAtomic(TimeStamp aDispatchDate, int32_t* aBytesWritten) {

Let's make it a `int32_t`, instead of `int32_t*`.

::: toolkit/components/osfile/NativeOSFileInternals.cpp:1156
(Diff revision 2)
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +  {
> +    JS::AutoCheckCannotGC nogc;
> +    bool isShared;
> +    buffer = JS_GetArrayBufferViewData(bufferObject.get(), &isShared, nogc);

As mentioned in another comment, please use `JS_StealArrayBufferContents(JSContext* cx, JS::HandleObject obj)` instead of `JS_GetArrayBufferViewData`. This will ensure that nobody mutates the contents of the buffer while we're writing it.

::: toolkit/components/osfile/NativeOSFileInternals.cpp:1168
(Diff revision 2)
> +  uint64_t bytes = JS_GetArrayBufferViewByteLength(bufferObject.get());
> +  bool flush;
> +  bool noOverwrite;
> +
> +  // This should initialize the dictionary with a set of useful defaults. Based on the WebIDL.
> +  // DOUBT: I've made some defaults in the webidl file, but I'm not sure where to

If it's not an object:

- use the same default values you mentioned in the webidl file;
- initialize the number of bytes with the length of the array buffer (as you already do).

::: toolkit/components/osfile/nsINativeOSFileInternals.idl:88
(Diff revision 2)
>              in nsINativeOSFileSuccessCallback onSuccess,
>              in nsINativeOSFileErrorCallback onError);
> +
> +  [implicit_jscontext]
> +  void writeAtomic(in AString path,
> +                   in jsval buffer, // DOUBT: Ok to assume the type is a javascript TypedArray?

Yes, as long as you throw an error if this is not the case.
Thanks for the feedback, I will try doing this the next iteration; and hopefully find a way to test it out as well.

Only question that's left to be answered is the one above the needinfo. Till then I'll continue with my current, one-class implementation (I will do the conversions to void* inside the public WriteAtomic using the JSContext over there).
Mmmh... I don't see any question to which I haven't answered. Yeah, do the conversion in the public WriteAtomic.
Flags: needinfo?(dteller)
Comment on attachment 8881356 [details]
Bug 1063635 Part 1 - Add native code for OS.File.writeAtomic.

https://reviewboard.mozilla.org/r/152534/#review157684

> Note: at this stage, I haven't checked whether this class is necessary. If it is, just get rid of the `_t` in the class name, though.

Yeah, I'm pretty sure that we want this class. Still, please rename it :)

> Ok, I was basing my design off the osfile_async_front.jsm file's `writeAtomic`, as per which the buffer can be a typed array or a c buffer. I wasn't sure about the latter so I just went ahead, thanks for clarifying.
> 
> The initial design I thought of was having 2 subclasses under the newly named `AbstractWriteAtomicEvent`, and in the `BeforeWriteAtomic` method of those classes, do any processing needed to convert the string/typed array into a buffer suitable for PR_Write. 
> 
> The issue with that is the need of JSContext to convert a string/typed array into a C buffer. I thought initially of passing the context inside the constructor, but one MDN page for JSAPI says that a JSContext can be used by only one thread, and the constructor and `BeforeWriteAtomic` seem to be run on different threads.
> 
> The easy fix to this would be conversion in the public `WriteAtomic` method, but if we do that then the subclasses will not be doing anything extra at all.
> 
> So what I wanted help with was ideas for designing this better.
> 
> Thanks!

You are right about `JSContext`. We should never touch it from another thread. So let's go with your proposal and perform conversion (and copy) in the public `WriteAtomic` method. This will be a bit slower than what I had hoped, but there is no choice, so let's do it. Also, let's handle only the `TypedArrayBuffer` in a first version, we'll deal with `JSString` in a followup bug, plus we can do this in JS without loss of performance. I'll let you open the followup bug :)

I believe that the best way to handle the `TypedArrayBuffer` is something along the lines of

```c++
UniquePtr<void> buffer(JS_StealArrayBufferContents(JSContext* cx, JS::HandleObject obj));
```

and move the `buffer` across threads as necessary. This will make sure that we never need to copy big buffers.

Is this ok with you?

> I'm entirely ok with landing this without compression and keeping compression for a followup bug.

I will let you open the followup bug, btw :)

> Let's use `uint32_t` instead of `PRInt32`.

Ah, my bad, `int32_t`, since for some reason, it seems to be signed.
Comment on attachment 8881356 [details]
Bug 1063635 Part 1 - Add native code for OS.File.writeAtomic.

https://reviewboard.mozilla.org/r/152534/#review157684

> I will let you open the followup bug, btw :)

I will do that + the other followups after finishing this
Comment on attachment 8881356 [details]
Bug 1063635 Part 1 - Add native code for OS.File.writeAtomic.

https://reviewboard.mozilla.org/r/152534/#review157684

> You are right about `JSContext`. We should never touch it from another thread. So let's go with your proposal and perform conversion (and copy) in the public `WriteAtomic` method. This will be a bit slower than what I had hoped, but there is no choice, so let's do it. Also, let's handle only the `TypedArrayBuffer` in a first version, we'll deal with `JSString` in a followup bug, plus we can do this in JS without loss of performance. I'll let you open the followup bug :)
> 
> I believe that the best way to handle the `TypedArrayBuffer` is something along the lines of
> 
> ```c++
> UniquePtr<void> buffer(JS_StealArrayBufferContents(JSContext* cx, JS::HandleObject obj));
> ```
> 
> and move the `buffer` across threads as necessary. This will make sure that we never need to copy big buffers.
> 
> Is this ok with you?

This is a good idea (I was using `void*`).

However, it seems like `UniquePtr<void>` is not permitted. I currently have used a `UniquePtr<char>`with a static cast to get around this, I think there could be a better way, I was searching around and came across this http://searchfox.org/mozilla-central/source/dom/canvas/WebGLTypes.h#177. Also, there seems to be an option for a custom deleter inside `UniquePtr`, will that help someway?

In short, from what I understand, we need some extra stuff to get around this issue. I chose `char` because it matches the type I get from JSString and also it is like Uint8Array...
Comment on attachment 8881356 [details]
Bug 1063635 Part 1 - Add native code for OS.File.writeAtomic.

https://reviewboard.mozilla.org/r/152534/#review159248

In this, you will notice that I've used `UniquePtr<char>` for reasons explained in a prior comment.

::: toolkit/components/osfile/NativeOSFileInternals.cpp:1000
(Diff revisions 2 - 3)
> +                      FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
> +                      /*Security attributes*/nullptr,
> +                      //CREATE_ALWAYS is used since since have already checked the noOverwrite
> +                      // condition, and thus can overwrite safely.
> +                      CREATE_ALWAYS,
> +                      /* TODO: fix flags below. */

There are a lot of flags on the Windows site, a lot of which I do not understand, so I would need help here.

::: toolkit/components/osfile/NativeOSFileInternals.cpp:1209
(Diff revisions 2 - 3)
> -  {
> -    JS::AutoCheckCannotGC nogc;
> -    bool isShared;
> -    buffer = JS_GetArrayBufferViewData(bufferObject.get(), &isShared, nogc);
> -    // TODO: I'm not sure whether isShared affects us, please advise.
> +
> +    bytes = JS_GetArrayBufferViewByteLength(bufferObject.get());
> +    buffer.reset(static_cast<char*>(
> +                   JS_StealArrayBufferContents(cx, bufferObject)));
> +
> +  } else if (aBuffer.isString()) {

I'd already done most of the work here regarding JSString when I got your comment about doing it in a followup. I've kept it here as of now, if it's totally incorrect/requires a lot more work, we can remove it.
Apologies for the delay, I was on vacation. I'll try and review today or tomorrow.
Comment on attachment 8881356 [details]
Bug 1063635 Part 1 - Add native code for OS.File.writeAtomic.

https://reviewboard.mozilla.org/r/152534/#review164614

This looks almost good to me. Have you already tested under Unix?

::: toolkit/components/osfile/NativeOSFileInternals.cpp:395
(Diff revision 3)
> + */
> +class Int32Result final: public AbstractResult
> +{
> +public:
> +  explicit Int32Result(TimeStamp aStartDate)
> +    : AbstractResult(aStartDate)

To aid with debugging, could you initialize `mContents` to 0?

::: toolkit/components/osfile/NativeOSFileInternals.cpp:993
(Diff revision 3)
> +    // general semantics of OS.File.
> +    HANDLE handle;
> +    // if we're dealing with a tmpFile, we need to write there.
> +    if (!mTmpPath.IsVoid()) {
> +      handle =
> +        ::CreateFileW(mTmpPath.get(),

Shouldn't this be `tmpPath.get()`?

::: toolkit/components/osfile/NativeOSFileInternals.cpp:997
(Diff revision 3)
> +      handle =
> +        ::CreateFileW(mTmpPath.get(),
> +                      GENERIC_WRITE,
> +                      FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
> +                      /*Security attributes*/nullptr,
> +                      //CREATE_ALWAYS is used since since have already checked the noOverwrite

Actually, the reason you can use `CREATE_ALWAYS` is that you're creating the temporary file.

::: toolkit/components/osfile/NativeOSFileInternals.cpp:1050
(Diff revision 3)
> +#endif // defined(XP_WIN)
> +
> +    int32_t bytesWrittenSuccess = PR_Write(file, (void* )(mBuffer.get()), mBytes);
> +
> +    if (bytesWrittenSuccess == -1) {
> +      Fail(NS_LITERAL_CSTRING("write"), nullptr, PR_GetOSError());

Shouldn't you `return` here?

::: toolkit/components/osfile/NativeOSFileInternals.cpp:1060
(Diff revision 3)
> +        Fail(NS_LITERAL_CSTRING("sync"), nullptr, PR_GetOSError());
> +        return NS_ERROR_FAILURE;
> +      }
> +    }
> +
> +    if(PR_Close(file) == PR_FAILURE) {

Nit: `if (` (with whitespace).

::: toolkit/components/osfile/NativeOSFileInternals.cpp:1082
(Diff revision 3)
> +      }
> +    }
> +
> +    // Apply any tmpPath renames.
> +    if (!mTmpPath.IsVoid()) {
> +      if (fileExists) {

If you have just renamed the file, you don't need to delete it, right?

::: toolkit/components/osfile/NativeOSFileInternals.cpp:1207
(Diff revision 3)
> +      return NS_ERROR_INVALID_ARG;
> +    }
> +
> +    bytes = JS_GetArrayBufferViewByteLength(bufferObject.get());
> +    buffer.reset(static_cast<char*>(
> +                   JS_StealArrayBufferContents(cx, bufferObject)));

Don't forget to document that you're stealing the ArrayBuffer contents.

::: toolkit/components/osfile/NativeOSFileInternals.cpp:1210
(Diff revision 3)
> +    bytes = JS_GetArrayBufferViewByteLength(bufferObject.get());
> +    buffer.reset(static_cast<char*>(
> +                   JS_StealArrayBufferContents(cx, bufferObject)));
> +
> +  } else if (aBuffer.isString()) {
> +    JSString* bufferString = JS::ToString(cx, aBuffer);

Please use `JS::RootedString` instead of `JSString*`, so that the gc knows it can't gc the string just yet.

::: toolkit/components/osfile/NativeOSFileInternals.cpp:1217
(Diff revision 3)
> +    if (!bufferString) {
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    bytes = JS_GetStringLength(bufferString);
> +    buffer.reset(JS_EncodeString(cx, bufferString));

if `buffer` is `nullptr`, you should return an error

::: toolkit/components/osfile/NativeOSFileInternals.cpp:1226
(Diff revision 3)
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +
> +  // Extract options.
> +  dom::NativeOSFileWriteAtomicOptions dict;
> +  bool flush;

Nit: Could you declare `flush`, `noOverwrite` closer to where they are first used? Also, they can probably be `const`.
Attachment #8881356 - Flags: review?(dteller)
Oh, right, you can't test it before you have written the JS front-end. Well, that should be the easy part.
Flags: needinfo?(i.milind.luthra)
Hi David, thanks for the review. 

I actually wrote a make-shift frontend and tested it a bit (it didn't work as expected - the file would be written but there would be an exception printed somewhere and the application would often crash). I thought it may be because of the mistakes you pointed out, or maybe my JS frontend had some error.

In any case, I'll apply the comments as soon as I can, I was a bit busy the past few days.

Thanks!
Flags: needinfo?(i.milind.luthra)
Attachment #8891624 - Flags: feedback?(dteller)
I rebased off a recent m-c, comments are applied, but I still cannot get it to work.

I am hoping it is a trivial error with the way I have written the frontend, but I think I might need some help to solve this issue. The private `WriteAtomic` is called, and it does indeed successfully write a file (the file exists with the data that was supposed to be written).

However, the whole application crashes, usually before `AfterWriteAtomic` is called.

On a debug build, I get the following messages:

> (result of my own debug printf) Ending private::WriteAtomic
> Assertion failure: I/O method is invalid, at $MOZ_CENTRAL/nsprpub/pr/src/io/priometh.c:81
> Redirecting call to abort() to mozalloc_abort
> Hit MOZ_CRASH() at $MOZ_CENTRAL/memory/mozalloc/mozalloc_abort.cpp:33
> Program $MOZ_CENTRAL/obj-debug-x86_64-pc-linux-gnu/dist/bin/firefox (pid = 2516) received signal 11.


To me it seems like I am making some error with one of the NSPR methods. When you are free, could you tell me how to tackle this?

Thanks!
Flags: needinfo?(dteller)
Comment on attachment 8881356 [details]
Bug 1063635 Part 1 - Add native code for OS.File.writeAtomic.

https://reviewboard.mozilla.org/r/152534/#review168138

::: toolkit/components/osfile/NativeOSFileInternals.cpp:397
(Diff revisions 3 - 4)
>  {
>  public:
>    explicit Int32Result(TimeStamp aStartDate)
>      : AbstractResult(aStartDate)
>    {
> +    mContents = 0;

Nit: Could you rather put `mContents(0)` in the initialization list (so, with `AbstractResult(aStartDate)`)?

::: toolkit/components/osfile/NativeOSFileInternals.cpp:1203
(Diff revisions 3 - 4)
>    UniquePtr<char> buffer;
>    int32_t bytes;
>  
>    // The incoming buffer can be an Object, a String, or something else.
>    if (aBuffer.isObject()) {
> +    // The contents of this bufferObject will be stolen by StealArrayBufferContents.

Sorry for the lack of clarity, I meant "could you document this in the documentation of the method?" - in case someone else decides to call this method.
Attachment #8881356 - Flags: review?(dteller)
Comment on attachment 8881356 [details]
Bug 1063635 Part 1 - Add native code for OS.File.writeAtomic.

https://reviewboard.mozilla.org/r/152534/#review168140

Looks good to me with minor nits. Next step is finding out why your code crashes. Do you want to submit your front-end patch here, so that I can review it?
Attachment #8891624 - Flags: feedback?(dteller) → review?(dteller)
Comment on attachment 8881356 [details]
Bug 1063635 Part 1 - Add native code for OS.File.writeAtomic.

https://reviewboard.mozilla.org/r/152534/#review168140

The frontend patch is already submitted, I had put it under feedback?. Now I am changing it to review? :)
Ah, my bad, sorry about that. I'll try and review it by Thursday.
Flags: needinfo?(dteller)
Comment on attachment 8891624 [details]
Bug 1063635 Part 2 - Call native writeAtomic code instead of JS backend when applicable.

https://reviewboard.mozilla.org/r/162238/#review170212

This looks good to me. Running tests to try and find out what's wrong.
Attachment #8891624 - Flags: review?(dteller)
It seems like try has similar failures as the one I have above, looking at a few. The logs seem to have 

> Assertion failure: I/O method is invalid, at /home/worker/workspace/build/src/nsprpub/pr/src/io/priometh.c:81
> Redirecting call to abort() to mozalloc_abort
> Hit MOZ_CRASH() at /home/worker/workspace/build/src/memory/mozalloc/mozalloc_abort.cpp:33 

It seems like there is some error using the NSPR stuff, because it crashes due to some assertion in in priometh.c. I didn't get much from seeing the given line in that file. Is there anything you recommend I do to try to solve this? I tried putting printf statements (which work fine till I hit the end of the private writeAtomic, and then the whole application crashes). I am a beginner at using gdb, so I don't have much idea about that.
Flags: needinfo?(dteller)
I haven't found the reason for the assertion failure yet. However, looking at https://treeherder.mozilla.org/logviewer.html#?job_id=120927806&repo=try&lineNumber=2536, I see lots of "invalid arguments" at JS-level. Could you investigate it?

For debugging, I do not have very good suggestions. I would start by adding breakpoints at the start and end of the private WriteAtomic, basically as you did, to check whether it crashes during one of the calls. If it doesn't, I'd look at the crash stack in gdb or lldb and try to find out exactly who is calling that line of code. If it's not in WriteAtomic, that's weird.
Flags: needinfo?(dteller) → needinfo?(i.milind.luthra)
So the current situation is I've tracked down the crashes that end up at priometh.c, and fixed them. (There were some small mistakes with initializing the result). So good news is, build doesn't crash immediately.

I'm still working on tracking down the "invalid arguments". They are stemming from any call to writeAtomic which uses ArrayBuffers (the strings work fine from what I can tell.) I am trying to fix this, will remove needinfo on me/request needinfo from you depending on my progress.

Thanks!
I think I have it down to a point where I can ask a focused query.

The problem seems to be that the `buffer` points to nothing. This stems from the fact that I am using `JS_StealArrayBufferContents` - the pointer I get from that is 0x0 - so that cascades down to a failure (the clause which checks `if (!buffer)` is hit and the method fails).

I'm fairly sure that the problem is not with the array buffer itself, for when I get the length of the buffer, it is a non zero number, and when I use JS_GetUint8ArrayData, I am able to extract the contents of the ArrayBuffer.

Do you have any ideas as to why this might be happening? 

Thanks!

PS: Also attaching soon patch which fixes the crashes
Flags: needinfo?(i.milind.luthra) → needinfo?(dteller)
Sorry for the delay, I'm back from a trip, I'll try and answer/review tomorrow.
Just to be sure I understand: your call to `JS_StealArrayBufferContents` returns `null`, right?

Looks like we both got confused between TypedArray and ArrayBuffer. Here, we pass as argument the TypedArray but we actually need an ArrayBuffer. So, you'll need to:

1. Change the front-end to check whether the argument is an ArrayBuffer view (e.g. a TypedArray), by using `ArrayBuffer.isView()`. If so, instead of passing the view, pass the buffer using its property `.buffer`;
2. Change the back-end to check whether the argument is an ArrayBuffer (using `JS_IsArrayBuffer`) instead of checking whether it is a TypedArray (using `JS_IsTypedArray`).

Hopefully, this should solve the issue :)

Also, can you add a `MOZ_ASSERT(NS_IsMainThread())` at the start of `WriteAtomic()`? This will serve mostly as documentation that this method is designed to be called on the main thread.
Flags: needinfo?(dteller) → needinfo?(i.milind.luthra)
Thanks! This does indeed solve the issue!

One potential issue here though (discovered while testing the new code):

```
let a = new Uint8Array(5);
...
let b = new Uint8Array(a.buffer, 1, 3);
```

Calling writeAtomic on `b`, we only expect that a part of the total ArrayBuffer would be written (the one which is 'in' the buffer view) but the entire buffer is written (since we steal the entire buffer). Note however, that the number of bytes written will actually be correct (because I set the options.bytes option as per the typed array length), but the starting point is not respected.

Possible solutions - 
* Copy the array buffer view into another array buffer (only the relevant bits). This would copy lots of stuff but may be easier to do I think.
* Pass the typed array itself into the method, and get length and offset from it, but use the WriteAtomic method to extract the buffer (using `JS_GetArrayBufferViewBuffer`?) and then proceed as usual.
Flags: needinfo?(i.milind.luthra) → needinfo?(dteller)
Ouch, good catch.

I believe that for a first version, we should just reject `b` from the front-end (this will need to be documented, of course). Now that OS.File is off-limits to add-ons, we may consider progressively making it a bit less powerful – especially since what you're doing is part of bug 1231711.
Flags: needinfo?(dteller) → needinfo?(nchen)
(In reply to David Teller [:Yoric] (hard to reach until July 17th - please use "needinfo") from comment #51)
> Ouch, good catch.
> 
> I believe that for a first version, we should just reject `b` from the
> front-end (this will need to be documented, of course). 

That works - we can also let it be handled by the non-C++ version (as it is being handled right now) - so the native method will throw an exception if it has a view with offset, and the method in osfile_async_front will make the non-C++ version handle it.

This will be quite easy to do. The status of my work right now is that I'm running unit tests, finding issues, and fixing them (and asking if I am unsure about something). 

(I think needinfo was set for nchen for something regarding bug 1231711?)
Sorry, needinfo error.
Flags: needinfo?(nchen)
The current patches fix issues with most tests.

The patch adds mechanism for avoiding byte offsets, and also adds the bit for updating |outExecutionDuration|, and with the last bit there are two problems.

1. the test_duration.js needs to be updated, as we are no longer guaranteed a |outSerializationDuration| from writeAtomic, since it's not gauranteed to |post| (similar to |read|). This bit was simple to solve, but it is not included in the patch (probably it will make sense to include that with a patch I may make for writeAtomic tests?)

2. adding |options.outExecutionDuration = ...| to any part of |File.writeAtomic| causes a TypeError at line where  |Internals.writeAtomic| is called. The exact error is "TypeError: attempting to access detached ArrayBuffer". I've not been able to figure it out for quite some time, do you have any suggestions?
Flags: needinfo?(dteller)
I'll try and review by Friday.
Comment on attachment 8881356 [details]
Bug 1063635 Part 1 - Add native code for OS.File.writeAtomic.

https://reviewboard.mozilla.org/r/152534/#review177936

Looks good to me. Let's see what the tests say.
Attachment #8881356 - Flags: review+
Comment on attachment 8891624 [details]
Bug 1063635 Part 2 - Call native writeAtomic code instead of JS backend when applicable.

https://reviewboard.mozilla.org/r/162238/#review177940

Looks good to me.
Attachment #8891624 - Flags: review+
Comment on attachment 8881356 [details]
Bug 1063635 Part 1 - Add native code for OS.File.writeAtomic.

https://reviewboard.mozilla.org/r/152534/#review177936

It should fail test_duration, please see this - https://bugzilla.mozilla.org/show_bug.cgi?id=1063635#c56

It should have been an easy fix, but see (2) in the above comment, a bit stuck there.
My bad, I missed your comment.

Is the error you encounter with the patch you posted?
Flags: needinfo?(dteller) → needinfo?(i.milind.luthra)
Yes, that's the error with the patch, I removed the others but couldn't fix this one.

I'm not sure why changing `options.outExecutionDuration` causes an error which talks about detaching an array buffer, the two things seem unrelated.
Flags: needinfo?(i.milind.luthra) → needinfo?(dteller)
That is certainly strange. Could you try to display the contents of `options` both before calling `writeAtomic` and in the callback? Something like


    dump(`Before: ${JSON.stringify(options)}\n`);


This will let us determine whether the code accidentally got arguments in the wrong order or if something somehow overwrites `options`.
Flags: needinfo?(dteller) → needinfo?(i.milind.luthra)
Comment on attachment 8891624 [details]
Bug 1063635 Part 2 - Call native writeAtomic code instead of JS backend when applicable.

https://reviewboard.mozilla.org/r/162238/#review178016

::: toolkit/components/osfile/modules/osfile_native.jsm:77
(Diff revision 3)
> + * Native implementation of OS.File.writeAtomic.
> + * This should not be called when |buffer| is a view with some non-zero byte offset.
> + * Does not handle option |compression|.
> + */
> +this.writeAtomic = function(path, buffer, options = {}) {
> +  // Sanity check on types of options

Ok, I just realized that the sanity check is not necessary. That's done automatically in the C++ when you call `dict.Init(cx, aOptions)`.

You need to keep the `ArrayBuffer.isView`, though.
I found the cause of the error, it was detached buffer in the test itself :(

What was happening was that:

-> whenever I changed `options.outExecutionDuration` anywhere, the rest of the code worked, and I got upto the point where the detached array buffer was passed throwing the TypeError. If the duration wasn't set, then it never reached that code, and threw an error earlier. 

-> I will now fix the other failures on try - seems like tests with characters like é, ö, ø etc also fail. That's what failures seem to be mostly coming from (except of course test_duration). I tracked this down to the fact that I'm not encoding the string anywhere. Putting a simple block of the form

```
  if (typeof buffer == "string") {
    // Normalize buffer to a C buffer by encoding it
    let encoding = options.encoding || "utf-8";
    buffer = new TextEncoder(encoding).encode(buffer);
  }
```
before calling Internals.writeAtomic fixes the issue, but since it converts the string to a buffer, it defeats the point of having the native writeAtomic deal with strings (which may be a good thing!)

I also think that I should find each consumer of this method and check if they might use the detached array buffer elsewhere. Would it be reasonable to make a patch fixing tests and consumers, along with making changes to above patches (applying comments, removing string part of writeAtomic etc)?

PS: Sorry, I was away, so didn't see your IRC ping till now
Flags: needinfo?(i.milind.luthra) → needinfo?(dteller)
Yes, you can start with TextEncoder on the main thread. We'll see if we can improve this in a followup.
Flags: needinfo?(dteller) → needinfo?(i.milind.luthra)
I made the changes, making the tests pass locally. I've not tested on Windows however.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=16aaa45743ae739c8331046106a5f7f24cea57de

(The linting error is fixed by the latest commit.)

What should be the next step?

Thanks!
Flags: needinfo?(i.milind.luthra) → needinfo?(dteller)
Comment on attachment 8902359 [details]
Bug 1063635 Part 3 - Fix tests for native writeAtomic.

https://reviewboard.mozilla.org/r/173926/#review179394
Attachment #8902359 - Flags: review?(dteller) → review+
I would say that the next steps are:

- take a minute to reflect on the fact that you did a good job :)
- land these patches;
- file a followup bug to resume what you were doing in comment 10;
- file a followup bug to move the TextEncoder out of the main thread (I don't think it is critical);
- get in touch with Florian to see if the patch already has an effect (I don't think it will immediately, but it's worth checking).
Thanks! :)

What do you think, will it be good to push to try for windows as well before you push to autoland?

I'll needinfo florian when the patch lands, to see what happened.
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 3 changesets with 6 changes to 6 files
remote: 
remote: WebIDL file dom/webidl/NativeOSFileInternals.webidl altered in changeset 92129238e117 without DOM peer review
remote: 
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: 
remote: Changes to WebIDL files in this repo require review from a DOM peer in the form of r=...
remote: This is to ensure that we behave responsibly with exposing new Web APIs. We appreciate your understanding..
remote: 
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.d_webidl hook failed
abort: push failed on remote
Attachment #8881356 - Flags: review?(bugs)
The latest patch fixes failing windows build. Status on try with win64, xpcshell tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ba732a70ff7ee8d7a519a32fb3fbd2eacd57afc

Since I need a review from a dom peer, I have requested review from smaug from https://wiki.mozilla.org/Modules/Core#Document_Object_Model

Thanks!
Comment on attachment 8881356 [details]
Bug 1063635 Part 1 - Add native code for OS.File.writeAtomic.

https://reviewboard.mozilla.org/r/152534/#review179950

::: dom/webidl/NativeOSFileInternals.webidl:45
(Diff revision 8)
> +
> +  /**
> +   * If specified and true, a failure will occur if the file
> +   * already exists in the given path.
> +   */
> +  boolean? noOverwrite = false;

Why is this nullable when the default value is after all false.
Please drop ? or explain what null value means here and why default is false.

::: dom/webidl/NativeOSFileInternals.webidl:51
(Diff revision 8)
> +
> +  /**
> +   * If specified and true, this will sync any buffered data
> +   * for the file to disk. This might be slower, but safer.
> +   */
> +  boolean? flush = false;

Same here. Why nullable?
Attachment #8881356 - Flags: review?(bugs) → review+
Thanks for review, latest patch adds changes from comment, so I think it may be ready to push.
Comment on attachment 8891624 [details]
Bug 1063635 Part 2 - Call native writeAtomic code instead of JS backend when applicable.

https://reviewboard.mozilla.org/r/162238/#review180464
Attachment #8891624 - Flags: review+
Pushed by dteller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/44ff3bc7bee5
Part 1 - Add native code for OS.File.writeAtomic. r=smaug,Yoric
https://hg.mozilla.org/integration/autoland/rev/0d9eb13ac53e
Part 2 - Call native writeAtomic code instead of JS backend when applicable. r=Yoric
https://hg.mozilla.org/integration/autoland/rev/61a23e8fbf54
Part 3 - Fix tests for native writeAtomic. r=Yoric
Sorry, this had to be backed out for failing xpcshell's toolkit/components/osfile/tests/xpcshell/test_read_write.js on Linux x64 debug (might be intermittent, there was one run so far):

https://hg.mozilla.org/integration/autoland/rev/36ab82ba404032464e1ebf8516bd705b7b8931b6
https://hg.mozilla.org/integration/autoland/rev/8b985e3ae457f294c50e75bc463c6e136cbbaab2
https://hg.mozilla.org/integration/autoland/rev/a747bd83f52e115336bb679abebd4128dac94f67

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=127729346&repo=autoland
[task 2017-09-01T13:35:33.929623Z] 13:35:33     INFO -  TEST-START | toolkit/components/osfile/tests/xpcshell/test_read_write.js
[task 2017-09-01T13:35:34.907379Z] 13:35:34  WARNING -  TEST-UNEXPECTED-FAIL | toolkit/components/osfile/tests/xpcshell/test_read_write.js | xpcshell return code: 0
[task 2017-09-01T13:35:34.915614Z] 13:35:34     INFO -  TEST-INFO took 945ms
[task 2017-09-01T13:35:34.915908Z] 13:35:34     INFO -  >>>>>>>
[task 2017-09-01T13:35:34.917725Z] 13:35:34     INFO -  PID 8362 | [8362] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2872
[task 2017-09-01T13:35:34.924244Z] 13:35:34     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2017-09-01T13:35:34.925744Z] 13:35:34     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2017-09-01T13:35:34.927121Z] 13:35:34     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2017-09-01T13:35:34.928255Z] 13:35:34     INFO -  running event loop
[task 2017-09-01T13:35:34.929910Z] 13:35:34     INFO -  PID 8362 | Couldn't convert chrome URL: chrome://branding/locale/brand.properties
[task 2017-09-01T13:35:34.931513Z] 13:35:34     INFO -  PID 8362 | [8362] WARNING: Could not get the program name for a cubeb stream.: 'NS_SUCCEEDED(rv)', file /builds/worker/workspace/build/src/dom/media/CubebUtils.cpp, line 370
[task 2017-09-01T13:35:34.932970Z] 13:35:34     INFO -  toolkit/components/osfile/tests/xpcshell/test_read_write.js | Starting init
[task 2017-09-01T13:35:34.934382Z] 13:35:34     INFO -  (xpcshell/head.js) | test init pending (2)
[task 2017-09-01T13:35:34.935781Z] 13:35:34     INFO -  (xpcshell/head.js) | test run_next_test 0 finished (2)
[task 2017-09-01T13:35:34.938403Z] 13:35:34     INFO -  (xpcshell/head.js) | test run_next_test 1 pending (2)
[task 2017-09-01T13:35:34.941129Z] 13:35:34     INFO -  (xpcshell/head.js) | test init finished (2)
[task 2017-09-01T13:35:34.944120Z] 13:35:34     INFO -  "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
[task 2017-09-01T13:35:34.945836Z] 13:35:34     INFO -  toolkit/components/osfile/tests/xpcshell/test_read_write.js | Starting
[task 2017-09-01T13:35:34.947409Z] 13:35:34     INFO -  (xpcshell/head.js) | test pending (2)
[task 2017-09-01T13:35:34.954274Z] 13:35:34     INFO -  "Executing test ordering with native operations"
[task 2017-09-01T13:35:34.957128Z] 13:35:34     INFO -  (xpcshell/head.js) | test run_next_test 1 finished (2)
[task 2017-09-01T13:35:34.959503Z] 13:35:34  WARNING -  TEST-UNEXPECTED-FAIL | toolkit/components/osfile/tests/xpcshell/test_read_write.js |  - "After writing 0.32788968041381046" == "After writing 0.3278896804138104"
[task 2017-09-01T13:35:34.962353Z] 13:35:34     INFO -  /builds/worker/workspace/build/tests/xpcshell/tests/toolkit/components/osfile/tests/xpcshell/test_read_write.js:ordering:25
[task 2017-09-01T13:35:34.964820Z] 13:35:34     INFO -  exiting test
[task 2017-09-01T13:35:34.966989Z] 13:35:34     INFO -  Unexpected exception 2147500036
[task 2017-09-01T13:35:34.969331Z] 13:35:34     INFO -  undefined
[task 2017-09-01T13:35:34.971624Z] 13:35:34     INFO -  exiting test

Precision error in the test (right number has a digit more)?

Please fix the issue and submit an updated patch. Thank you.
Flags: needinfo?(i.milind.luthra)
This also broke a devtools test: https://treeherder.mozilla.org/logviewer.html#?job_id=127745142&repo=autoland

14:55:47     INFO -  376 INFO Console message: [JavaScript Warning: "XUL box for toolbar element contained an inline #text child, forcing all its children to be wrapped in a block." {file: "chrome://devtools/content/scratchpad/scratchpad.xul" line: 0}]
14:55:47     INFO -  377 INFO Console message: [JavaScript Error: "NS_BASE_STREAM_CLOSED: Component returned failure code: 0x80470002 (NS_BASE_STREAM_CLOSED) [nsIInputStream.available]" {file: "chrome://devtools/content/scratchpad/scratchpad.js" line: 1174}]
14:55:47     INFO -  378 INFO Console message: [JavaScript Error: "NS_BASE_STREAM_CLOSED: Component returned failure code: 0x80470002 (NS_BASE_STREAM_CLOSED) [nsIInputStream.available]" {file: "chrome://devtools/content/scratchpad/scratchpad.js" line: 1174}]
14:55:47     INFO -  379 INFO Console message: [JavaScript Error: "NS_BASE_STREAM_CLOSED: Component returned failure code: 0x80470002 (NS_BASE_STREAM_CLOSED) [nsIInputStream.available]" {file: "chrome://devtools/content/scratchpad/scratchpad.js" line: 1174}]
14:55:47     INFO -  380 INFO TEST-PASS | devtools/client/scratchpad/test/browser_scratchpad_recent_files.js | the temporary file was saved successfully with Scratchpad -
14:55:47     INFO -  381 INFO TEST-PASS | devtools/client/scratchpad/test/browser_scratchpad_recent_files.js | the recent files menu was populated successfully. -
14:55:47     INFO -  382 INFO TEST-PASS | devtools/client/scratchpad/test/browser_scratchpad_recent_files.js | the temporary file was saved successfully with Scratchpad -
14:55:47     INFO -  383 INFO TEST-PASS | devtools/client/scratchpad/test/browser_scratchpad_recent_files.js | the recent files menu was populated successfully. -
14:55:47     INFO -  Buffered messages finished
14:55:47    ERROR -  384 INFO TEST-UNEXPECTED-FAIL | devtools/client/scratchpad/test/browser_scratchpad_recent_files.js | Test timed out -
Flags: needinfo?(dteller)
Flags: needinfo?(i.milind.luthra)
Comment on attachment 8881356 [details]
Bug 1063635 Part 1 - Add native code for OS.File.writeAtomic.

https://reviewboard.mozilla.org/r/152534/#review193662


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/osfile/NativeOSFileInternals.cpp:948
(Diff revision 11)
> +    }
> +    NS_ReleaseOnMainThreadSystemGroup("DoWriteAtomicEvent::mResult",
> +                                      mResult.forget());
> +  }
> +
> +  NS_IMETHOD Run() override {

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
Comment on attachment 8881356 [details]
Bug 1063635 Part 1 - Add native code for OS.File.writeAtomic.

https://reviewboard.mozilla.org/r/152534/#review193662

> Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]

Oh, right, this should actually be `NS_IMETHODIMP`, rather than `NS_IMETHOD`.
Code added with clangbot's comments applied, and not failing on windows (See https://treeherder.mozilla.org/#/jobs?repo=try&revision=26e85b6e9f27458cce69ba430289037e4b533d45). 

Yoric: Please push this if you think it is alright, and I can then continue the work in comment#74. Many thanks for the help in finding/fixing the issues!
Flags: needinfo?(dteller)
Comment on attachment 8881356 [details]
Bug 1063635 Part 1 - Add native code for OS.File.writeAtomic.

https://reviewboard.mozilla.org/r/152534/#review193748

::: toolkit/components/osfile/NativeOSFileInternals.cpp:1005
(Diff revisions 6 - 12)
>          }
>        }
> +
> +#if defined(XP_WIN)
> +      // On Windows, we cannot use PR_Rename because it doesn't handle
> +      // filenames which might be Unicode (and not just UTF8). Thus,

Let's reuse the same comment as below (more or less): "we cannot use `PR_Rename` because it doesn't handle UTF-16 encoding".

::: toolkit/components/osfile/NativeOSFileInternals.cpp:1101
(Diff revisions 6 - 12)
>          }
>        }
>  
> +#if defined(XP_WIN)
> +      // On Windows, we cannot use PR_Rename because it doesn't handle
> +      // filenames which might be Unicode (and not just UTF8). Thus,

As above.
Comment on attachment 8902359 [details]
Bug 1063635 Part 3 - Fix tests for native writeAtomic.

https://reviewboard.mozilla.org/r/173926/#review193752

::: toolkit/components/osfile/tests/xpcshell/xpcshell.ini:35
(Diff revision 7)
>  skip-if = os != "win"
>  [test_osfile_writeAtomic_backupTo_option.js]
>  [test_osfile_writeAtomic_zerobytes.js]
> +# Windows unicode filename renames may be problematic.
> +[test_osfile_writeAtomic_unicode_filename.js]
> +skip-if = os != "win"

Why do you want to skip it? We should test it on all platforms, no?
A few trivial things to change, then I'll push it.
Flags: needinfo?(dteller)
Applied comments to make it ready for push.
Flags: needinfo?(dteller)
The push replaces, in Windows, those NSPR file I/O functions which act on the file names with their Windows-specific versions to handle Unicode, and adds comments in the code to that effect.

The Windows 7 `dt6` failure seems ominous, but it's an intermittent failure, and retriggering it causes it to pass. 

Thanks!
Pushed by dteller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2a9fc9a1825b
Part 1 - Add native code for OS.File.writeAtomic. r=smaug,Yoric
https://hg.mozilla.org/integration/autoland/rev/71f8ad3208d9
Part 2 - Call native writeAtomic code instead of JS backend when applicable. r=Yoric
https://hg.mozilla.org/integration/autoland/rev/09157e44e113
Part 3 - Fix tests for native writeAtomic. r=Yoric
Flags: needinfo?(dteller)
See Also: → 1408729
You need to log in before you can comment on or make changes to this bug.