I need to replace some remaining NS_ENSURE_SUCCESS(rv, rv) with:

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

because I want to add:

if (NS_WARN_IF(NS_FAILED(rv))) {
  QM_REPORT_INTERNAL_ERR();
  return rv;
}

The same needs to be done with NS_ENSURE_TRUE.

I'll file a separate bug for adding QM_REPORT_INTERNAL_ERR.

We will be also adding more failure recording in telemetry, especially for EnsureStorageIsInitialized. So it would look like this in the end:

if (NS_WARN_IF(NS_FAILED(rv))) {
  QM_REPORT_INTERNAL_ERR();
  REPORT_TELEMETRY_INIT_ERR(kQuotaInternalError, Some_Category);
  return rv;
}

In some cases this is even more verbose/complicated, for example:

if (NS_WARN_IF(NS_FAILED(rv))) {
  QM_REPORT_INTERNAL_ERR();
  REPORT_TELEMETRY_INIT_ERR(kQuotaInternalError, Some_Category);
  RECORD_IN_NIGHTLY(statusKeeper, rv);
#ifndef NIGHTLY_BUILD
  return rv;
#endif
}

The other option would be to introduce QM_ENSURE_SUCCESS which would do all necessary checking/logging/returning. However, we now use Result in many places, so we would need to use C++ templates or something for that.

Let me know what you think.

On the one hand, macros fiddling with the control flow impair readability. On the other hand, long methods that result from expanding them into these blocks of code also impairs readability. It will be hard to spot subtle differences. What's worse, duplicating these blocks of code makes evolution harder, since any change of this pattern will need to be done in lots of places either manually or using some complex regular expression matching.

Overall, I think these issues way more than the negative effect of a macro in this context. So introducing a QM_ENSURE_SUCCESSmacro seems like the better idea that fits into the current approach to do this.

I am not sure if this needs some special template handling? What for?

I mentioned it before, but I'd prefer an approach to this that does not involve a macro (at least not a control-flow-fiddling one, we will probably still need something to capture the code location information) and does neither come with so much code duplication. I think Result's andThen method and lambda expressions bring the necessary tools for that. This should be applicable to all our components.

But before we do that, I think that introducing a QM_ENSURE_SUCCESS macro here is a good intermediate step. Rolling out the macro everywhere would not be a good one, since it will make it much harder to evolve this.

Personally, I am not a fan of using macros like NS_ENSURE_TRUE since it hides return which reduces readability. (e.g. it might cause beginners to take more time to read through the code)

But, I agree with Simon's comment about the disadvantages of long methods that result from expanding them into these blocks of code. QM_ENSURE_SUCCESS macro here cab probably be a good compromise here.

if (NS_WARN_IF(NS_FAILED(rv))) {
  QM_REPORT_INTERNAL_ERR();
  REPORT_TELEMETRY_INIT_ERR(kQuotaInternalError, Some_Category);
  return rv;
}

In the case above, I lean to QM_ENSURE_SUCCESS because it can reduce the size of the code and help us to find the differences much easier.

But if we want to handle cases in QM_ENSURE_SUCCESS as well, like:

if (NS_WARN_IF(NS_FAILED(rv))) {
  QM_REPORT_INTERNAL_ERR();
  REPORT_TELEMETRY_INIT_ERR(kQuotaInternalError, Some_Category);
  RECORD_IN_NIGHTLY(statusKeeper, rv);
#ifndef NIGHTLY_BUILD
  return rv;
#endif
}

Maybe we should handle that differently (e.g. have another macro for that) so that others can differentiate if a macro would always return or could continue in some cases.

I guess the template that Jan mentioned is about reporting the internal error in the failures cases?

If I take IDB_ENSURE_SUCCESS as a starting point:

#define IDB_ENSURE_SUCCESS(res, ret)
  do {
    nsresult __rv = res; /* Don't evaluate |res| more than once */
    if (NS_FAILED(__rv)) {
      IDB_REPORT_INTERNAL_ERR();
      NS_ENSURE_SUCCESS_BODY(res, ret)
      return ret;
    }
  } while (0)

I can use that for cases like this:

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

However, I also need to cover:

if (NS_WARN_IF(storageFileOrErr.isErr())) {
  QM_REPORT_INTERNAL_ERR();
  return storageFileOrErr.unwrapErr();
}

Do we want separate macros for that or a generic function/macro which would cover both (based on the type, nsresult and Result) ?

(In reply to Jan Varga [:janv] from comment #3)

Do we want separate macros for that or a generic function/macro which would cover both (based on the type, nsresult and Result) ?

Ah, I though you meant something to handle the different Result template argument types, but I think that's not necessary. Yes, maybe it would be nicer to have one macro that can handle both nsresult and Result<V, E>.

This might use something like (just a sketch):

template<typename T> struct ResultTraits;

template<> struct ResultTraits<nsresult>
{
  static bool IsErr(nsresult aRv) { return NS_FAILED(aRv); }
  static nsresult PropagateErrAsNsresult(nsresult aRv) { return aRv; }
  static auto PropagateErrAsResult(nsresult aRv) { return Err(aRv); }
};

template<typename V> struct ResultTraits<Result<V, nsresult>>
{
  static bool IsErr(const Result<V,E>& aRv) { return aRv.isErr(); }
  static nsresult PropagateErrAsNsresult(Result<V,E>& aRv) { return aRv.unwrapErr(); }
  static auto PropagateErrAsResult(Result<V,E>& aRv) { return aRv.propagateErr(); }
};

However, I think there's no way to detect the return type of the current function, so we would probably need two different macros for that (that's why there is both PropagateErrAsNsresult and PropagateErrAsResult), or at least an argument that distinguishes the cases.

