928321 - Implement a variant of safe-file-output-stream that doesn't flush by default

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

The "safe file output stream" ("@mozilla.org/network/safe-file-output-stream;1") always calls flush(). Even when this is done off the main thread, that's pretty bad for overall performance. We should make this optional.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Note: doing this should be quite simple. The difficult part is deciding whether we want flush() to be the default case or not.

Comment 2

[(no longer active)]

Doesn't that break the nsISafeFileOutputStream semantics?

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Note that there is no such thing as nsISafeFileOutputStream. As far as I know, there is no specification/documentation of nsSafeFileOutputStream outside of the code.

Now, I believe that the expectation of users of nsSafeFileOutputStream is that we write the file to disk before renaming it to its final name. This is still true if we remove flushing. This is sufficient to protect against most of the issues against which we want to be protected (in particular, thread being killed, process shutting down, process crash, soft reboot).

Flushing is only useful to protect against a subset of kernel crash/freeze or hardware issue happening within a few seconds after the write. This is only useful for valuable data. Now, if you look at all the uses of nsSafeFileOutputStream on m-c, you will see many uses that I believe don't need to be so well protected.

Comment 4

[(no longer active)]

(In reply to comment #3)
> Note that there is no such thing as nsISafeFileOutputStream. As far as I know,
> there is no specification/documentation of nsSafeFileOutputStream outside of
> the code.

Bah, sorry, I meant to type nsISafeOutputStream.

> Now, I believe that the expectation of users of nsSafeFileOutputStream is that
> we write the file to disk before renaming it to its final name. This is still
> true if we remove flushing. This is sufficient to protect against most of the
> issues against which we want to be protected (in particular, thread being
> killed, process shutting down, process crash, soft reboot).
> 
> Flushing is only useful to protect against a subset of kernel crash/freeze or
> hardware issue happening within a few seconds after the write. This is only
> useful for valuable data. Now, if you look at all the uses of
> nsSafeFileOutputStream on m-c, you will see many uses that I believe don't need
> to be so well protected.

Have you looked at the c-c and add-on callers as well?

Comment 5

[David Teller [:Yoric] - still alive but not very active]

(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #4)
> (In reply to comment #3)
> > Note that there is no such thing as nsISafeFileOutputStream. As far as I know,
> > there is no specification/documentation of nsSafeFileOutputStream outside of
> > the code.
> 
> Bah, sorry, I meant to type nsISafeOutputStream.

I actually didn't know that interface, thanks.
The change looks compatible, though.

> > Flushing is only useful to protect against a subset of kernel crash/freeze or
> > hardware issue happening within a few seconds after the write. This is only
> > useful for valuable data. Now, if you look at all the uses of
> > nsSafeFileOutputStream on m-c, you will see many uses that I believe don't need
> > to be so well protected.
> 
> Have you looked at the c-c and add-on callers as well?

Not yet, no, but this should definitely be done.

I believe that making flushing optional is the right thing to do. Now, the question is whether we want it opt-in or opt-out.
Vlad, do you have suggestions on that topic?

Comment 6

[David Teller [:Yoric] - still alive but not very active]

needinfo?-ing taras, too

Comment 7

[(dormant account)]

I disagree that we should remove flushing from safeoutput stream. I think it's best to deprecate it and manually convert to a new interface. Alternatively we can fix it up to do OMT flush/close.

Comment 8

[Vladan Djeric (:vladan)]

^ agree

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Adapting bug topic.

Comment 10

[Sumit Agrawal[:sumit4iit]]

Hi David,

I would like to take on this bug. Please assign this bug to me. I am new to mozilla and this my first bug, will you please tell me where do I start to fix this bug. 

Thanks

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Hi sumit4iit.
The first step is to build Firefox. You can find the instructions for building Firefox here: https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
Is this done already?

Comment 12

[Sumit Agrawal[:sumit4iit]]

Hi Yoric,

I have already compiled firefox using "./mach build". I have successfully run the test cases against the build as well.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Good :)
The safe file output stream is defined by class nsSafeFileOutputStream, here: http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsFileStreams.h and http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsFileStreams.cpp

The first part of this bug is to create a variant of nsSafeFileOutputStream that behaves exactly as nsSafeFileOutputStream except it doesn't call Flush() in nsSafeFileOutputStream::Finish(). Let's call that class nsAtomicFileOutputStream. Ideally, nsSafeFileOutputStream should be a subclass of nsAtomicFileOutputStream.

The second part of this bug is to register this class with contract "@mozilla.org/network/atomic-file-output-stream;1". To do this, take at NS_SAFELOCALFILEOUTPUTSTREAM_CONTRACTID and all its instances: http://dxr.mozilla.org/mozilla-central/search?q=NS_SAFELOCALFILEOUTPUTSTREAM_CONTRACTID .

If you're not sure about part 2, it's ok, we'll just start with part 1.

Comment 14

[Sumit Agrawal[:sumit4iit]]

Attachment: Added class nsAtomicFileStream

Comment 15

[Sumit Agrawal[:sumit4iit]]

Hi David,
I have attached patch for part I of this bug. Please review this.

Comment 16

[Sumit Agrawal[:sumit4iit]]

Please tell me more about the second part as well, I looked at nsNetCID.h and there are few hexa values

#define NS_SAFELOCALFILEOUTPUTSTREAM_CID             \
{ /* a181af0d-68b8-4308-94db-d4f859058215 */         \
    0xa181af0d,                                      \
    0x68b8,                                          \
    0x4308,                                          \
    {0x94, 0xdb, 0xd4, 0xf8, 0x59, 0x05, 0x82, 0x15} \
}

I am not really sure about this.

Comment 17

[Sumit Agrawal[:sumit4iit]]

Attachment: nsAtomicFileStreams.patch updated

Comment 18

[David Teller [:Yoric] - still alive but not very active]

I almost missed your comment. I'll try and review this today or tomorrow.
Next time you upload a patch, can you use the "feedback" or "review" fields? "feedback" if you're asking for guidance or feedback, "review" if you believe that the patch is complete and that I should be harsher :)

Comment 19

[Sumit Agrawal[:sumit4iit]]

Sure, will make sure of that next time. Will you please assign this bug to me? That way it will be easier for me to keep track of progress from "My Dashbord". And please give me your feedback on the previous patch, don't be harsh :)

Comment 20

[Benjamin Smedberg]

Assigned to you, and I also gave you editbugs so that you can make those kinds of changes yourself in the future. Happy hacking and thank you!

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 826526 [details] [diff] [review]
nsAtomicFileStreams.patch updated

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

Good start.

You're using too much copy and paste, however. All the code of DoOpen() and Close() and most of the code of Init() and Finish() should be shared between nsAtomicFileOutputStream and nsSafeFileOutputStream. You should implement these methods in nsAtomicFileOutputStream and take advantage of the fact that nsSafeFileOutputStream is a subclass to help reuse (all the methods are virtual by default, which should help).

At some point, I'd also like to ensure that the patch is kept small, to avoid murking the |hg blame|.

::: netwerk/base/src/nsFileStreams.cpp
@@ +910,5 @@
> +}
> +
> +NS_IMETHODIMP
> +nsAtomicFileOutputStream::Finish()
> +{

This method could be shared by ns{Atomic, Safe}FileOutputStream if there was a flag (let's call it |bool mShouldFlush|) set to |true| in |nsAtomicFileOutputStream| and |false| in |nsSafeFileOutputStream| that determines whether we need to call |Flush()|.

@@ +977,4 @@
>  // nsSafeFileOutputStream
>  
>  NS_IMPL_ISUPPORTS_INHERITED3(nsSafeFileOutputStream,
>                               nsFileOutputStream,

Since nsSafeFileOutputStream now descends directly from nsAtomicOutputStream rather than nsFileOutputStream, please update the parent class here.

::: netwerk/base/src/nsFileStreams.h
@@ +233,5 @@
> +    NS_DECL_NSISAFEOUTPUTSTREAM
> +
> +    nsAtomicFileOutputStream() :
> +        mTargetFileExists(true),
> +        mWriteResult(NS_OK) {}

Does that compile?
mTargetFileExists and mWriteResult are defined in nsSafeFileOutputStream,, aren't they?

Comment 22

[Sumit Agrawal[:sumit4iit]]

Attachment: nsAtomicFileStreams.patch

Updated according to the feedback received.

Comment 23

[Sumit Agrawal[:sumit4iit]]

Please let me know which tests should I run on local system to verify the build.

Comment 24

[Sumit Agrawal[:sumit4iit]]

Attachment: Fixing [Wreorder] compiler warnings.

Updated code according to the feedback received.

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 827918 [details] [diff] [review]
Fixing [Wreorder] compiler warnings.

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

In the future, could you number your patches?

::: netwerk/base/src/nsFileStreams.cpp
@@ +914,2 @@
>  {
> +    if(mShouldFlush)

Nit: this brace should go at the end of the previous line. Also, whitespace between |if| and |(|.

@@ +959,5 @@
>      mTempFile = nullptr;
>      return rv;
>  }
>  
> +

Nit: That newline is not necessary.

@@ +990,5 @@
> +nsSafeFileOutputStream::Init(nsIFile* file, int32_t ioFlags, int32_t perm,
> +                             int32_t behaviorFlags)
> +{
> +    return nsAtomicFileOutputStream::Init(file, ioFlags, perm, behaviorFlags);
> +}

You probably don't need Init().

@@ +996,5 @@
> +nsresult
> +nsSafeFileOutputStream::DoOpen()
> +{
> +    return nsAtomicFileOutputStream::DoOpen();
> +}

You probably don't need DoOpen().

@@ +1002,5 @@
> +NS_IMETHODIMP
> +nsSafeFileOutputStream::Close()
> +{
> +    return nsAtomicFileOutputStream::Close();
> +}

You probably don't need Close()

@@ +1009,5 @@
> +nsSafeFileOutputStream::Finish()
> +{
> +    mShouldFlush = true; // Enable Flush() in case of nsSafeFileOutputStream
> +    return nsAtomicFileOutputStream::Finish();
> +}

Actually, I realize that |mShouldFlush| is not the right way to do this.
Rather, |nsSafeFileOutputStream::Finish()| should call |Flush()| then |nsAtomicFileOutputStream::Finish()|.

@@ +1015,5 @@
> +NS_IMETHODIMP
> +nsSafeFileOutputStream::Write(const char *buf, uint32_t count, uint32_t *result)
> +{
> +    return nsAtomicFileOutputStream::Write(buf,count,result);
> +}

You probably don't need Write()

::: netwerk/base/src/nsFileStreams.h
@@ +241,5 @@
> +    {
> +        Close();
> +    }
> +
> +    virtual nsresult DoOpen();

For consistency, you should use |NS_IMETHOD| instead of |virtual nsresult|.

@@ +283,5 @@
>  
>  protected:
>      nsCOMPtr<nsIFile>         mTargetFile;
>      nsCOMPtr<nsIFile>         mTempFile;
>  

You should be able to remove mTargetFile and mTempFile now.

Comment 26

[Sumit Agrawal[:sumit4iit]]

I realize that there is no need of to keep Init(), DoOpen(), Close() and Write() in the child class i.e. nsSafeFileOutputStream. But the reason I kept them there was to keep the code easy to understand and that way it should be more easy for to change that piece of code for the next developer. And I think there should not be any performance issue with this practice, I expect that compiler will anyway remove that part while compiling, won't it? anyway this only for the discussion, I will remove these functions from the code.

Comment 27

[Sumit Agrawal[:sumit4iit]]

Attachment: nsAtomicFileStreams.patch

Updated patch. [Successful compile: No warnings]

Comment 28

[Sumit Agrawal[:sumit4iit]]

David, please review my previous patch.

Comment 29

[Andrew McCreight [:mccr8]]

Comment on attachment 828186 [details] [diff] [review]
nsAtomicFileStreams.patch

You should set a specific person for review, like this.

Comment 30

[Andrew McCreight [:mccr8]]

Also, no need for both the feedback and review flags.

Comment 31

[David Teller [:Yoric] - still alive but not very active]

I'll try and review this patch by Tuesday.

Comment 32

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 828186 [details] [diff] [review]
nsAtomicFileStreams.patch

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

Looks good.
Now, let's move to step 2 and writing tests.

::: netwerk/base/src/nsFileStreams.cpp
@@ +984,5 @@
> +
> +NS_IMETHODIMP
> +nsSafeFileOutputStream::Finish()
> +{
> +    Flush(); // Enable Flush() in case of nsSafeFileOutputStream

It looks like there was a minor style error in the original file. Could you make this
  (void)Flush();
to mark that we're not using the result of |Flush()|?

Comment 33

[David Teller [:Yoric] - still alive but not very active]

(In reply to Sumit Agrawal[:sumit4iit] from comment #16)
> Please tell me more about the second part as well, I looked at nsNetCID.h
> and there are few hexa values
> 
> #define NS_SAFELOCALFILEOUTPUTSTREAM_CID             \
> { /* a181af0d-68b8-4308-94db-d4f859058215 */         \
>     0xa181af0d,                                      \
>     0x68b8,                                          \
>     0x4308,                                          \
>     {0x94, 0xdb, 0xd4, 0xf8, 0x59, 0x05, 0x82, 0x15} \
> }
> 
> I am not really sure about this.

This is a unique identifier, which is basically a sequence of random numbers.
Here's a new one, I just generated it:

#define NS_ATOMICLOCALFILEOUTPUTSTREAM_CID           \
{ /* 6EAE857E-4BA9-11E3-9B39-B4036188709B */         \
    0x6EAE857E,                                      \
    0x4BA9,                                          \
    0x11E3,                                          \
    {0x9b, 0x39, 0xb4, 0x03, 0x61, 0x88, 0x70, 0x9b} \
}

Comment 34

[David Teller [:Yoric] - still alive but not very active]

Hey, Sumit, any news about this patch?

Comment 35

[Sumit Agrawal[:sumit4iit]]

Hi David,

Sorry for the delay. 

Actually I had exams going on. I guess I should be able to submit a new patch in couple of days. Will 

that be ok? Thanks :)

Comment 36

[Sumit Agrawal[:sumit4iit]]

Attachment: Part II [Register Class]

Comment 37

[Andrew McCreight [:mccr8]]

Comment on attachment 8337380 [details] [diff] [review]
Part II [Register Class]

You should set a specific person for feedback, to make it more likely that they notice.

Comment 38

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8337380 [details] [diff] [review]
Part II [Register Class]

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

Looks good to me. Next step is to write (and run!) a unit test.
You'll need to patch http://dxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_safeoutputstream.js. Write a function based on write() and call it in run_test().
To run that unit test, after having built Firefox:
 ./mach xpcshell-test netwerk/test/unit/test_safeoutputstream.js

Comment 39

[Sumit Agrawal[:sumit4iit]]

I ran test_safeoutputstream.js without making any changes. Surprisingly test failed. 

Here are the errors.
-------------------------------------------------------------------------------------------------------
 0:03.02 ###!!! ASSERTION: No CID found when attempting to map contract ID: 'Error', file /home/sumit4iit/mozilla-central/xpcom/components/nsComponentManager.cpp, line 516
 0:03.02 UNKNOWN [/home/sumit4iit/mozilla-central/obj-i686-pc-linux-gnu/dist/bin/libxul.so +0x0192881b]
 0:03.02 UNKNOWN [/home/sumit4iit/mozilla-central/obj-i686-pc-linux-gnu/dist/bin/libxul.so +0x0192a132]
 0:03.02 UNKNOWN [/home/sumit4iit/mozilla-central/obj-i686-pc-linux-gnu/dist/bin/libxul.so +0x0192b904]
 0:03.02 NS_InitXPCOM2+0x00000664 [/home/sumit4iit/mozilla-central/obj-i686-pc-linux-gnu/dist/bin/libxul.so +0x018efd9b]
 0:03.02 XRE_XPCShellMain+0x00000596 [/home/sumit4iit/mozilla-central/obj-i686-pc-linux-gnu/dist/bin/libxul.so +0x010d0461]
 0:03.02 UNKNOWN [/home/sumit4iit/mozilla-central/obj-i686-pc-linux-gnu/dist/bin/xpcshell +0x00002903]
 0:03.02 ###!!! ASSERTION: No CID found when attempting to map contract ID: 'Error', file /home/sumit4iit/mozilla-central/xpcom/components/nsComponentManager.cpp, line 516
 0:03.02 Hit MOZ_CRASH() at /home/sumit4iit/mozilla-central/memory/mozalloc/mozalloc_abort.cpp:30
 0:03.16 TEST-UNEXPECTED-FAIL | /home/sumit4iit/mozilla-central/obj-i686-pc-linux-gnu/_tests/xpcshell/netwerk/test/unit/test_safeoutputstream.js | test failed (with xpcshell return code: -11)

-------------------------------------------------------------------------------------------------------

Comment 40

[David Teller [:Yoric] - still alive but not very active]

Have you rebuilt Firefox before doing this?
Can you attach the complete output of your test run?

Comment 41

[Sumit Agrawal[:sumit4iit]]

Yes, I built Firefox before doing this. Further when I tested all of the unit test cases failed. 
Here is the output for this test_safeoutputstream.js http://pastebin.mozilla.org/3668347. 
And output for all other test cases is kept here http://pastebin.mozilla.org/3668565 .

Comment 42

[David Teller [:Yoric] - still alive but not very active]

Not 100% it's the cause of the issue, but in nsNetModule.cpp, there's a line
 { &kNS_SAFELOCALFILEOUTPUTSTREAM_CID, false, nullptr, nsSafeFileOutputStreamConstructor },
that you haven't adapted to ATOMICFILEOUTPUTSTREAM

Comment 43

[Sumit Agrawal[:sumit4iit]]

Attachment: nsAtomicFileStreams.patch

Comment 44

[Sumit Agrawal[:sumit4iit]]

Attachment: nsAtomicFileStreams.patch

Final working patch. Test passes.

Comment 45

[Peter Van der Beken [:peterv]]

Comment on attachment 8337835 [details] [diff] [review]
nsAtomicFileStreams.patch

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

::: netwerk/base/src/nsFileStreams.cpp
@@ +979,5 @@
> +NS_IMPL_ISUPPORTS_INHERITED3(nsSafeFileOutputStream,
> +                             nsAtomicFileOutputStream,
> +                             nsISafeOutputStream,
> +                             nsIOutputStream,
> +                             nsIFileOutputStream)

I pointed this out on irc, you don't actually need this

::: netwerk/base/src/nsFileStreams.h
@@ +261,5 @@
> +class nsSafeFileOutputStream : public nsAtomicFileOutputStream
> +{
> +public:
> +    NS_DECL_ISUPPORTS_INHERITED
> +    NS_DECL_NSISAFEOUTPUTSTREAM

or these (you just need a |NS_IMETHOD Finish();| here)

@@ +269,5 @@
> +
> +    virtual ~nsSafeFileOutputStream()
> +    {
> +        Close();
> +    }

or these.

Comment 46

[Sumit Agrawal[:sumit4iit]]

Attachment: nsAtomicFileStreams.patch

Comment 47

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8337864 [details] [diff] [review]
nsAtomicFileStreams.patch

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

Fine with me, with the trivial changes below. You don't need to ask for another review.
Have you run this through the Try Server yet?

::: netwerk/base/public/nsNetUtil.h
@@ +1013,1 @@
>  // returns a file output stream which can be QI'ed to nsISafeOutputStream.

Nit: Could you copy that comment, too?

::: netwerk/base/src/nsFileStreams.cpp
@@ +978,5 @@
> +
> +NS_IMETHODIMP
> +nsSafeFileOutputStream::Finish()
> +{
> +    (void) Flush(); // Enable Flush() in case of nsSafeFileOutputStream

Nit: That comment is not useful.

::: netwerk/base/src/nsFileStreams.h
@@ +223,5 @@
>      Create(nsISupports *aOuter, REFNSIID aIID, void **aResult);
>  };
>  
>  ////////////////////////////////////////////////////////////////////////////////
>  

Let's take the opportunity to add a comment along the lines of:

/**
 * A safe file output stream that overwrites the destination file only
 * once writing is complete. This protects against incomplete writes
 * due to the process or the thread being interrupted or crashed.
 */

@@ +256,5 @@
> +
> +};
> +
> +////////////////////////////////////////////////////////////////////////////////
> +

Let's also add a comment along the lines of:

/**
 * A safe file output stream that overwrites the destination file only
 * once writing + flushing is complete. This protects against more
 * classes of software/hardware errors than nsAtomicFileOutputStream,
 * at the expense of being more costly to the disk, OS and battery.
 */

::: netwerk/build/nsNetCID.h
@@ +395,5 @@
>      0x11d3,                                          \
>      {0x8c, 0xda, 0x00, 0x60, 0xb0, 0xfc, 0x14, 0xa3} \
>  }
>  
>  // component implementing nsISafeOutputStream

Nit: "components" (plural)

::: netwerk/test/unit/test_safeoutputstream.js
@@ +2,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +function test_atomicFileOutputStream(file, str) {

Nit: Could you rename this function to something along the lines of |write_atomic|?

@@ +56,5 @@
>      checkFile(file, "First write");
>  
>      write(file, "Second write");
>      checkFile(file, "Second write");
> +    

Nit: Please remove this needless indentation.

@@ +59,5 @@
>      checkFile(file, "Second write");
> +    
> +    test_atomicFileOutputStream(file, "First write: Atomic");
> +    checkFile(file, "First write: Atomic");
> +    

Nit: Here, too.

Comment 48

[Sumit Agrawal[:sumit4iit]]

Attachment: nsAtomicFileStreams.patch

Final Patch.

Comment 49

[Sumit Agrawal[:sumit4iit]]

which options should I select for this bug? http://trychooser.pub.build.mozilla.org/

Comment 50

[David Teller [:Yoric] - still alive but not very active]

Use "xpcshell".

Comment 51

[David Teller [:Yoric] - still alive but not very active]

On all platforms, that is.

Comment 52

[Sumit Agrawal[:sumit4iit]]

Actually I have already pushed it on tryserver. Should I cancel it? 
https://tbpl.mozilla.org/?tree=Try&rev=d1913741ecde

Comment 53

[David Teller [:Yoric] - still alive but not very active]

Yes, please.

Comment 54

[Sumit Agrawal[:sumit4iit]]

I was unable to cancel the build. Filed a new bug for LDAP password. 
Try Server : https://tbpl.mozilla.org/?tree=Try&rev=d1913741ecde

Comment 55

[Sumit Agrawal[:sumit4iit]]

xpcshell test has been updated and passes on local build.
[netwerk/test/unit/test_safeoutputstream.js]

Comment 56

[Sumit Agrawal[:sumit4iit]]

Attachment: nsAtomicFileStreams.patch

Fixed build issue on windows. |virtual nsresult DoOpen() MOZ_OVERRIDE;|

Comment 57

[Sumit Agrawal[:sumit4iit]]

Try : https://tbpl.mozilla.org/?tree=Try&rev=c930b5e40cc9

1. Static routing hazard analysis : Fails. [observed in both runs.]
2. B2G Desktop Linux x64 Opt (Mochitest) : Fails. [observed in both runs.]

Comment 58

[David Teller [:Yoric] - still alive but not very active]

The errors seem completely unrelated to your patch.
I believe that you can go ahead and mark it checkin-needed.

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f66fa95ae065

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f66fa95ae065

Comment 61

[guigs [:guigs] PST (please needinfo me!)]

Hey would this signature have to do with this patch? 
https://crash-stats.mozilla.com/report/index/b262d7fa-a09b-40bb-abaa-cfba02140618
For tracking: 
https://support.mozilla.org/en-US/questions/1006824