1709352 - QM: Add support for error stacks

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, task, P2)

Description

[Jan Varga [:janv]]

Jens suggested that we could get precise error stacks if we had a wrapper around nsresult that would hold info about error propagation. I think I figured out how to do that.

Comment 1

[Jan Varga [:janv]]

Attachment: Bug 1709352 - Introduce QMResult; r=#dom-storage

Comment 2

[Jan Varga [:janv]]

Attachment: Bug 1709352 - Add ToResult function for QMResult; r=#dom-storage

Depends on D114241

Comment 3

[Jan Varga [:janv]]

Attachment: Bug 1709352 - Allow QMResult errors to use existing stack id and to increase the frame id during error propagation; r=#dom-storage

Depends on D114242

Comment 4

[Jan Varga [:janv]]

Attachment: Bug 1709352 - Add QMResult specialization for GenericErrorResult; r=#dom-storage

Depends on D114243

Comment 5

[Jan Varga [:janv]]

Attachment: Bug 1709352 - Convert MaybeRemoveLocalStorageArchiveTmpFile to return Result<Ok, QMResult>; r=#dom-storage

Depends on D114244

Comment 6

[Jan Varga [:janv]]

Attachment: Bug 1709352 - Add logging support for QMResult; r=#dom-storage

Depends on D114245

Comment 7

[Jan Varga [:janv]]

Attachment: Bug 1709352 - Reorder appending of extra keys to match the definition in Events.yaml; r=#dom-storage

Depends on D114338

Comment 8

[Jan Varga [:janv]]

Attachment: Bug 1709352 - Add frame_id, process_id and stack_id to QM_TRY telemetry; r=#dom-storage

Depends on D114339

Comment 9

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

Did you analyze the effect of QMResult on code generation?
I am not sure in how many cases we benefit from the unused-zero optimization of Result<V, nsresult> now.

I fear this will make it even more relevant to restrict the builds this gets into.

Maybe QMResult should better be called QMError if it's never used to indicate success.

Comment 10

[Jan Varga [:janv]]

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

Did you analyze the effect of QMResult on code generation?

Not yet, but maybe it's not that urgent if we add some ifdef, see below.

I fear this will make it even more relevant to restrict the builds this gets into.

That's actually a good observation, we can put all extra member variables in QMResult behind the ifdef that we already have for stuff like this.

Maybe QMResult should better be called QMError if it's never used to indicate success.

Originally I wanted to have the ability to use it without mozilla::Result, but maybe it's not needed and then we would rename it to QMError.

Comment 11

[Jan Varga [:janv]]

Attachment: Bug 1709352 - Define LogError only when needed; r=#dom-storage

This makes it consistent with other places where we check EARLY_BETA_OR_EARLIER
and DEBUG identifier. For example HandleError.

Depends on D114340

Comment 12

[Jan Varga [:janv]]

Attachment: Bug 1709352 - Support error stacks only in EARLY_BETA_OR_EARLIER) || DEBUG builds; r=#dom-storage

Comment 13

[Jan Varga [:janv]]

Attachment: Bug 1709352 - Remove the module extra key from LogError; r=#dom-storage

The appening of module extra key was already commented out.

Comment 14

[Jan Varga [:janv]]

Ok, so I was able to limit the overhead to EARLY_BETA_OR_EARLIER || DEBUG builds.
sizeof(Result<Ok, nsresult) and sizeof(Result<Ok, QMResult) is the same in Release builds (the size is 4 bytes).

sizeof(Result<Ok, QMResult) in Nightly builds is 24 bytes.

Comment 15

[Jens Stutte [:jstutte]]

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

Ok, so I was able to limit the overhead to EARLY_BETA_OR_EARLIER || DEBUG builds.
sizeof(Result<Ok, nsresult) and sizeof(Result<Ok, QMResult) is the same in Release builds (the size is 4 bytes).

sizeof(Result<Ok, QMResult) in Nightly builds is 24 bytes.

