1504535 - Completely broken extension data storage -- due to Firefox creating directory in profile/storage/default

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect, P2)

Description

[Sergey Olefir]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0

Steps to reproduce:

I used Firefox normally until I restarted it at some point and most (all?) of the extensions data that I use wouldn't load.

This wasn't the first time that it happened, so I've spent a considerable amount of time trying to figure out what has happened / how to fix it -- turns out Firefox for whatever reason created certain directory names in profile/storage/default that completely broke extension data storage.

This can be reproduced by doing the following:
- Download Portable Firefox -- FirefoxPortable_63.0.1_English.paf.exe (the same also happens on Firefox 62)
- Install it
- Run it
- Install some extension that will be used to detect that extension data is not loading, I'm using https://addons.mozilla.org/en-US/firefox/addon/styl-us/?src=search
-- Open Stylus, press 'Manage'
-- Create some style, I just put /*test*/ in the style and named it 'Test'.
-- Save Style
- Restart Firefox, open Stylus, confirm you can see the style that you've created.
- Close Firefox
- Create directory <FirefoxPortableDir>\Data\profile\storage\default\https+++www.walmart.ca\idb\3444488154t5e1s9t6-203.93458848.files\  
- Start Firefox
- Open Stylus, press 'Manage' -- observe that the style that was created is not present -- extension data is not loading.
- Close Firefox
- Remove created directory (sufficient to only remove the last directory: 3444488154t5e1s9t6-203.93458848.files )
- Start Firefox, observe that style is now present in Stylus.

I want to stress that I did *NOT* create this directory myself -- this is something that Firefox did by itself thus completely breaking itself. I have to mention that I do use certain backup applications that back up Firefox directory -- it is somewhat possible that Firefox might have tried to delete this directory at some point but it was not allowed due to it being held by the process doing the back up.


Actual results:

Extension data storage (possibly other data storage) is completely broken -- nothing works.


Expected results:

Everything should work normally.

Comment 1

[Rares Doghi, Desktop QA]

Hi, We managed to reproduce this issue on Portable Firefox 63.0.1 after creating the folders, the "Test" style created was no longer displayed in the Manage section, until we deleted the folder from https+++www.walmart.ca\idb\.

I tested this issue using the 63.0.1 portable version of Firefox, but this issue occurs on Nightly 65.0a1 (2018-11-09) and Beta 64.0b8 (64-bit) as well.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

If you follow the process at https://wiki.mozilla.org/CloudServices/Sync/File_a_desktop_bug, you should find some information about the failure in the generated logs.

Comment 3

[Rares Doghi, Desktop QA]

Hi Mark, After setting everything in about:config I'm getting 0 Error logs created using that "about:sync" addon, when I'm reproducing this issue there are no errors displayed. When this folder exists 
\storage\default\https+++www.walmart.ca\idb\3444488154t5e1s9t6-203.93458848.files and I reach the "Manage" section of the Stylus, it just doesn't display any of the Styles I created previously. 

There are no Failures, no Errors its almost like the Addon is working normally just starts with a different path.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

My apologies - I thought this bug was about chrome.storage.sync not working correctly for this addon, but that appears to not be the case. My instructions re the logs would only have helped it is was. Sorry for the noise.

Comment 5

[David Durst [:ddurst]]

Can you reproduce this in a non-portable build?

Comment 6

[Sergey Olefir]

Rares Doghi above indicated that the issue occurs in other Firefox version as well.

My understanding is that portable build isn't different from normal Firefox at all other than basically specifying the profile path?

If needed, I can do some additional testing, but how can I test the non-portable build without breaking my current Firefox configuration?

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

So, this definitely looks like a duplicate of Bug 944918 / Bug 1423917.

In this particular case the path seems to be in a valid format, but it is likely that other files (1) expected to be in that storage sub-directory are actually missing (e.g. the backup software may have missed to backup some of all those additional files, maybe because Firefox could have been keeping those files opened and prevent any other windows process to even read them).

As an example:

