1713820 - Enable QM_TRY reporting to the browser console in release builds

Status: RESOLVED FIXED

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

Description

[Jan Varga [:janv]]

I'll provide more details later, I'm currently investigating code size implications.

Comment 1

[Jan Varga [:janv]]

Attachment: Bug 1713820 - Add dedicated QM_LOG_ERROR_ENABLED identifier for conditional LogError compilation; r=#dom-storage

Comment 2

[Jan Varga [:janv]]

Attachment: Bug 1713820 - Move QM_ERROR_STACKS_ENABLED definition to Config.h; r=#dom-storage

Depends on D117132

Comment 3

[Jan Varga [:janv]]

Attachment: Bug 1713820 - Move QM_ENABLE_SCOPED_LOG_EXTRA_INFO definition to Config.h; r=#dom-storage

Depends on D117133

Comment 4

[Jan Varga [:janv]]

Attachment: Bug 1713820 - Rename QM_ENABLE_SCOPED_LOG_EXTRA_INFO to QM_SCOPED_LOG_EXTRA_INFO_ENABLED; r=#dom-storage

Depends on D117134

Comment 5

[Jan Varga [:janv]]

Attachment: Bug 1713820 - Use QM_SCOPED_LOG_EXTRA_INFO_ENABLED in CachingDatabaseConnection instead of defined(EARLY_BETA_OR_EARLIER) || defined(DEBUG); r=#dom-storage

Depends on D117135

Comment 6

[Jan Varga [:janv]]

Attachment: Bug 1713820 - Change the order of helper variable defintion in LogError to match the use in log message construction; r=#dom-storage

Depends on D117136

Comment 7

[Jan Varga [:janv]]

Attachment: Bug 1713820 - Allow to use the context string in other types of logging; r=#dom-storage

Depends on D117137

Comment 8

[Jan Varga [:janv]]

Attachment: Bug 1713820 - Don't report to the browser console if the context string is empty; r=#dom-storage

Depends on D117138

Comment 9

[Jan Varga [:janv]]

Attachment: Bug 1713820 - Introduce dedicated identifiers for different types of logging and define QM_LOG_ERROR_ENABLED only when at least one such type is defined; r=#dom-storage

This patch doesn't enable or disable logging in specific builds.

Depends on D117139

Comment 10

[Jan Varga [:janv]]

Attachment: Bug 1713820 - Enable QM_TRY reporting to the browser console in release builds; r=#dom-storage

Failures are not reported to the browser console until a context information is
available like the storage or temporary storage initialization.

Depends on D117140

Comment 11

[Jan Varga [:janv]]

Attachment: Bug 1713820 - Never inline LogError if the definition is not empty; r=#dom-storage

MOZ_NEVER_INLINE can't 100% guarantee that all compilers never inline such
functions, but the current combination of MOZ_COLD and MOZ_NEVER_INLINE
produces sane builds with only minor code size increase (max 0.19% increase).

Depends on D117141

Comment 12

[Jan Varga [:janv]]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e96eff0eb8ea
Add dedicated QM_LOG_ERROR_ENABLED identifier for conditional LogError compilation; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/be98873b8404
Move QM_ERROR_STACKS_ENABLED definition to Config.h; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/ec6e82c8b989
Move QM_ENABLE_SCOPED_LOG_EXTRA_INFO definition to Config.h; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/0f2b472fe633
Rename QM_ENABLE_SCOPED_LOG_EXTRA_INFO to QM_SCOPED_LOG_EXTRA_INFO_ENABLED; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/a7d288075734
Use QM_SCOPED_LOG_EXTRA_INFO_ENABLED in CachingDatabaseConnection instead of defined(EARLY_BETA_OR_EARLIER) || defined(DEBUG); r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/3ac5bb33ec5d
Change the order of helper variable defintion in LogError to match the use in log message construction; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/97747f7a58cc
Allow to use the context string in other types of logging; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/5ba6c93ff181
Don't report to the browser console if the context string is empty; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/1ba47f35b50d
Introduce dedicated identifiers for different types of logging and define QM_LOG_ERROR_ENABLED only when at least one such type is defined; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/17e9bc4f9bc7
Enable QM_TRY reporting to the browser console in release builds; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/ae93a7d0e2cc
Never inline LogError if the definition is not empty; r=dom-storage-reviewers,jstutte

Comment 14

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/e96eff0eb8ea
https://hg.mozilla.org/mozilla-central/rev/be98873b8404
https://hg.mozilla.org/mozilla-central/rev/ec6e82c8b989
https://hg.mozilla.org/mozilla-central/rev/0f2b472fe633
https://hg.mozilla.org/mozilla-central/rev/a7d288075734
https://hg.mozilla.org/mozilla-central/rev/3ac5bb33ec5d
https://hg.mozilla.org/mozilla-central/rev/97747f7a58cc
https://hg.mozilla.org/mozilla-central/rev/5ba6c93ff181
https://hg.mozilla.org/mozilla-central/rev/1ba47f35b50d
https://hg.mozilla.org/mozilla-central/rev/17e9bc4f9bc7
https://hg.mozilla.org/mozilla-central/rev/ae93a7d0e2cc

Comment 15

[Jan Varga [:janv]]

Comment on attachment 9225849 [details]
Bug 1713820 - Enable QM_TRY reporting to the browser console in release builds; r=#dom-storage

Beta/Release Uplift Approval Request

• User impact if declined: localStorge next-gen (LSNG) is going to be enabled in 90 release for some users (as part of a staged rollout). It will be much easier to identify possible problems reported by users if QM_TRY logging to the browser console is enabled in 90 too.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Code size implications have been analyzed for all platforms and it turns out that the increase is only +0.19% (size of lib XUL) in the worst case. Possible spamming of the browser console shouldn't occur since the logging is currently limited only to specific contexts (storage and temporary storage initialization) and only enabled during the first initialization attempt. There are multiple patches for this, but they are all quite simple. The logging to the browser console have been enabled in early beta or earlier for long time. All in all, these pathces should be quite safe for an uplift.
• String changes made/needed: None

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 9225840 [details]
Bug 1713820 - Add dedicated QM_LOG_ERROR_ENABLED identifier for conditional LogError compilation; r=#dom-storage

approved for 90.0b7, thanks

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 9225847 [details]
Bug 1713820 - Don't report to the browser console if the context string is empty; r=#dom-storage

so many patches :)

Comment 18

[Jens Stutte [:jstutte]]

(In reply to Julien Cristau [:jcristau] from comment #17)

so many patches :)

But they are small and comprehensive :) !

Comment 19

[Julien Cristau [:jcristau]]

Yep, definitely.

Comment 20

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1da51f20f721
https://hg.mozilla.org/releases/mozilla-beta/rev/08201e5b65bb
https://hg.mozilla.org/releases/mozilla-beta/rev/2dd7d08a49c4
https://hg.mozilla.org/releases/mozilla-beta/rev/09014bdb8693
https://hg.mozilla.org/releases/mozilla-beta/rev/d67ca06c85d9
https://hg.mozilla.org/releases/mozilla-beta/rev/4950c5144857
https://hg.mozilla.org/releases/mozilla-beta/rev/51c42176714e
https://hg.mozilla.org/releases/mozilla-beta/rev/1fb0c59d64b4
https://hg.mozilla.org/releases/mozilla-beta/rev/20f24febcc54
https://hg.mozilla.org/releases/mozilla-beta/rev/b295b08ee150
https://hg.mozilla.org/releases/mozilla-beta/rev/307025cdae1d