1686191 - Have a way to specifically report warnings around QM_TRY

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, task)

Description

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

Currently, the QM_TRY mechanism doesn't allow to distinguish between errors that cause the failure of some operation and warnings for non-standards situations that can be mitigated without failing the overall operation. The choice in the latter case is currently between not using QM_TRY (and, in particular, having no telemetry report) or reporting it like an error. While it's possible to identify these from the telemetry reports, it requires additional work for each analyzed build. It would be better to allow distinguishing this in the first place, ideally in a way that expresses the intent in code and allows to remove additional comments.

Comment 1

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

This could be used, e.g. at https://searchfox.org/mozilla-central/rev/31ddf859c57e812878a5f817e4097efb06de4d97/dom/quota/ActorsParent.cpp#9743

Comment 2

[Jan Varga [:janv]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #1)

This could be used, e.g. at https://searchfox.org/mozilla-central/rev/31ddf859c57e812878a5f817e4097efb06de4d97/dom/quota/ActorsParent.cpp#9743

Yes, and also here: https://phabricator.services.mozilla.com/D98076#inline-569530 (not critical though).

Comment 3

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

One way to address this might seem to change the implementation of the existing QM_TRY macros to automatically detect cases where an error is not propagated, because a custom result is provided that is not an error (not GenericErrorResult<T> and not a non-NS_OK nsresult), but this seems to be too smart. It is also not sufficient alone, since it wouldn't cover uses of Result::orElse, which consume an error result.

Possible approaches to this include:

• Add a function to explicitly add a warning, and have the implementation of that function emit that as a dom.quota.try telemetry event
• Add *_WARN variants of QM_TRY that don't propagate the error, and doesn't simply use a custom return value, but accepts a functor like the one that can be passed to Result::orElse. This would allow to cover most of the existing uses; it would make the syntax for the simple case where a custom non-error return value was provided a bit more verbose (e.g. QM_TRY_WARN(OkIf(originProps.mType != OriginProps::eInvalid), [](const nsresult) { return mozilla::Ok{}; ))).

While the first approach doesn't require the introduction of new macro variants, it will lead to a less uniform handling and miss the self-documenting property of the second approach. While this might be added additionally for very special situations, implementing the second approach might actually be sufficient, and could replace all NS_WARNINGs with custom messages.

Comment 4

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

Attachment: Bug 1686191 - Remove module argument from LogError. r=#dom-workers-and-storage

The module argument in LogError is redundant: the module information can
already be determined from the source file path. Indeed, reporting the module
separately in the telemetry events seems unnecessary. Instead, the relative
path is now reported. This is what the analysis of the telemetry data did
reconstruct anyway.

As a consequence, it's no longer necessary to define module-specific
HandleError functions, and therefore it's also unnecessary to define
module-specific TRY macros. These are retained as simple aliases for now,
but can be removed in a later patch entirely.

This also avoids misuses of a TRY macros in the wrong module, which were
happening a few times before, resulting in confusing output or telemetry
events.

Since Bug 1686191 will add some more TRY macro variants that warn, so
simplifying the module-specific aspects will simplify that task. Furthermore,
this is in preparation of moving the TRY macro extensions to MFBT.

Comment 5

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

Attachment: Bug 1686191 - Added a comment on the sequence number. r=#dom-workers-and-storage

Depends on D102767

Comment 6

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

Attachment: Bug 1686191 - Fix spelling of occurred. r=#dom-workers-and-storage

Depends on D102768

Comment 7

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

Attachment: Bug 1686191 - Add support for warnings integrated with QM_TRY. r=#dom-workers-and-storage

Depends on D102769

Comment 8

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

Attachment: Bug 1686191 - Add QM_TRY_OR_WARN and QM_TRY_WARN_ONLY macros. r=#dom-workers-and-storage

Depends on D102770

Comment 9

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

Attachment: Bug 1686191 - WIP: Add QM_LOG_WARNING macro. r=#dom-workers-and-storage

Depends on D102771

Comment 10

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

Attachment: Bug 1686191 - Replace non-propagating uses of NS_WARNING with redundant messages by QM_TRY_WARN_ONLY. r=#dom-workers-and-storage

Depends on D102772

Comment 11

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

Attachment: Bug 1686191 - Replace uses of QM_TRY with orElse by QM_TRY_OR_WARN. r=#dom-workers-and-storage

Depends on D102945

Comment 12

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

Attachment: Bug 1686191 - Replace uses of QM_TRY inside an extra lambda with QM_TRY_WARN_ONLY. r=#dom-workers-and-storage

Depends on D103031

Comment 13

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

Attachment: Bug 1686191 - Replace uses of QM_TRY with QM_VOID with QM_TRY_WARN_ONLY. r=#dom-workers-and-storage

Depends on D103166

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e29359c8d2fc
Added a comment on the sequence number. r=dom-workers-and-storage-reviewers,janv DONTBUILD
https://hg.mozilla.org/integration/autoland/rev/0dfece814501
Fix spelling of occurred. r=dom-workers-and-storage-reviewers,janv DONTBUILD

Comment 15

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/e29359c8d2fc
https://hg.mozilla.org/mozilla-central/rev/0dfece814501

Comment 16

[Jan Varga [:janv]]

I've been thinking about this lately, but I need to do a bit more investigation.
So far, it looks like these patches don't require a lot of discussion:
https://phabricator.services.mozilla.com/D102767
https://phabricator.services.mozilla.com/D102770
https://phabricator.services.mozilla.com/D102772
https://phabricator.services.mozilla.com/D102945

I'll try to put my investigation into a comment here. Then we can go through all the patches.

Comment 17

[Jan Varga [:janv]]

Attachment: Bug 1686191 - Add experimental QM_WARNING_TRY macro and use it in various places

This is a WIP patch which can be used as a base for a broader discussion about
the support for warnings in QM_TRY telemetry.

Comment 18

[Jan Varga [:janv]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #0)

Currently, the QM_TRY mechanism doesn't allow to distinguish between errors that cause the failure of some operation and warnings for non-standards situations that can be mitigated without failing the overall operation. The choice in the latter case is currently between not using QM_TRY (and, in particular, having no telemetry report) or reporting it like an error. While it's possible to identify these from the telemetry reports, it requires additional work for each analyzed build. It would be better to allow distinguishing this in the first place, ideally in a way that expresses the intent in code and allows to remove additional comments.

I collected a list of areas which need to be addressed:

1. Conversion of existing (not necessarily all) NS_WARNING/QM_WARNING
   Currently, QM_TRY can only report errors, and in case of an error, it always return from a function.
   Example: https://searchfox.org/mozilla-central/rev/87a8afd9f57ee4bc542ba0ec3f96a891042b6db7/dom/cache/CacheParent.cpp#60

2. Missing warnings in cases when a specific errors is converted to a success
   This is not directly about QM_TRY, but rather about having a way to issue a warning which is reported to telemetry too.
   Example: https://searchfox.org/mozilla-central/rev/87a8afd9f57ee4bc542ba0ec3f96a891042b6db7/dom/cache/QuotaClientImpl.h#99
   Another problem is that passing a generic (template) function to orElse makes it impossible to report concrete line in the source.
   Example: https://searchfox.org/mozilla-central/rev/87a8afd9f57ee4bc542ba0ec3f96a891042b6db7/dom/indexedDB/ActorsParent.cpp#671

3. Having no way to report harmless errors (like syntax errors)
   Example: https://searchfox.org/mozilla-central/rev/87a8afd9f57ee4bc542ba0ec3f96a891042b6db7/dom/indexedDB/IDBDatabase.cpp#372

4. Errors which don't cause overall failures and which should be reported as warnings only
   Example: https://searchfox.org/mozilla-central/rev/87a8afd9f57ee4bc542ba0ec3f96a891042b6db7/dom/indexedDB/ActorsParent.cpp#7683

5. Special NS_ERROR_ABORT error which is used to cancel operations for example during shutdown and which propagates through multiple functions
   Example: https://searchfox.org/mozilla-central/rev/87a8afd9f57ee4bc542ba0ec3f96a891042b6db7/dom/indexedDB/ActorsParent.cpp#13745
   which is called from: https://searchfox.org/mozilla-central/rev/87a8afd9f57ee4bc542ba0ec3f96a891042b6db7/dom/indexedDB/ActorsParent.cpp#13862

I think there are actually two things which QM_TRY currently lacks:

1. Log severity
2. Custom control flow (including no flow control)

Comment 19

[Jan Varga [:janv]]

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

I think there are actually two things which QM_TRY currently lacks:

1. Log severity
2. Custom control flow (including no flow control)

Custom control flow has been rejected at the meeting, TRY implies return if there's an error and having an argument for custom flow control could be confusing and easily overlooked, example:

QM_WARNING_TRY(OkIf(Send__delete__(this)), QM_NO_CTRL);

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14f26f041eb4
Remove module argument from LogError. r=dom-workers-and-storage-reviewers,janv

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eff86418bc03
Add support for warnings integrated with QM_TRY. r=dom-workers-and-storage-reviewers,janv

Comment 22

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/14f26f041eb4
https://hg.mozilla.org/mozilla-central/rev/eff86418bc03

Comment 23

[Jan Varga [:janv]]

Attachment: Bug 1686191 - Have a way to specifically report warnings around QM_TRY; r=asuth,sg,#dom-storage

The new macros are also used in several places.

Comment 24

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

Re-assigning the bug to jan, as the remaining patch is owned by him. The patches already landed on this bug are not solving the core problem, but are preparations for that.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2a3316bd2409
Have a way to specifically report warnings around QM_TRY; r=asuth,sg,dom-storage-reviewers

Comment 26

[Atila Butkovits]

Backed out for causing bustages on QuotaCommon.h.

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

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=YdqohPDLQhWM9H4v6xDxKQ.0&searchStr=linux%2Cx64%2Cdebug%2Cbuild-linux64-base-toolchains%2Fdebug%2Cbb&revision=2a3316bd2409459708d287c82deaeb04ed29e28e

Failure log: https://treeherder.mozilla.org/logviewer?job_id=334243479&repo=autoland&lineNumber=8681

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb08fc2c750e
Have a way to specifically report warnings around QM_TRY; r=asuth,sg,dom-storage-reviewers

Comment 28

[Cristian Brindusan [:cbrindusan]]

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

Comment 29

[Jan Varga [:janv]]

This has been fixed.