Ok, thanks for all the suggestions, I think I'm now able to continue.

I think QM_TRY/QM_TRY_VAL would be a good compromise, here's how it looks like:

nsresult EnsureDirectory(nsIFile* aDirectory, bool* aCreated) {
  AssertIsOnIOThread();

  nsresult rv = aDirectory->Create(nsIFile::DIRECTORY_TYPE, 0755);
  if (rv == NS_ERROR_FILE_ALREADY_EXISTS) {
    bool isDirectory;
    rv = aDirectory->IsDirectory(&isDirectory);
    NS_ENSURE_SUCCESS(rv, rv);
    NS_ENSURE_TRUE(isDirectory, NS_ERROR_UNEXPECTED);

    *aCreated = false;
  } else {
    NS_ENSURE_SUCCESS(rv, rv);

    *aCreated = true;
  }

  return NS_OK;
}

becomes:

// This method can be reused in other places
bool FileAlreadyExists(nsresult aValue) {
  return aValue == NS_ERROR_FILE_ALREADY_EXISTS;
}

nsresult EnsureDirectory(nsIFile* aDirectory, bool* aCreated) {
  AssertIsOnIOThread();

  bool exists;
  QM_TRY_VAR(exists, ToResult(aDirectory->Create(nsIFile::DIRECTORY_TYPE, 0755),
                              FileAlreadyExists));

  if (exists) {
    bool isDirectory;
    QM_TRY(aDirectory->IsDirectory(&isDirectory));
    QM_TRY(isDirectory, NS_ERROR_UNEXPECTED);
  }

  *aCreated = !exists;
  return NS_OK;
}

I'm about to submit patches for this.

Depends on D83927

Depends on D83928

https://treeherder.mozilla.org/#/jobs?repo=try&revision=71fae3ff2e6221961c43842d6f00bb6844f50440

Depends on D83931

Depends on D83953

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45039da84bc3d20a6776b5bb2566c4d365809049

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e96302a9b5570aa217e8e6ae9ec7d199e6f87d27

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1736250b51eedc165221aa5ff2ced85f4955f4d0

Depends on D83958

Depends on D84312

Depends on D84313

Depends on D84314

Depends on D84315

Depends on D84316

Depends on D84318

Depends on D84663

https://treeherder.mozilla.org/#/jobs?repo=try&revision=82e43f2923f2f327f5bb6ed8b9e1baa2c94d1aa1

Backed out 9 changesets (Bug 1651016) for causing hazard failures in TestQuotaCommon.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/ded0022e100405afa43e8bd857c142028cfe3517

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&tochange=2879ab33ffccb5e681f615877a09084febf3caf6&searchStr=Linux%2Cx64%2Cdebug%2Chazard-linux64-haz%2Fdebug%2CH&fromchange=5e566996d34e4d424750bf184a13e751a7e3b95f&selectedTaskRun=XQtZJ6cYR1O9EmWBKSmboA.0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310883381&repo=autoland&lineNumber=15554

Backed out for build bustages at gtest.h

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=c5a8759b3f85ecf2c395a25d84b0d98ecb9111eb&selectedTaskRun=K5rdQKVgQPW-9WXxVzo1VA.0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310921711&repo=autoland&lineNumber=11605

Backout: https://hg.mozilla.org/integration/autoland/rev/82565e07766096066b27278f58408da6e64006f1

Interesting that the first error (resulting into the first back out) couldn't be caught by mach try auto.

Latest try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d49d9dd1beaa54f9969a159d025451020ab8dcfa

https://hg.mozilla.org/mozilla-central/rev/2686e984d999

https://hg.mozilla.org/mozilla-central/rev/6e442bda9d02
https://hg.mozilla.org/mozilla-central/rev/3abd3c08e6f7
https://hg.mozilla.org/mozilla-central/rev/d53a199807f5
https://hg.mozilla.org/mozilla-central/rev/2480843e3791
https://hg.mozilla.org/mozilla-central/rev/7c1a187ff727
https://hg.mozilla.org/mozilla-central/rev/d6fb104d4910
https://hg.mozilla.org/mozilla-central/rev/498afabdaccc
https://hg.mozilla.org/mozilla-central/rev/3facc551c834
https://hg.mozilla.org/mozilla-central/rev/6175b036979e
https://hg.mozilla.org/mozilla-central/rev/65068ac1fb08
https://hg.mozilla.org/mozilla-central/rev/237cfeab38fd
https://hg.mozilla.org/mozilla-central/rev/99e8811aad2c
https://hg.mozilla.org/mozilla-central/rev/77cecccb140a