Closed Bug 1542478 Opened 6 years ago Closed 5 years ago

Add more temporary logging to IndexedDBShutdownTimeout crash annotation

Categories

(Core :: Storage: IndexedDB, task, P1)

Product:
Core
Component:
Storage: IndexedDB
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla69
Tracking Flags:
Tracking Status
firefox68 --- fixed

People

(Reporter: janv, Assigned: janv)

References

Details

Attachments

(2 files)

Bug 1542478 - Add more temporary logging to IndexedDBShutdownTimeout crash annotation; r=asuth
(deleted), text/x-phabricator-request
RyanVM
: approval-mozilla-beta+
Details
data-request.txt
(deleted), text/plain
chutten
: data-review-
chutten
: data-review+
Details
No description provided.
Blocks: 1541370
Group: mozilla-employee-confidential

Crash stats in bug 1541776 show that even though we fixed one of the problems that cause shutdown hangs, hangs still occur.
The good news is that it's only caused by gFactoryOps array being not empty.
I'd like to add more logging which would iterate over the array and collect persistence type, origin string and maybe also database name.
We need this info only for short period of time to get an idea what type of IDB databases is causing this.
So we would be able to focus on exact areas in the code.

Assignee: nobody → jvarga
Status: NEW → ASSIGNED

Actually, I just found "gLiveDatabaseHashtable: 1" in this crash report:
https://crash-stats.mozilla.org/report/index/4c578083-db77-413a-967b-034010190406#tab-metadata

It doesn't occur very often.

Latest "IndexedDBShutdownTimeout" contains mostly "gFactoryOps: N", where N look lower.
It seems that bug 1538619 has helped, but there is a problem still.

The priority flag is not set for this bug.
:overholt, could you have a look please?

Flags: needinfo?(overholt)
Type: defect → task
Flags: needinfo?(overholt)
Priority: -- → P3
Keywords: leave-open
Priority: P3 → P1

Chris, you already reviewed this crash annotation in bug 1540668, but we want to temporarily expose more info in the crash annotation.

This is how the crash annotation looks now:
gFactoryOps: N
(N is a number)

We want to change that to something like:
gFactoryOps: N (default*http://aaa.aaaaaaa.aaa*BeginVersionChange, default*https://aaa.aaaaaaa.aaa:DDDD*SendingResults, ...)

http://aaa.aaaaaaa.aaa is a sanitized origin (e.g. for http://www.mozilla.org)
See bug 1536596 comment 6 for more details how we sanitize the origin string.
BeginVersionChange, SendingResults, etc. are just stringified states of an intenal C++ object.

The crash annotation is only visible to users with a special permission (I think it's the Personal Identifiable Information permission).

Do I need to fill out a new data review request ?

Flags: needinfo?(chutten)

Yes, we need to fill out a new data review request as we're going to collect new data.

Flags: needinfo?(chutten)
Attached file data-request.txt (deleted) —
Attachment #9068112 - Flags: data-review?(chutten)
Comment on attachment 9068112 [details] data-request.txt Ack, sorry, forgot to mention. Data Collection Reviews need to be in public bugs so that they're open and accessible. If you need to obscure some information by pointing at moco-conf bugs in the review, then do so sparingly as Data Collection Review is one of the methods by which we show we can be trusted to collect data.
Attachment #9068112 - Flags: data-review?(chutten) → data-review-

Ok, we can make this public (I checked with the reviewer of the patch).

Group: mozilla-employee-confidential
Attachment #9068112 - Flags: data-review?(chutten)
Comment on attachment 9068112 [details] data-request.txt DATA COLLECTION REVIEW RESPONSE: Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate? Yes. This collection is a Crash Report Annotation so is documented in its definitions file [CrashAnnotations.yaml](https://hg.mozilla.org/mozilla-central/file/tip/toolkit/crashreporter/CrashAnnotations.yaml). Is there a control mechanism that allows the user to turn the data collection on and off? Yes. This collection is part of the Crash Reporter which requests explicit opt-in from the user before sending a report. Also, if the user at one time opted to always send crash reports they can opt-out from within Firefox's Preferences. If the request is for permanent data collection, is there someone who will monitor the data over time? Yes, Jan Varga is responsible. Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under? Category 1, Technical. Is the data collection request for default-on or default-off? Default on for all channels. Does the instrumentation include the addition of any new identifiers? No. Is the data collection covered by the existing Firefox privacy notice? Yes. Does there need to be a check-in in the future to determine whether to renew the data? No. This collection is permanent. --- Result: datareview+
Attachment #9068112 - Flags: data-review?(chutten) → data-review+
Pushed by jvarga@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a5050f53db29 Add more temporary logging to IndexedDBShutdownTimeout crash annotation; r=asuth, dr=chutten

It seems we now have enough debugging info.

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Blocks: idb-shutdown-hangs

Comment on attachment 9066340 [details]
Bug 1542478 - Add more temporary logging to IndexedDBShutdownTimeout crash annotation; r=asuth

Beta/Release Uplift Approval Request

  • User impact if declined: We were able to reduce IndexedDB shutdown hangs thanks to additional debugging info added to crash annotations. However, there might be other types of shutdown hangs which appear only in beta/release so it would be helpful to have this debugging info on beta/release 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): This patch landed 2 weeks ago. There were no regressions or other problems related to this patch. The patch has data review and data is actually visible to crash-stat accounts with special permission only. Origin strings are sanitized, so it's not directly obvious what sites the user visited.
  • String changes made/needed: None
Attachment #9066340 - Flags: approval-mozilla-beta?

Comment on attachment 9066340 [details]
Bug 1542478 - Add more temporary logging to IndexedDBShutdownTimeout crash annotation; r=asuth

Improves annotations for IndexedDB shutdown hang reports. Approved for 68.0b11.

Attachment #9066340 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: