Closed Bug 1651016 Opened 1 year ago Closed 1 year ago

Convert NS_ENSURE_SUCCESS and NS_ENSURE_TRUE in Quota manager and quota clients

Categories

(Core :: Storage: Quota Manager, task)

task

Tracking

()

RESOLVED FIXED
mozilla80
Tracking Status
firefox80 --- fixed

People

(Reporter: janv, Assigned: janv)

References

(Blocks 1 open bug)

Details

Attachments

(14 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

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.

Keywords: leave-open
Attachment #9164302 - Attachment description: Bug 1651016 - Introduce QM_TRY and QM_TRY_VAL macros; r=#dom-workers-and-storage-reviewers → Bug 1651016 - Introduce QM_TRY and QM_TRY_VAR macros; r=#dom-workers-and-storage-reviewers
Attachment #9164303 - Attachment description: Bug 1651016 - Allow MOZ_TRY/QM_TRY to handle bool values; r=#dom-workers-and-storage-reviewers → Bug 1651016 - Allow MOZ_TRY/QM_TRY to handle bool values by wrapping them with OkIf; r=#dom-workers-and-storage-reviewers
Attachment #9164972 - Attachment description: Bug 1651016 - Allow definition of module specific TRY/TRY_VAL macros using generic QM_TRY_META macro; r=#dom-workers-and-storage-reviewers → Bug 1651016 - Allow definition of module specific TRY/TRY_VAR macros using generic QM_TRY_META/QM_TRY_VAR_META macros; r=#dom-workers-and-storage-reviewers
Attachment #9164973 - Attachment description: Bug 1651016 - Introduce IDB_TRY and IDB_TRY_VAL macros; r=#dom-workers-and-storage-reviewers → Bug 1651016 - Introduce IDB_TRY and IDB_TRY_VAR macros; r=#dom-workers-and-storage-reviewers
Attachment #9164975 - Attachment description: Bug 1651016 - Introduce CACHE_TRY and CACHE_TRY_VAL macros; r=#dom-workers-and-storage-reviewers → Bug 1651016 - Introduce CACHE_TRY and CACHE_TRY_VAR macros; r=#dom-workers-and-storage-reviewers
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8d2cc761e5ba
Introduce QM_TRY and QM_TRY_VAR macros; r=dom-workers-and-storage-reviewers,ttung,sg
https://hg.mozilla.org/integration/autoland/rev/9142d4249c29
Allow MOZ_TRY/QM_TRY to handle bool values by wrapping them with OkIf; r=dom-workers-and-storage-reviewers,ttung,sg,asuth
https://hg.mozilla.org/integration/autoland/rev/5d5b249ca664
Allow MOZ_TRY/QM_TRY to handle nsresult values and optionally enforce success for some special values; r=ttung
https://hg.mozilla.org/integration/autoland/rev/7f6eb1658a16
Convert NS_ENSURE_SUCCESS and NS_ENSURE_TRUE in EnsureDirectory; r=dom-workers-and-storage-reviewers,ttung,sg
https://hg.mozilla.org/integration/autoland/rev/8f2aa854a083
Convert remaining NS_ENSURE_TRUE to QM_TRY; r=ttung
https://hg.mozilla.org/integration/autoland/rev/2f4529e62cd3
Convert remaining NS_ENSURE_SUCCESS to QM_TRY in ActorsParent.cpp; r=dom-workers-and-storage-reviewers,ttung,sg
https://hg.mozilla.org/integration/autoland/rev/aed04f32eb91
Convert remaining NS_ENSURE_SUCCESS to QM_TRY in FileStreams.cpp; r=dom-workers-and-storage-reviewers,ttung,sg
https://hg.mozilla.org/integration/autoland/rev/002ddedb37dc
Allow definition of module specific TRY/TRY_VAR macros using generic QM_TRY_META/QM_TRY_VAR_META macros; r=dom-workers-and-storage-reviewers,ttung,sg
https://hg.mozilla.org/integration/autoland/rev/5e566996d34e
Introduce IDB_TRY and IDB_TRY_VAR macros; r=ttung
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c5a8759b3f85
Introduce QM_TRY and QM_TRY_VAR macros; r=dom-workers-and-storage-reviewers,ttung,sg

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

Flags: needinfo?(jvarga)
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2686e984d999
Introduce QM_TRY and QM_TRY_VAR macros; r=dom-workers-and-storage-reviewers,ttung,sg
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6e442bda9d02
Allow MOZ_TRY/QM_TRY to handle bool values by wrapping them with OkIf; r=dom-workers-and-storage-reviewers,ttung,sg,asuth
https://hg.mozilla.org/integration/autoland/rev/3abd3c08e6f7
Allow MOZ_TRY/QM_TRY to handle nsresult values and optionally enforce success for some special values; r=ttung
https://hg.mozilla.org/integration/autoland/rev/d53a199807f5
Convert NS_ENSURE_SUCCESS and NS_ENSURE_TRUE in EnsureDirectory; r=dom-workers-and-storage-reviewers,ttung,sg
https://hg.mozilla.org/integration/autoland/rev/2480843e3791
Convert remaining NS_ENSURE_TRUE to QM_TRY; r=ttung
https://hg.mozilla.org/integration/autoland/rev/7c1a187ff727
Convert remaining NS_ENSURE_SUCCESS to QM_TRY in ActorsParent.cpp; r=dom-workers-and-storage-reviewers,ttung,sg
https://hg.mozilla.org/integration/autoland/rev/d6fb104d4910
Convert remaining NS_ENSURE_SUCCESS to QM_TRY in FileStreams.cpp; r=dom-workers-and-storage-reviewers,ttung,sg
https://hg.mozilla.org/integration/autoland/rev/498afabdaccc
Allow definition of module specific TRY/TRY_VAR macros using generic QM_TRY_META/QM_TRY_VAR_META macros; r=dom-workers-and-storage-reviewers,ttung,sg
https://hg.mozilla.org/integration/autoland/rev/3facc551c834
Introduce IDB_TRY and IDB_TRY_VAR macros; r=ttung
Attachment #9164974 - Attachment description: Bug 1651016 - Convert remaining NS_ENSURE_SUCCESS to QM_TRY in dom/indexedDB; r=#dom-workers-and-storage-reviewers → Bug 1651016 - Convert remaining NS_ENSURE_SUCCESS to IDB_TRY in dom/indexedDB; r=#dom-workers-and-storage-reviewers
Attachment #9164977 - Attachment description: Bug 1651016 - Convert remaining NS_ENSURE_SUCCESS to QM_TRY in dom/cache; r=#dom-workers-and-storage-reviewers → Bug 1651016 - Convert remaining NS_ENSURE_SUCCESS to CACHE_TRY in dom/cache; r=#dom-workers-and-storage-reviewers
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6175b036979e
Convert remaining NS_ENSURE_SUCCESS to IDB_TRY in dom/indexedDB; r=dom-workers-and-storage-reviewers,ttung,sg
https://hg.mozilla.org/integration/autoland/rev/65068ac1fb08
Introduce CACHE_TRY and CACHE_TRY_VAR macros; r=ttung
https://hg.mozilla.org/integration/autoland/rev/237cfeab38fd
Convert remaining NS_ENSURE_SUCCESS to CACHE_TRY in dom/cache; r=dom-workers-and-storage-reviewers,ttung,sg
https://hg.mozilla.org/integration/autoland/rev/99e8811aad2c
Introduce SDB_TRY and SDB_TRY_VAR macros; r=ttung
https://hg.mozilla.org/integration/autoland/rev/77cecccb140a
Introduce LS_TRY and LS_TRY_VAR macros; r=ttung
Keywords: leave-open
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla80
You need to log in before you can comment on or make changes to this bug.