For the sake of readability I was just wondering if we should have some specifically labeled switch (#define) instead of sparse #if defined(EARLY_BETA_OR_EARLIER) || defined(DEBUG) throughout the code?

Comment 16

[Jan Varga [:janv]]

The new code for error stacks already has it, QM_ERROR_STACKS_ENABLED.

Comment 17

[Jens Stutte [:jstutte]]

I was referring to the ones introduced in Bug 1709352 - Define LogError only when needed; r=#dom-storage, are those the same scope?

Comment 18

[Jan Varga [:janv]]

That can be changed too.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ed477dfcd93
Introduce QMResult; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/922a380b0d86
Add ToResult function for QMResult; r=dom-storage-reviewers,jstutte,asuth
https://hg.mozilla.org/integration/autoland/rev/b4c891b1a31e
Allow QMResult errors to use existing stack id and to increase the frame id during error propagation; r=dom-storage-reviewers,asuth,glandium
https://hg.mozilla.org/integration/autoland/rev/8313509e7cc1
Convert MaybeRemoveLocalStorageArchiveTmpFile to return Result<Ok, QMResult>; r=dom-storage-reviewers,asuth

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e538b40f3e43
Add logging support for QMResult; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/038434c75607
Reorder appending of extra keys to match the definition in Events.yaml; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/6d45855c91a7
Add frame_id, process_id and stack_id to QM_TRY telemetry; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/f1905864fccd
Define LogError only when needed; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/bdfc8a9626c1
Support error stacks only in EARLY_BETA_OR_EARLIER) || DEBUG builds; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/d239739c9637
Remove the module extra key from LogError; r=dom-storage-reviewers,jstutte

Comment 21

[Jan Varga [:janv]]

Attachment: Bug 1709352 - Follow-up patch to fix indentation; r=#dom-storage

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa9b64cfbfc9
Follow-up patch to fix indentation; r=dom-storage-reviewers,jstutte

Comment 23

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/5ed477dfcd93
https://hg.mozilla.org/mozilla-central/rev/922a380b0d86
https://hg.mozilla.org/mozilla-central/rev/b4c891b1a31e
https://hg.mozilla.org/mozilla-central/rev/8313509e7cc1
https://hg.mozilla.org/mozilla-central/rev/e538b40f3e43
https://hg.mozilla.org/mozilla-central/rev/038434c75607
https://hg.mozilla.org/mozilla-central/rev/6d45855c91a7
https://hg.mozilla.org/mozilla-central/rev/f1905864fccd
https://hg.mozilla.org/mozilla-central/rev/bdfc8a9626c1
https://hg.mozilla.org/mozilla-central/rev/d239739c9637
https://hg.mozilla.org/mozilla-central/rev/fa9b64cfbfc9

Comment 24

[Jan Varga [:janv]]

Attachment: Bug 1709352 - Fix compilation when QM_ERROR_STACKS_ENABLED is not defined; r=#dom-storage

LogError signature is now separately defined for the case when
QM_ERROR_STACKS_ENABLED is defined and when it's not defined). HandleError has
been changed as well to have explicit handling depending on
QM_ERROR_STACKS_ENABLED.

Comment 25

[Jan Varga [:janv]]

Attachment: Bug 1709352 - Add missing mozilla/Variant.h include to QuotaCommon.h; r=#dom-storage

Depends on D116070

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f65bf0f5a4d8
Fix compilation when QM_ERROR_STACKS_ENABLED is not defined; r=dom-storage-reviewers,asuth

Comment 27

[Andreea Pavel [:apavel]]

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

Comment 28

[Jan Varga [:janv]]

Attachment: Bug 1709352 - Clean up and polish LogError; r=#dom-storage

Depends on D116121

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13f9d77af67b
Add missing mozilla/Variant.h include to QuotaCommon.h; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/0caad459ce02
Clean up and polish LogError; r=dom-storage-reviewers,jstutte

Comment 30

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/13f9d77af67b
https://hg.mozilla.org/mozilla-central/rev/0caad459ce02