1651016 - Convert NS_ENSURE_SUCCESS and NS_ENSURE_TRUE in Quota manager and quota clients

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, task)

Description

[Jan Varga [:janv]]

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.

Comment 1

[Simon Giesecke [:sg] [he/him]]

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.

Comment 2

[Tom Tung [:tt, :ttung]]

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?

Comment 3

[Jan Varga [:janv]]

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) ?

Comment 4

[Simon Giesecke [:sg] [he/him]]

(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.

Comment 5

[Jan Varga [:janv]]

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

Comment 6

[Jan Varga [:janv]]

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.

Comment 7

[Jan Varga [:janv]]

Attachment: Bug 1651016 - Introduce QM_TRY and QM_TRY_VAR macros; r=#dom-workers-and-storage-reviewers

Comment 8

[Jan Varga [:janv]]

Attachment: Bug 1651016 - Allow MOZ_TRY/QM_TRY to handle bool values by wrapping them with OkIf; r=#dom-workers-and-storage-reviewers

Depends on D83927

Comment 9

[Jan Varga [:janv]]

Attachment: Bug 1651016 - Allow MOZ_TRY/QM_TRY to handle nsresult values and optionally enforce success for some special values; r=#dom-workers-and-storage-reviewers

Depends on D83928

Comment 10

[Jan Varga [:janv]]

Attachment: Bug 1651016 - Convert NS_ENSURE_SUCCESS and NS_ENSURE_TRUE in EnsureDirectory; r=#dom-workers-and-storage-reviewers

Comment 11

[Jan Varga [:janv]]

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

Comment 12

[Jan Varga [:janv]]

Attachment: Bug 1651016 - Convert remaining NS_ENSURE_TRUE to QM_TRY; r=#dom-workers-and-storage-reviewers

Depends on D83931

Comment 13

[Jan Varga [:janv]]

Attachment: Bug 1651016 - Convert remaining NS_ENSURE_SUCCESS to QM_TRY in ActorsParent.cpp; r=#dom-workers-and-storage-reviewers

Depends on D83953

Comment 14

[Jan Varga [:janv]]

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

Comment 15

[Jan Varga [:janv]]

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

Comment 16

[Jan Varga [:janv]]

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

Comment 17

[Jan Varga [:janv]]

Attachment: Bug 1651016 - Convert remaining NS_ENSURE_SUCCESS to QM_TRY in FileStreams.cpp; r=#dom-workers-and-storage-reviewers

Depends on D83958

Comment 18

[Jan Varga [:janv]]

Attachment: 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

Depends on D84312

Comment 19

[Jan Varga [:janv]]

Attachment: Bug 1651016 - Introduce IDB_TRY and IDB_TRY_VAR macros; r=#dom-workers-and-storage-reviewers

Depends on D84313

Comment 20

[Jan Varga [:janv]]

Attachment: Bug 1651016 - Convert remaining NS_ENSURE_SUCCESS to IDB_TRY in dom/indexedDB; r=#dom-workers-and-storage-reviewers

Depends on D84314

Comment 21

[Jan Varga [:janv]]

Attachment: Bug 1651016 - Introduce CACHE_TRY and CACHE_TRY_VAR macros; r=#dom-workers-and-storage-reviewers

Depends on D84315

Comment 22

[Jan Varga [:janv]]

Attachment: Bug 1651016 - Convert remaining NS_ENSURE_SUCCESS to CACHE_TRY in dom/cache; r=#dom-workers-and-storage-reviewers

Depends on D84316

Comment 23

[Jan Varga [:janv]]

Attachment: Bug 1651016 - Introduce SDB_TRY and SDB_TRY_VAR macros; r=#dom-workers-and-storage-reviewers

Depends on D84318

Comment 24

[Jan Varga [:janv]]

Attachment: Bug 1651016 - Introduce LS_TRY and LS_TRY_VAR macros; r=#dom-workers-and-storage-reviewers

Depends on D84663

Comment 25

[Jan Varga [:janv]]

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

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

Comment 27

[Alexandru Michis [:malexandru]]

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

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c5a8759b3f85
Introduce QM_TRY and QM_TRY_VAR macros; r=dom-workers-and-storage-reviewers,ttung,sg

Comment 29

[Andreea Pavel [:apavel]]

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

Comment 30

[Jan Varga [:janv]]

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

Comment 31

[Jan Varga [:janv]]

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

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2686e984d999
Introduce QM_TRY and QM_TRY_VAR macros; r=dom-workers-and-storage-reviewers,ttung,sg

Comment 33

[Pulsebot]

Pushed by [e-mail]:
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

Comment 34

[Pulsebot]

Pushed by [e-mail]:
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

Comment 35

[Cristina Coroiu [:ccoroiu]]

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

Comment 36

[Noemi Erli[:noemi_erli]]

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