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

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Yoric, Assigned: sumit4iit)

Tracking

({dev-doc-needed, main-thread-io, perf})

unspecified
mozilla28
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Async][mentor=Yoric][lang=c++][mentored but not simple])

Attachments

(1 attachment, 10 obsolete attachments)

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.
Note: doing this should be quite simple. The difficult part is deciding whether we want flush() to be the default case or not.
Whiteboard: [Async][mentor=Yoric][lang=c++]

Comment 2

6 years ago
Doesn't that break the nsISafeFileOutputStream semantics?
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

6 years ago
(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?
(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?
Flags: needinfo?(vdjeric)
needinfo?-ing taras, too
Flags: needinfo?(taras.mozilla)

Comment 7

6 years ago
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.
Flags: needinfo?(taras.mozilla)
^ agree
Flags: needinfo?(vdjeric)
Adapting bug topic.
Summary: safe-file-output-stream should not always flush → Implement a variant of safe-file-output-stream that doesn't flush by default
(Assignee)

Comment 10

6 years ago
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
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?
Whiteboard: [Async][mentor=Yoric][lang=c++] → [Async][mentor=Yoric][lang=c++][mentored but not simple]
(Assignee)

Comment 12

6 years ago
Hi Yoric,

I have already compiled firefox using "./mach build". I have successfully run the test cases against the build as well.
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.
(Assignee)

Comment 14

6 years ago
(Assignee)

Comment 15

6 years ago
Hi David,
I have attached patch for part I of this bug. Please review this.
(Assignee)

Comment 16

6 years ago
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.
(Assignee)

Comment 17

6 years ago
Attachment #826522 - Attachment is obsolete: true
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 :)
(Assignee)

Comment 19

6 years ago
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 :)
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!
Assignee: nobody → sumit4iit
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?
Attachment #826526 - Flags: feedback+
(Assignee)

Comment 22

6 years ago
Posted patch nsAtomicFileStreams.patch (obsolete) — Splinter Review
Updated according to the feedback received.
Attachment #826526 - Attachment is obsolete: true
Attachment #827580 - Flags: feedback?
(Assignee)

Comment 23

6 years ago
Please let me know which tests should I run on local system to verify the build.
(Assignee)

Comment 24

6 years ago
Updated code according to the feedback received.
Attachment #827580 - Attachment is obsolete: true
Attachment #827580 - Flags: feedback?
Attachment #827918 - Flags: feedback?
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.
Attachment #827918 - Flags: feedback? → feedback+
(Assignee)

Comment 26

6 years ago
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.
(Assignee)

Comment 27

6 years ago
Posted patch nsAtomicFileStreams.patch (obsolete) — Splinter Review
Updated patch. [Successful compile: No warnings]
Attachment #827918 - Attachment is obsolete: true
Attachment #828186 - Flags: review?
Attachment #828186 - Flags: feedback?
(Assignee)

Comment 28

6 years ago
David, please review my previous patch.
Comment on attachment 828186 [details] [diff] [review]
nsAtomicFileStreams.patch

You should set a specific person for review, like this.
Attachment #828186 - Flags: review?(dteller)
Attachment #828186 - Flags: review?
Attachment #828186 - Flags: feedback?
Also, no need for both the feedback and review flags.
I'll try and review this patch by Tuesday.
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()|?
Attachment #828186 - Flags: review?(dteller) → feedback+
(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} \
}
Hey, Sumit, any news about this patch?
Flags: needinfo?(sumit4iit)
(Assignee)

Comment 35

6 years ago
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 :)
Flags: needinfo?(sumit4iit)
(Assignee)

Comment 36

6 years ago
Posted patch Part II [Register Class] (obsolete) — Splinter Review
Attachment #8337380 - Flags: feedback?
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.
Attachment #8337380 - Flags: feedback? → feedback?(dteller)
(Assignee)

Updated

6 years ago
Attachment #828186 - Attachment is obsolete: true
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
Attachment #8337380 - Flags: feedback?(dteller) → feedback+
(Assignee)

Comment 39

6 years ago
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)

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

Comment 41

6 years ago
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 .
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
(Assignee)

Comment 43

6 years ago
Posted patch nsAtomicFileStreams.patch (obsolete) — Splinter Review
Attachment #8337380 - Attachment is obsolete: true
Attachment #8337748 - Flags: feedback?(dteller)
(Assignee)

Comment 44

6 years ago
Posted patch nsAtomicFileStreams.patch (obsolete) — Splinter Review
Final working patch. Test passes.
Attachment #8337748 - Attachment is obsolete: true
Attachment #8337748 - Flags: feedback?(dteller)
Attachment #8337835 - Flags: review?(dteller)
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.
(Assignee)

Comment 46

6 years ago
Posted patch nsAtomicFileStreams.patch (obsolete) — Splinter Review
Attachment #8337835 - Attachment is obsolete: true
Attachment #8337835 - Flags: review?(dteller)
Attachment #8337864 - Flags: review?(dteller)
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.
Attachment #8337864 - Flags: review?(dteller) → review+
(Assignee)

Comment 48

6 years ago
Posted patch nsAtomicFileStreams.patch (obsolete) — Splinter Review
Final Patch.
Attachment #8337864 - Attachment is obsolete: true
(Assignee)

Comment 49

6 years ago
which options should I select for this bug? http://trychooser.pub.build.mozilla.org/
Flags: needinfo?(dteller)
Use "xpcshell".
Flags: needinfo?(dteller)
On all platforms, that is.
(Assignee)

Comment 52

6 years ago
Actually I have already pushed it on tryserver. Should I cancel it? 
https://tbpl.mozilla.org/?tree=Try&rev=d1913741ecde
Yes, please.
(Assignee)

Comment 54

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

Comment 55

6 years ago
xpcshell test has been updated and passes on local build.
[netwerk/test/unit/test_safeoutputstream.js]
(Assignee)

Comment 56

6 years ago
Fixed build issue on windows. |virtual nsresult DoOpen() MOZ_OVERRIDE;|
Attachment #8338549 - Attachment is obsolete: true
(Assignee)

Comment 57

6 years ago
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.]
Flags: needinfo?(dteller)
The errors seem completely unrelated to your patch.
I believe that you can go ahead and mark it checkin-needed.
Flags: needinfo?(dteller)
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f66fa95ae065
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.