1542478 - Add more temporary logging to IndexedDBShutdownTimeout crash annotation

Status: RESOLVED FIXED

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

Comment 1

[Jan Varga [:janv]]

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.

Comment 2

[Jan Varga [:janv]]

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.

Comment 3

[Jan Varga [:janv]]

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

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 5

[Jan Varga [:janv]]

Attachment: Bug 1542478 - Add more temporary logging to IndexedDBShutdownTimeout crash annotation; r=asuth

Comment 6

[Jan Varga [:janv]]

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 ?

Comment 7

[Chris H-C :chutten]

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

Comment 8

[Jan Varga [:janv]]

Attachment: data-request.txt

Comment 9

[Chris H-C :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.

Comment 10

[Jan Varga [:janv]]

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

Comment 11

[Chris H-C :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+

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5050f53db29
Add more temporary logging to IndexedDBShutdownTimeout crash annotation; r=asuth, dr=chutten

Comment 13

[Noemi Erli[:noemi_erli]]

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

Comment 14

[Jan Varga [:janv]]

It seems we now have enough debugging info.

Comment 15

[Jan Varga [:janv]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c1a2af7d053d