- If I open Firefox on a new profile and then navigate a tab to walmart.ca, then a directory with a name using that format is created (to store the walmart's IndexedDB data), and after restarting Firefox everything still works as expected. 

- But if I manually create that directory (so that all the other files expected there are going to be missing), 
  then an UnknownError DOMExpection is raised as soon as any webpage or webextensions page tries to open an IndexedDB database 
  (Bug 944918)

The stylus extension internally has a fallback logic (2) which change the kind of storage that is going to be used when IndexedDB fails to open.

[1]: e.g. there should be a ".metadata" and a ".metadata-v2" files, and a file with a sqlite exension
[2]: https://github.com/openstyles/stylus/blob/b622ebc172a302027f7a478c46e3b3aea5332364/background/db.js#L20-L33

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Luca Greco [:rpl] from comment #7) 
>   then an UnknownError DOMExpection is raised as soon as any webpage or

Typo: DOMExpection => DOMException

Comment 9

[Tom Tung [:tt, :ttung]]

Hi Sergey,

I guess I found the problem that you had. However, it's not clear to me how can I run into the same situation. I want to make sure I understand correctly about the steps you have. Could you tell me more about that? Thanks!

(In reply to Sergey Olefir from comment #0)
> - Create directory
> <FirefoxPortableDir>\Data\profile\storage\default\https+++www.walmart.
> ca\idb\3444488154t5e1s9t6-203.93458848.files\  

Is the above directory created by the Firefox based on what you described below? 

> I want to stress that I did *NOT* create this directory myself -- this is
> something that Firefox did by itself thus completely breaking itself. I have

As far as I know the unexpected things from the perspective of IndexedDB/QuotaManager in Firefox, the name of folder like "3444488154t5e1s9t6-203.93458848.files" shouldn't exist. The expected name should be like "3444488154wBaDltmra.files". It looks like the encoding/decoding methods of Firefox or other applications have some problems or somethings like that. 

Besides, IIUC, after removing that problematic folder, your firefox can run normally, right? If so, it seems that the folder might not be generated by the firefox itself at this point. (Otherwise, the folder that causing issues should be generated again. And, your firefox should run into the same or similar issues again)

So, could you show me the applications you are using for the backup so that I could investigate that more? Or do you have any idea that how can the folder like "3444488154t5e1s9t6-203.93458848.files" (having two dots, a dash) be generated?

Comment 10

[Tom Tung [:tt, :ttung]]

(In reply to Rares Doghi from comment #1)
> Hi, We managed to reproduce this issue on Portable Firefox 63.0.1 after
> creating the folders, the "Test" style created was no longer displayed in
> the Manage section, until we deleted the folder from
> https+++www.walmart.ca\idb\.
> 
> I tested this issue using the 63.0.1 portable version of Firefox, but this
> issue occurs on Nightly 65.0a1 (2018-11-09) and Beta 64.0b8 (64-bit) as well.

Hi Rares,

Could you show me how do you reproduce the issue? Did you somehow make the Firefox generate the folders like "3444488154t5e1s9t6-203.93458848.files"? Or you put them into your profile directory manually?

Would you mind elaborating more about the steps to reproduce the file if you know how to generate that? Thanks!

Comment 11

[Tom Tung [:tt, :ttung]]

(In reply to Luca Greco [:rpl] from comment #7)
> So, this definitely looks like a duplicate of Bug 944918 / Bug 1423917.

It does look like. However, I guess they are running into a similar issue (fail to initialize QuotaManager), but the reason for causing the issue seems different based on the clues I can get for now.

> In this particular case the path seems to be in a valid format, but it is
> likely that other files (1) expected to be in that storage sub-directory are
> actually missing (e.g. the backup software may have missed to backup some of
> all those additional files, maybe because Firefox could have been keeping
> those files opened and prevent any other windows process to even read them).
> 
> As an example:
> 
> - If I open Firefox on a new profile and then navigate a tab to walmart.ca,
> then a directory with a name using that format is created (to store the
> walmart's IndexedDB data), and after restarting Firefox everything still
> works as expected. 
> - But if I manually create that directory (so that all the other files
> expected there are going to be missing), 
>   then an UnknownError DOMExpection is raised as soon as any webpage or
> webextensions page tries to open an IndexedDB database 
>   (Bug 944918)

Actually, if the folder name is correct, then files inside should be able to be recovered. I removed the files inside the "idb" folder and "https+++www.walmart.ca" folder separately, and my Firefox worked fine (don't have an initialization failure on QuotaManager) after restarting.

There is a corner case, though. If the website's origin name is obsolete (somehow unexpectedly created by Firefox in the past, but the name of it is not supported e.g. moz-safe-about+++home) and you removed the metadata file insides, Firefox would encounter the initialization failure. Please let me know or file bugs under the meta bug 1482662. They should be fixed once bug Bug 1423917 is resolved.

Comment 12

[Sergey Olefir]

(In reply to Tom Tung [:tt, :ttung] from comment #9)
> > - Create directory
> > <FirefoxPortableDir>\Data\profile\storage\default\https+++www.walmart.
> > ca\idb\3444488154t5e1s9t6-203.93458848.files\  
> 
> Is the above directory created by the Firefox based on what you described
> below? 

Yes, to the best of my knowledge this directory was created by Firefox itself. I certainly did not create it by myself (well, except later when reproducing an issue on the clean installs). This problem has happened more than once, btw.

I do not believe that my backup software (or anything else really) would create such a directory -- except for Firefox itself. For backups I was using at the time Backblaze (nightly backups) and CrashPlan (which was set to back up Firefox folder every 15 minutes). Of possible note -- I haven't seen this issue since I've stopped using CrashPlan (and started using Duplicati).

The only reason I even mentioned the backup software is a theory that the folder is created by Firefox (for whatever reason) and is subsequently removed automatically -- it is theoretically plausible that the folder fails to be removed sometimes because it is held open by the backup software at that moment.

I haven't ever seen either of backup softwares to create any folders (amid the folders being backed up).

Comment 13

[Sergey Olefir]

To clarify -- "This problem has happened more than once, btw" -- means I have had Firefox fail to work properly in this fashion more than once. It is only the last time that I've spent time to figure out the problem with extra directory.

On all previous occasions I've just restore some backup from hours/days before that wouldn't exhibit the problem. But I'm assuming that the cause was more or less the same.

Comment 14

[Tom Tung [:tt, :ttung]]

(In reply to Sergey Olefir from comment #12)
> Yes, to the best of my knowledge this directory was created by Firefox
> itself. I certainly did not create it by myself (well, except later when
> reproducing an issue on the clean installs). This problem has happened more
> than once, btw.

I see. Thanks for the explanation and clarification!

What you did for finding the "3444488154t5e1s9t6-203.93458848.files" folder does help me narrow down the scope.

I guess the next step I need to do is that somehow figure out why does Firefox (or other application, but it seems they don't) create this folder.

Comment 15

[Sergey Olefir]

I am not sure about underlying database mechanism and how feasible that would be, but I think that on startup Firefox should remove any such empty folders under /storage/ if they are bound to cause such problems. There's no reason to completely and silently 'crash' the entire Firefox data storage due to some random empty directory names (whatever might be the reason that they've appeared there).

Figuring and fixing the cause of such directories appearing would be great too, but I think in this case fixing the result is more important than finding the cause -- particularly as I can't imagine what removing the *empty* directories under /storage/ might plausibly break. I assume that the 'contract' is that everything under /profile/ is managed by Firefox and users should not expect to be able to keep 'theirs' files and folders there.

Comment 16

[Tom Tung [:tt, :ttung]]

(In reply to Luca Greco [:rpl] from comment #7)
Luca, thanks for connecting this issue! And, you are right. the reason for failing to use the extension is due to storage initialization. However, based on what I got so far, it seems to me that this issue is more related to indexedDB itself. It somehow creates an unhandlable folder (or other applications do).

Based on what I understand so far, I'd like to reopen this issue, change the relationship of it and deal with it in this bug. Please feel free to change that if you have another opinion or you find something else.

Comment 17

[Tom Tung [:tt, :ttung]]

(In reply to Sergey Olefir from comment #15)
> I am not sure about underlying database mechanism and how feasible that
> would be, but I think that on startup Firefox should remove any such empty
> folders under /storage/ if they are bound to cause such problems. There's no
> reason to completely and silently 'crash' the entire Firefox data storage
> due to some random empty directory names (whatever might be the reason that
> they've appeared there).
> 
> Figuring and fixing the cause of such directories appearing would be great
> too, but I think in this case fixing the result is more important than
> finding the cause -- particularly as I can't imagine what removing the
> *empty* directories under /storage/ might plausibly break. I assume that the
> 'contract' is that everything under /profile/ is managed by Firefox and
> users should not expect to be able to keep 'theirs' files and folders there.

Thanks for noticing! I guess I might not mention that clearly. I'll definitely fix the result, but that should be done after fixing the cause if it was caused by Firefox. Such unnecessary IO (creating these problematic folders) shouldn't be spent if they are created by Firefox even if deleting or ignoring them is simple.

Comment 18

[Tom Tung [:tt, :ttung]]

For fixing the result, I guess the [1] and [2] need to be loosened (rather just returning an error) here so that an unexpected directory won't block the initialization anymore.

[1] https://searchfox.org/mozilla-central/rev/e22c0d152060f4f8d4ca8904094f15f65a1b6f93/dom/indexedDB/ActorsParent.cpp#17500
[2] https://searchfox.org/mozilla-central/rev/e22c0d152060f4f8d4ca8904094f15f65a1b6f93/dom/indexedDB/ActorsParent.cpp#17505

Comment 19

[Jan Varga [:janv]]

Could you please provide full directory listing of Data\profile\storage\default\https+++www.walmart.ca\idb ?
Thank you.

Comment 20

[Sergey Olefir]

Right now:
23.10.2018  18:12    <DIR>          3121711575wBaDltmra.files
20.11.2018  22:38            49 152 3121711575wBaDltmra.sqlite

I cannot provide the listing at the time that the storage was broken, sorry.

Comment 21

[Jan Varga [:janv]]

I just wanted to know if there was also:
3444488154t5e1s9t6-203.93458848.sqlite
besides:
3444488154t5e1s9t6-203.93458848.files

Comment 22

[Sergey Olefir]

(In reply to Jan Varga [:janv] from comment #21)
> I just wanted to know if there was also:
> 3444488154t5e1s9t6-203.93458848.sqlite
> besides:
> 3444488154t5e1s9t6-203.93458848.files

Can't say, sorry.

Comment 23

[Rares Doghi, Desktop QA]

Hi Tom, Yes After installing the Addon and created the "Test" Style from the manage section, you'll need to reach the Profile Folder, you can do that from the About:support, after which you reach the Storage folder > default.

In this default folder you have to manually create a folder with the name https+++www.walmart.ca and inside it the idb folder with the 3444488154t5e1s9t6-203.93458848.files folder inside of it. I can still reproduce this issue this way in Nightly 65.0a1 (2018-12-02)

After creating these 3 folders youll have a path like this: profile\storage\default\https+++www.walmart.ca\idb\3444488154t5e1s9t6-203.93458848.files\ and when you Reach the Manage section of the Stylus Addon the previously created style is no longer displayed.

Comment 24

[Tom Tung [:tt, :ttung]]

(In reply to Rares Doghi from comment #23)
> After creating these 3 folders youll have a path like this:
> profile\storage\default\https+++www.walmart.ca\idb\3444488154t5e1s9t6-203.
> 93458848.files\ and when you Reach the Manage section of the Stylus Addon
> the previously created style is no longer displayed.

I see, thanks! So, the initialization of IndexedDB is really strict, it only allows a folder that have a corresponding database. If not, then the initialization just fails.

(In reply to Tom Tung [:tt, :ttung] from comment #9)
> As far as I know the unexpected things from the perspective of
> IndexedDB/QuotaManager in Firefox, the name of folder like
> "3444488154t5e1s9t6-203.93458848.files" shouldn't exist. The expected name

Correcting myself, actually, this kinds of folder's name can be generated. There is no any name limitation for the database and the folder.

So, the only unclear thing here is why indexedDB can have a folder insides the origin idb folder, but without an SQLite database to accompany with. However, it seems that in extreme case like if it somehow fails unexpectedly before constructing the database [1], the storage initialization would fail every time after that.

I guess the correct thing to do here is to ignore but warn the folder without a corresponding database.

[1] https://searchfox.org/mozilla-central/source/dom/indexedDB/ActorsParent.cpp#19152

Comment 25

[Tom Tung [:tt, :ttung]]

Attachment: bug1504535.patch

Jan, although I haven't figure out why the reporter had a directory without the corresponding SQLite database, could you help me to check if it's possible to allow but warn there is an unexpected directory in an indexedDB directory during initialization? 

The mitigation that came to my mind was to allow but warn an unexpected directory in an indexedDB directory. Besides, I'm thinking to delete them in the storage upgrades like what the quota manager does for the obsolete origins. The downsides I can imagine are:
- It might increase chances for failing to open a new database because the unexpected folder name might collide the name of the opening database.
- It makes us harder to catch the "unexpected directories" problems in indexedDB because users would probably don't aware them.

However, they seem not to be cases to block us from doing this. Please let me know if there is a case that I ignore. Thanks!

Comment 26

[Jan Varga [:janv]]

Comment on attachment 9029717 [details] [diff] [review]
bug1504535.patch

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

I'm not sure if we can do this without more thinking, what if there are files under this directory ?
GetUsageForOrigin will iterate over those files and add them to calculated usage for given origin.

Anyway, I think I know how this could happen. Take a look at:
DeleteDatabaseOp::VersionChangeOp::RunOnIOThread
the .files directory is removed in last step. So if something faild before that, we could end up with the .files directory w/o a database

In future, we could implement atomic delete, that would prevent a problem like this.

For now, we have to figure out what to do with the .files directory, I think we can't just continue in initialization.
Maybe we should delete it.

Comment 27

[Jan Varga [:janv]]

Also, what if the same database is created again ?
If there are any files in the .files directory, it could cause problems.

Comment 28

[Jan Varga [:janv]]

(In reply to Tom Tung [:tt, :ttung] from comment #25)
> - It might increase chances for failing to open a new database because the
> unexpected folder name might collide the name of the opening database.

Yes, that's why we can't just ignore the unexpected .files directory.

Comment 29

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #26)
Thanks for pointing out the place I miss! I expected the GetUsageForOrigin uses the hashtable like InitOrigin [1] so that it can only calculate the expected directories on the list.

[1] https://searchfox.org/mozilla-central/rev/3fdc51e66c9487b39220ad58dcee275fca070ccd/dom/indexedDB/ActorsParent.cpp#15517 

> I'm not sure if we can do this without more thinking, what if there are
> files under this directory ?
> GetUsageForOrigin will iterate over those files and add them to calculated
> usage for given origin.
> 
> Anyway, I think I know how this could happen. Take a look at:
> DeleteDatabaseOp::VersionChangeOp::RunOnIOThread
> the .files directory is removed in last step. So if something faild before
> that, we could end up with the .files directory w/o a database

Will take a look into that. Thanks again!

> In future, we could implement atomic delete, that would prevent a problem
> like this.
> 
> For now, we have to figure out what to do with the .files directory, I think
> we can't just continue in initialization.
> Maybe we should delete it.

I thought deleting might be too aggressive, but it looks like the only appropriate way that preventing from calculating sizes of these unexpected directories. I guess we could also avoid that by storing the databaseFilenames and maintaining the table when opening and deleting databases. However, considering about hundreds of idb directories in a profile is possible, it's not a good idea to keep that hashtable somewhere as long as the lifetime of the Firefox.

Will think about if deleting is okay a bit more.

Comment 30

[Jan Varga [:janv]]

If there's just a .files directory and no .sqlite file, there's no way to recover the database.

Comment 31

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #30)
> If there's just a .files directory and no .sqlite file, there's no way to
> recover the database.

That's why I am curious about why it deletes sqlite database before the directory in DeleteDatabaseOp::VersionChangeOp::RunOnIOThread(). I'll check if it's okay to remove the directory before the sqlite database here as well.

Comment 32

[Sergey Olefir]

(In reply to Jan Varga [:janv] from comment #30)
> If there's just a .files directory and no .sqlite file, there's no way to
> recover the database.

I am not sure that the database is actually 'destroyed'. Might it be possible that these filenames change? I.e. at some point it was directoryA and then for whatever reason it became directoryB, but again for unknown reason it failed to completely delete directoryA (so deleted the files but failed to delete the dir)? And since it can't initialize when directoryA is present, then 'everything dies'.

Comment 33

[Tom Tung [:tt, :ttung]]

Jan suggested having a marker file to indicate whether do we finish all the deletes. He also said we could do that even for QuotaManager.

Comment 34

[Tom Tung [:tt, :ttung]]

Attachment: [WIP] P1

This patch definitely needs to be revised.
It does: 
  - Remove the unexpected directories on initialization (more specifically, in InitOrigin)
  - Introduce the marker file. That file is created at the beginning of removing database files and directory and is deleted at the end of removing.
    = If the file is found when initializing, then it would remove all the corresponding database files and directory.
    = If the file is found when getting usage, then it would just ignore the file.
    = If the file is found when opening a database with the same name, it would remove all the corresponding database files and directory.
    = If the file is found when deleting a database with the same name, it would remove all the corresponding database files and directory.

To do:
  - Add telemetry probes to record how often do we have unexpected directories.
  - Write a test
  - Test the patch with current tests
  - Optimize the patch and maybe rename some functions

Comment 35

[Tom Tung [:tt, :ttung]]

Attachment: P1

Patch for having a file (marker file) to indicate whether the job for a delete operation is completed or not. If not, then a marker file is expected to exist. I tested locally, and the patch seems fine.

ToDo:
- Push to try to see if there is a situation hasn't been taken care yet
- Modify the log message
- Maybe optimize it

Comment 36

[Tom Tung [:tt, :ttung]]

Attachment: P2

Test for verifying P1

Comment 37

[Tom Tung [:tt, :ttung]]

Attachment: P3

Just removing unexpected directories during initialization. It should fix the current issue (if someone has a directory without a corresponding database file in idb directory)

Comment 38

[Tom Tung [:tt, :ttung]]

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

Comment 39

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1504535 - P1 - Have a file to check if the previous removing has completed or not;

After this patch, in the beginning of each removing for a database, a marker
file will be created and then will be deleted after completing the remove. If
somehow the remove fails, we can know that by checking the file in the next
open.
Note that this patch intends to have a lazy clean-up if there is a failed
remove. Which means that the marker files are checked only when initialization,
and opening the same database again.

Comment 40

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1504535 - P2 - A test for verifying the functionality of the marker file;

Depends on D16446

Comment 41

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1504535 - P3 - Fix the exsiting unexpectedly directory files without their coressponding database file;

Depends on D16447

Comment 42

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1504535 - P4 - A test for verifing idb initialization won't be blocked by unexpected directories;

Depends on D16448

Comment 43

[Tom Tung [:tt, :ttung]]

Andrew, would you mind checking the change I made from addressing your comment for P1? Especially, line 7870 and line 15792. Thanks!

interdiff: https://phabricator.services.mozilla.com/D16446?vs=51249&id=52586&whitespace=ignore-most#toc

Comment 44

[Andrew Sutherland [:asuth] (he/him)]

(Responded, looks good!)

Comment 45

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1504535 - P5 - Send a telemetry probe when failing to remove database files during initialization;

Depends on D16820

Comment 46

[Tom Tung [:tt, :ttung]]

Andrew, I rebase patches and change a few of them. Would you mind taking one more look? Thanks!

Since the bug for supporting async/await on IDB tests was fixed, I change the patches for test. If you don't think it's necessary, I could change them back to generator function.

Here is the short-cut for the interdiff patches.

P2 changing style to async/await
https://phabricator.services.mozilla.com/D16447?vs=52543&id=53647&whitespace=ignore-most#toc
P4 changing style to async/await
https://phabricator.services.mozilla.com/D16820?vs=52545&id=53650&whitespace=ignore-most#toc

Also for P3, I update the telemetry to external error both IDB_GetEntry and IDB_GetBaseFilename since P3 removes unexpected directory when finding it at first.
https://phabricator.services.mozilla.com/D16448?vs=52544&id=53648&whitespace=ignore-most#toc

Comment 47

[Tom Tung [:tt, :ttung]]

In the team meeting, we decided to land patches in 67 because it might be problematic when backing out.

Comment 48

[Tom Tung [:tt, :ttung]]

Attachment: request-bug1504535.md

Hi chutten, we want to check one more external error for initialization. The implementation is P5. Would you mind reviewing it? Thanks!

Comment 49

[Tom Tung [:tt, :ttung]]

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

Comment 50

[Chris H-C :chutten]

Comment on attachment 9038807 [details]
request-bug1504535.md

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 Telemetry so is documented in its definitions file ([Histograms.json](https://hg.mozilla.org/mozilla-central/file/tip/toolkit/components/telemetry/Histograms.json)), the [Probe Dictionary](https://telemetry.mozilla.org/probe-dictionary/), and on telemetry.mozilla.org's [Measurement Dashboards](https://telemetry.mozilla.org/new-pipeline/dist.html).

    Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.

    If the request is for permanent data collection, is there someone who will monitor the data over time?

N/A. This collection expires in Firefox 70

    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 pre-release channels only. (Specifically nightly-only by feature pref)

    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?

Yes. :tt will need to remove or renew this collection before it expires in Firefox 70.

---
Result: datareview+

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/beb12bda21cb
P1 - Have a file to check if the previous removing has completed or not; r=asuth
https://hg.mozilla.org/integration/autoland/rev/a1dc7e61497c
P2 - A test for verifying the functionality of the marker file; r=asuth
https://hg.mozilla.org/integration/autoland/rev/028a1ddd6052
P3 - Fix the exsiting unexpectedly directory files without their coressponding database file; r=asuth
https://hg.mozilla.org/integration/autoland/rev/2cfab2d01692
P4 - A test for verifing idb initialization won't be blocked by unexpected directories; r=asuth
https://hg.mozilla.org/integration/autoland/rev/7418083f19a8
P5 - Send a telemetry probe when failing to remove database files during initialization; r=asuth

Comment 52

[Tom Tung [:tt, :ttung]]

Last 7 days, we got 2.42M samples for failing to get IDB entries [1]. The number should drop down after applying P3.

[1] https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2019-01-28&include_spill=0&keys=internal!external!__none__!__none__&max_channel_version=nightly%252F66&measure=QM_INIT_TELEMETRY_ERROR&min_channel_version=nightly%252F63&os=Windows_NT%252C6.3!Windows_NT%252C6.1!Windows_NT%252C10.0!Windows_NT%252C6.2!Windows_NT%252C6.0!Windows_NT%252C5.1&processType=parent&product=Firefox&sanitize=1&sort_by_value=0&sort_keys=submissions&start_date=2019-01-22&table=0&trim=1&use_submission_date=0

Comment 53

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/beb12bda21cb
https://hg.mozilla.org/mozilla-central/rev/a1dc7e61497c
https://hg.mozilla.org/mozilla-central/rev/028a1ddd6052
https://hg.mozilla.org/mozilla-central/rev/2cfab2d01692
https://hg.mozilla.org/mozilla-central/rev/7418083f19a8

Comment 54

[Julien Cristau [:jcristau] (back August 18)]

Guessing this is not an uplift candidate. Feel free to reset 66 status and request approval if I got this wrong.