Closed Bug 1124126 Opened 6 years ago Closed 5 years ago

[FFOS7715 v2.1] permissions.sqlite might be corrupted during power off test

Categories

(Firefox OS Graveyard :: Stability, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.1S+, firefox39 wontfix, firefox40 wontfix, firefox41 fixed, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
2.2 S13 (29may)
blocking-b2g 2.1S+
Tracking Status
firefox39 --- wontfix
firefox40 --- wontfix
firefox41 --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: rhung, Assigned: rhung)

References

Details

Attachments

(4 files, 8 obsolete files)

4.00 KB, application/x-sqlite3
Details
4.09 KB, patch
Details | Diff | Splinter Review
4.89 KB, patch
Details | Diff | Splinter Review
4.05 KB, patch
Details | Diff | Splinter Review
When running power off test, permissions.sqlite might be corrupted when creating the table(moz_hosts). Currently, there is no error handling or permissions.sqlite and table re-creation, so the file keeps corrupted in subsequent reboot and app permissions are never be loaded to it successfully.
blocking-b2g: --- → 2.1S?
as triage, plus it
blocking-b2g: 2.1S? → 2.1S+
Assignee: nobody → rhung
Create a patch as attachment to delete and re-create /data/local/permissions.sqlite when it is corrupted already.
Update patch files as attachment to confirm the creation of permissions.sqlite and table.
See Also: → 1115619
Update a patch with more debug information.
Hi, Fabrice,

Could you help to review this patch or forward it to proper owners?
Thank you.
Flags: needinfo?(fabrice)
Attachment #8579185 - Flags: review?(fabrice)
Flags: needinfo?(fabrice)
Comment on attachment 8579185 [details] [diff] [review]
Bug-1124126-Retry-database-connection-for-the-case-o.patch

Review of attachment 8579185 [details] [diff] [review]:
-----------------------------------------------------------------

Redirecting to Benjamin since he seems to have reviewed some of the recents changes there.
Attachment #8579185 - Flags: review?(fabrice) → review?(benjamin)
Comment on attachment 8579185 [details] [diff] [review]
Bug-1124126-Retry-database-connection-for-the-case-o.patch

I like where this is going, however I don't think we should be dumping to stderr for this sort of thing. There are two things we should be doing for these kinds of errors:

* sending a console message so that it appears in the browser console
* sending a telemetry probe

That should happen both with generic InitDB failure and in the specific case of NS_ERROR_FILE_CORRUPTED.
Attachment #8579185 - Flags: review?(benjamin) → review-
Hi, Benjamin,

Thank you for your comments.
The reason why using "printf_stderr" is because I can check if DB initialization is failed via adb logs(adb logcat) for the case of b2g. If using the two ways you mentioned at comment#7, could you let me know how to check this DB init failure for the case of b2g?
Flags: needinfo?(benjamin)
I don't know b2g code that well, but presumably the remote debugger has access to the console messages. The point of telemetry is that we can count these errors in the field: I don't know whether B2G has telemetry enabled or not.
Flags: needinfo?(benjamin)
Hi, Benjamin,

I have revised my patch according to comment 9, could you help to take a look at my patch? About the telemetry, I found that it is not enabled by default for all projects. In dolphin 2.1s, it is not enabled, but it is enabled in flame master branch. However, I still add a telemetry probe for generic InitDB failure since it is a common bug both for B2G and firefox browser.
Attachment #8579185 - Attachment is obsolete: true
Attachment #8595782 - Flags: review?(benjamin)
Comment on attachment 8595782 [details] [diff] [review]
Bug-1124126-Retry-database-connection-for-the-case-of-corrupted-DB-file.patch

This patch records telemetry/log if InitDB fails completely. But it doesn't record the case where the initial OpenDatabase fails but the subsequent remove/OpenDatabase succeeds. Which means that if we throw away the user's permissions, we won't notice.

Can we log and measure the initial failure case also?
Flags: needinfo?(rhung)
Hi, Benjamin,

Agree on your points.
Besides, I think it would be better to focus on the recovery of a corrupted permissions.sqlite in this bug. So, I have revised the patch considerably again. Could you take a look to the patch?
The patch focus on the recovery of corrupted permissions.sqlite and recording related information in console messages and telemetry if it is still failed, instead of handling the case of InitDB fails completely. Furthermore, the patch also records the message about remove/OpenDatabase succeed and a DB connection is established successfully to the valid permissions.sqlite.
Attachment #8595782 - Attachment is obsolete: true
Attachment #8595782 - Flags: review?(benjamin)
Flags: needinfo?(rhung) → needinfo?(benjamin)
Attachment #8599156 - Attachment is obsolete: true
Attachment #8599160 - Flags: review?(benjamin)
Comment on attachment 8599160 [details] [diff] [review]
Bug-1124126-Retry-database-connection-for-the-case-of-corrupted-DB-file.patch

This looks good, but I'd like another telemetry probe to count "Defective permissions.sqlite has been removed".
Flags: needinfo?(benjamin)
Attachment #8599160 - Flags: review?(benjamin) → feedback+
Hi, Benjamin,

I have revised my patch again according to comment 14. Could you have a look to the patch? Thank you.
Attachment #8599160 - Attachment is obsolete: true
Flags: needinfo?(benjamin)
Attachment #8606899 - Flags: review?(benjamin)
Flags: needinfo?(benjamin)
Attachment #8606899 - Flags: review?(benjamin) → review+
Addressed reviewer comment carry r+.
Attachment #8606899 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #8554379 - Attachment is obsolete: true
Attachment #8563859 - Attachment is obsolete: true
Attachment #8571226 - Attachment is obsolete: true
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Potential bug.
User impact if declined: Device cannot boot up under some conditions.
Testing completed: Pass monkey test locally 
Risk to taking this patch (and alternatives if risky): Low, only retry DB file creation when it is corrupted.
String or UUID changes made by this patch: None
Attachment #8608524 - Flags: approval-mozilla-b2g37?
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Potential bug.
User impact if declined: Device cannot boot up under some conditions.
Testing completed: Pass monkey test locally.
Risk to taking this patch (and alternatives if risky): Low, only retry DB file creation when it is corrupted.
String or UUID changes made by this patch: None
Attachment #8608527 - Flags: approval-mozilla-b2g34?
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Attachment #8608527 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Attachment #8608524 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
https://hg.mozilla.org/mozilla-central/rev/d28b6a36dfee
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S13 (29may)
No longer blocks: 1211981
You need to log in before you can comment on or make changes to this bug.