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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Stability, defect)

Description

[Rex Hung[:rhung]]

Attachment: permissions.sqlite_v2.1

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.

Comment 1

[Steven Yang [:styang]]

as triage, plus it

Comment 2

[Rex Hung[:rhung]]

Attachment: Retry-database-connection-for-the-case-of-corrupted-.patch

Create a patch as attachment to delete and re-create /data/local/permissions.sqlite when it is corrupted already.

Comment 3

[Rex Hung[:rhung]]

Attachment: Retry-database-connection-for-the-case-of-corrupted-permissions.sqlite_v2.patch

Update patch files as attachment to confirm the creation of permissions.sqlite and table.

Comment 4

[Rex Hung[:rhung]]

Attachment: Retry-database-connection-for-the-case-of-corrupted-permissions.sqlite_v3.patch

Update a patch with more debug information.

Comment 5

[Rex Hung[:rhung]]

Attachment: Bug-1124126-Retry-database-connection-for-the-case-o.patch

Hi, Fabrice,

Could you help to review this patch or forward it to proper owners?
Thank you.

Comment 6

[[:fabrice] Fabrice Desré]

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.

Comment 7

[Benjamin Smedberg]

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.

Comment 8

[Rex Hung[:rhung]]

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?

Comment 9

[Benjamin Smedberg]

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.

Comment 10

[Rex Hung[:rhung]]

Attachment: Bug-1124126-Retry-database-connection-for-the-case-of-corrupted-DB-file.patch

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.

Comment 11

[Benjamin Smedberg]

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?

Comment 12

[Rex Hung[:rhung]]

Attachment: Bug-1124126-Retry-database-connection-for-the-case-of-corrupted-DB-file.patch

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.

Comment 13

[Rex Hung[:rhung]]

Attachment: Bug-1124126-Retry-database-connection-for-the-case-of-corrupted-DB-file.patch

Comment 14

[Benjamin Smedberg]

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".

Comment 15

[Rex Hung[:rhung]]

Attachment: Bug-1124126-Retry-database-connection-for-the-case-of-corrupted-DB-file.patch

Hi, Benjamin,

I have revised my patch again according to comment 14. Could you have a look to the patch? Thank you.

Comment 16

[Rex Hung[:rhung]]

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

Comment 17

[Rex Hung[:rhung]]

Attachment: Bug-1124126-Retry-database-connection-for-the-case-of-corrupted-DB-file.patch

Addressed reviewer comment carry r+.

Comment 18

[Rex Hung[:rhung]]

Attachment: [v2.2] Bug-1124126-Retry-database-connection-for-the-case-of-corrupted-DB-file.patch

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

Comment 19

[Rex Hung[:rhung]]

Attachment: [v2.1] Bug-1124126-Retry-database-connection-for-the-case-of-corrupted-DB.patch

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

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/d28b6a36dfee

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/fd7b06b22d00
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/23203116eac3

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 23

[Vincent Liu[:vliu]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/23203116eac3