Closed Bug 1713820 Opened 8 months ago Closed 8 months ago

Enable QM_TRY reporting to the browser console in release builds

Categories

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

task

Tracking

()

RESOLVED FIXED
91 Branch
Tracking Status
firefox90 + fixed
firefox91 --- fixed

People

(Reporter: janv, Assigned: janv)

References

(Regressed 1 open bug)

Details

Attachments

(11 files)

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

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

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

Depends on D117140

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

Pushed by jvarga@mozilla.com:
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 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
Attachment #9225849 - Flags: approval-mozilla-beta?
Attachment #9225840 - Flags: approval-mozilla-beta?
Attachment #9225841 - Flags: approval-mozilla-beta?
Attachment #9225842 - Flags: approval-mozilla-beta?
Attachment #9225843 - Flags: approval-mozilla-beta?
Attachment #9225844 - Flags: approval-mozilla-beta?
Attachment #9225845 - Flags: approval-mozilla-beta?
Attachment #9225846 - Flags: approval-mozilla-beta?
Attachment #9225847 - Flags: approval-mozilla-beta?
Attachment #9225848 - Flags: approval-mozilla-beta?
Attachment #9225850 - Flags: approval-mozilla-beta?

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

Attachment #9225840 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9225841 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9225842 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9225843 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9225844 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9225845 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9225846 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Attachment #9225847 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9225848 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9225849 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9225850 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

so many patches :)

But they are small and comprehensive :) !

Yep, definitely.

Regressions: 1717843
You need to log in before you can comment on or make changes to this bug.