1423917 - Existing IndexedDB files can completely break IndexedDB on upgrade from Firefox 56 to 57

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, defect, P2)

Description

[Ian Moody [:Kwan] (UTC+1)]

Attachment: The two responsible folders zipped up

I tried to upgrade to Firefox 57 yesterday, only to discover I'd lost lots of data in the form of userscripts and userstyles, and some extensions ceased working entirely, because indexedDB was completely broken for web extensions.  Fortunately not only had I backed up my profile beforehand, but thanks to some help from :rpl on IRC today I was able to discover that deleting two folders from <profileDir>/storage/default was enough to fix it.  It also turns out that web indexedDB was broken as well, by testing on https://mdn.mozillademos.org/de/docs/IndexedDB/IndexedDB_verwenden$samples/Full_IndexedDB_example
The two culprit folders were
chrome++++content+browser.xul
and
chrome++++content+toolbar+html+element-ancestors.html

and the browser console contained these relevant logs:
13:33:26.543 Quota 'chrome' is not a valid scheme!: ActorsParent.cpp:8300  (unknown)
13:33:26.543 Quota Origin 'chrome++++content+browser.xul' failed to parse, handled tokens: : ActorsParent.cpp:8218  (unknown)
13:33:26.544 IndexedDB UnknownErr: ActorsParent.cpp:598  (unknown)

(if one deleted just the folder mentioned in the logs, the other would then be mentioned instead)

Non-definitive list of extensions affected:
Greasemonkey (all userscripts gone)
Stylus (all userstyles gone, unable to create/import more)
New Tab Tools (error page with "New Tab Tools couldn't start properly. Try these:" message)

Comment 1

[Ian Moody [:Kwan] (UTC+1)]

Regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7f1714325cb3e40427e40031a2191fe20cdc3241&tochange=6aa1cc819058deaa13f85278e2f5970b8ced0998

Regressed by: Tom Tung — Bug 1290481 - r=bkelly,janv

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Andrew, any ideas here?

Comment 3

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

This looks like the same problem :overholt reported to me and I had on my to-do list to file and investigate.  In his case, the origin was 'chrome++++content+preferences+permissions.xul'.  In all cases, we're dealing with empty Cache API storage opened by chrome pages.  It's not clear what caused these pages to try and use the cache API... I would suspect some type of 3rd party library doing feature detection, or possibly the side-effect of using devtools in a chrome context trying to populate the storage tab through the page or a devtools extension doing the same thing, or something similar.

We've made this somewhat more awkward by a well-intentioned nightly-only (and formerly aurora-only) fix https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/dom/quota/ActorsParent.cpp#2042 that will refuse to create origins that don't round-trip and generate an error.  Since it's likely no one intended to use the Cache API in those cases, no one is also likely to notice the errors.  Especially if something like devtools is trained to compensate for the errors already.

In both the zips here and :overholt's provided sample, the problem only manifested for the Cache API.  IDB does do some things a little bit differently when handling the origin and principal, such as setting the persistence type to PERSISTENT for the system principal, and explicitly calling QuotaManager::GetInfoForChrome instead of calling QuotaManager::GetInfoFromPrincipal, but the latter will call the former if it encounters the system principal.

It's possible there are gaps related to QM's treatment of the origin "chrome" as the same as the system principal and the existence of the "chrome" protocol.  There's a check at https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/dom/quota/ActorsParent.cpp#5430 that forbids the use of the origin "chrome" without having the system principal, but the only way to have that origin is to have had the system principal in the first place.  I guess I'm not totally sure of the intent there.

;janv, does anything jump out at you about all of this?

Comment 4

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

I should also say, maybe it's possible that the page had somehow dropped the system principal?  It's a bit confusing...

Ian, do you know if you ever would have opened the browser devtools on a chrome privileged page and/or have tried to browse to the URL "chrome://browser/content/browser.xul" or have any suspicions of how something might have tried to use the DOM Cache API in those origins?


In the meantime, maybe we should consider a MOZ_DIAGNOSTIC_ASSERT for nightly-only.

Mitigation-wise, I think we need to teach the origin parser about the chrome protocol and have it flag the origin for nuking on upgrade.  It so happens that we also want to nuke some garbage origins based on their originattributes, so we can get some good synergy there.

Regresson-wise, I'm not quite sure what the regression test function was, but I doubt this was a product of the semantics of Tom's changes other than that they initially induced a major schema upgrade.  (And after my hack fix, induced a minor schema upgrade, but when bisecting, the bisection would potentially only be seeing the major schema upgrade.)  Although, Tom, obviously if you think your changes made us more likely to try and initialize Cache API storage for an origin, please do speak up! ;)

Comment 5

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Andrew Sutherland [:asuth] from comment #3)
> type of 3rd party library doing feature detection, or possibly the
> side-effect of using devtools in a chrome context trying to populate the
> storage tab through the page

Bingo:

STR:
1) Open Fx56 with a fresh profile
2) Open DevTools
3) In DevTools settings enable Browser Toolbox and Remote debugging
3) Open the Browser Toolbox
4) Go to the Storage tab (this creates chrome++++content+browser.xul in storage/default)
5) Close Fx56
6) Open Fx57+ with the profile from step 1
7) IndexedDB is now busted

Repeating the steps in 57 instead of 56 (so chrome++++content+browser.xul is created by 57) doesn't exhibit the same problem.

(In reply to Andrew Sutherland [:asuth] from comment #4)
> Ian, do you know if you ever would have opened the browser devtools on a
> chrome privileged page and/or have tried to browse to the URL
> "chrome://browser/content/browser.xul" or have any suspicions of how
> something might have tried to use the DOM Cache API in those origins?

I use the browser toolbox a fair bit, and could easily have opened the storage tab accidentally at some point, and think I probably actually did so deliberately only recently.

> Regresson-wise, I'm not quite sure what the regression test function was,

Run mozregression with an existing profile with the known bad storage/default/chrome++++content+browser.xul etc. and New Tab Tools installed, and open a new tab.  Either the page thumbnails showed (good), or New Tab Tools error page showed (bad).

Comment 6

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

Thank you *so* much for the Steps To Reproduce!  (And for reporting the bug!  This is an area of brittle-ness in the code-base we are working to improve in the near future.)  Can you clarify what type of build the 57 build was?  Specifically, was it a release, beta, or nightly build?

Regression-wise it sounds like it's the upgrade that's the trigger.  And that's consistent with the code, we only try and parse the origin on upgrades or when restoring metadata, per https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla3dom5quota12_GLOBAL__N_122StorageDirectoryHelper11OriginProps4InitEP7nsIFile&redirect=false.

Comment 7

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Andrew Sutherland [:asuth] from comment #6)
> Thank you *so* much for the Steps To Reproduce!  (And for reporting the bug!
> This is an area of brittle-ness in the code-base we are working to improve
> in the near future.)  Can you clarify what type of build the 57 build was? 
> Specifically, was it a release, beta, or nightly build?
> 
> Regression-wise it sounds like it's the upgrade that's the trigger.  And
> that's consistent with the code, we only try and parse the origin on
> upgrades or when restoring metadata, per
> https://searchfox.org/mozilla-central/search?q=symbol:
> _ZN7mozilla3dom5quota12_GLOBAL__N_122StorageDirectoryHelper11OriginProps4Init
> EP7nsIFile&redirect=false.
The one I used for testing just now to create the STR was a beta build, specifically 57.0b9 downloaded from mozilla servers with build ID 20171016185129 built from https://hg.mozilla.org/releases/mozilla-beta/rev/4b93f26c85540a41eeef07e1116770e47b78e104, because that is the one I already had hanging around in my downloads directory for whatever reason (and it didn't occur to me to do $ firefox instead of $ ./firefox when I was already doing the latter for 56). I could re-run with the final release if you'd like.
The bug was originally observed in release build 57.0.1+build2-0ubuntu0.17.04.1 from the official ubuntu repo.

Comment 8

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #4)
> Mitigation-wise, I think we need to teach the origin parser about the chrome
> protocol and have it flag the origin for nuking on upgrade.  It so happens
> that we also want to nuke some garbage origins based on their
> originattributes, so we can get some good synergy there.

Yes, that's what we need to do, something similar happened in the past and we had to teach the origin parser to support it.

> 
> Regresson-wise, I'm not quite sure what the regression test function was,
> but I doubt this was a product of the semantics of Tom's changes other than
> that they initially induced a major schema upgrade.  (And after my hack fix,
> induced a minor schema upgrade, but when bisecting, the bisection would
> potentially only be seeing the major schema upgrade.)  Although, Tom,
> obviously if you think your changes made us more likely to try and
> initialize Cache API storage for an origin, please do speak up! ;)

Hm, can we just be more aggressive in EnsureOriginDirectory and do the check in all builds, not just RELEASE_OR_BETA ?

Comment 9

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

(In reply to Jan Varga [:janv] from comment #8)
> Hm, can we just be more aggressive in EnsureOriginDirectory and do the check
> in all builds, not just RELEASE_OR_BETA ?

Yes, I think that would be preferable.  Per https://bugzilla.mozilla.org/show_bug.cgi?id=1286914#c17 I think the original guard was due to overhead concerns.

Relatedly, it looks like we want to make the parser return ObsoleteOrigin/eObsolete, since we already have logic that explicitly removes such origins thanks to our previous efforts to clean up "app" origins, as the logic at https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/dom/quota/ActorsParent.cpp#8265 does.

1: the removal logic is: https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/dom/quota/ActorsParent.cpp#7988
and the logic that uses them is just a teeny ways down at https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/dom/quota/ActorsParent.cpp#8032

Comment 10

[Tom Tung [:tt, :ttung]]

(In reply to Andrew Sutherland [:asuth] from comment #4)
> Regresson-wise, I'm not quite sure what the regression test function was,
> but I doubt this was a product of the semantics of Tom's changes other than
> that they initially induced a major schema upgrade.  (And after my hack fix,
> induced a minor schema upgrade, but when bisecting, the bisection would
> potentially only be seeing the major schema upgrade.)  Although, Tom,
> obviously if you think your changes made us more likely to try and
> initialize Cache API storage for an origin, please do speak up! ;)

My patches shouldn't initialize Cache API storage for that origin, but I may know why it's initilaized.

(In reply to Ian Moody [:Kwan] from comment #5)
> Bingo:
> 
> STR:
> 1) Open Fx56 with a fresh profile
> 2) Open DevTools
> 3) In DevTools settings enable Browser Toolbox and Remote debugging
> 3) Open the Browser Toolbox
> 4) Go to the Storage tab (this creates chrome++++content+browser.xul in
> storage/default)

Is it Storage Inspector? If it is, then it use "caches.keys()" to check the usage of Cache API for that origin. And, it can explain why the Cache API for that origin is initialized. I filed bug 1401482 for creating cache directory unexpectedly if the origin have never used cache API before.

Comment 11

[Tom Tung [:tt, :ttung]]

I'd like to take this bug since Andrew doesn't have time to fix this now. :p

Comment 12

[Tom Tung [:tt, :ttung]]

Jan suggested me that I might need a hash table for improving the performance after removing the #ifndef and #endif.

Comment 13

[Andrew Overholt [:overholt]]

This is pretty bad but i doubt we can get a 57 dot release out (in <= 20 days no less) for this so I'll mark it wontfix for 57.

Comment 14

[Tom Tung [:tt, :ttung]]

Jan, I found out our browser toolbox creates another parent process while I was implementing a hashtable for recording pared origins/directories. It launches another Firefox, creates another QuotaManager and makes it live in another parent process. However, since the js code for storage inspector access the profile for getting storage under the chrome origin (which is chrome://browser/content/browser.xul). That makes that QuotaManager(for the toolbox) be initialized and traverse all files under the profile. I'm afraid that it increases the chance of accessing the same file at the same time between two different processes.

So, my question is:
- Is it a known issue for QuotaManager? I mean two different processes may access a file at the same time.
- Do we have anything method to avoid this? I reckon at least we may consider avoiding the storage inspector on the toolbox to accessing the profile when we open it.

Comment 15

[Jan Varga [:janv]]

What, another parent process with QuotaManager ?!
That sounds terrible.

We should stop doing that or implement an inter-process locking which also sounds terrible.

Comment 16

[Jan Varga [:janv]]

How in the world can actually two separate Gecko instances share the same profile ?

Comment 17

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #16)
> How in the world can actually two separate Gecko instances share the same
> profile ?

Sorry for the confusion, I recheck the whole path and the profile are actually different. 

m-c/obj-x86_64-apple-darwin16.6.0/tmp/scratch_user/storage/permanent/chrome                           for the Original Nightly
m-c/obj-x86_64-apple-darwin16.6.0/tmp/scratch_user/chrome_debugger_profile/storage/permanent/chrome   for the browser toolbox

But, it does make two different QuotaManagers live in two different parent processes at the same time.

Comment 18

[Jan Varga [:janv]]

(In reply to Tom Tung [:tt] from comment #17)
> But, it does make two different QuotaManagers live in two different parent
> processes at the same time.

That shouldn't be a problem if they use two different storage directories.

Comment 19

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P1: Expose the check for ensuring the new origin is supported by the OriginParser to Beta and Release. r?janv

Comment 20

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P2: Test to verify whether QuotaManager remove the unexpected chrome origin during upgrading. r?janv

Comment 21

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P3: Make originParser recognize the chrome origin and treat them as an obsolete origin except "chrome". r?janv

Comment 22

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P2: Test to verify whether QuotaManager remove the unexpected chrome origin during upgrading. r?janv

Fix adding extra empty lines to align the coding format

Comment 23

[Tom Tung [:tt, :ttung]]

Comment on attachment 8940647 [details] [diff] [review]
Bug 1423917 - P1: Expose the check for ensuring the new origin is supported by the OriginParser to Beta and Release. r?janv

Hi Jan,

After rethinking this issue, I reckon the minor upgrade in this bug is not necessary. This issue happens due to a browser toolbox create a Cache storage with an unparsed'able chrome origin. And it makes the QuotaManager fail to upgrade from 2_0 to 2_1. Consequently, users are not able to use the quota clients under the QuotaManager.

Anyway, we can divide users into four groups by two different perspectives (whether having upgraded to version 2_1 or not & whether having these unparsed'able chrome origins). I present them below:
                                | have not upgraded yet         | have upgraded
---------------------------------------------------------------------------
don't have these chrome origins |      group 1, ok              | group 2, ok 
---------------------------------------------------------------------------
have these chrome origins       |      group 3, should be fixed | group 4, bad

The group 3 should be fixed here, because they cannot use any quota clients anymore.

If we apply the check to every channel and mark those origins to be obsolete, we can stop increasing the number of unparsed's origins in group 4. Becasue of makring those origins to be obsolete, users in group 3 will be able to be upgraded from 2_0 to 2_1, which will remove obsolete origins and their firefox will be fixed. 

However, we can consider whether to fix group 4 here or not since the users in group 4 still can access quota clients. The only concern behind is the size of the unparse'able chrome origins may be increased since they have been stored (having a directory) and thus they will escape from the check for "whether the origin is able to be parsed". But I reckon this isn't a serious problem because the general website won't be a chrome origin. Besides, we might have another minor upgrade for fixing the eTld+1 mismatch issue few weeks later (hope so :)). Given these reasons and having an upgrade might increase many lines of codes, I think perphs we can consdier not having a minor upgrade in this bugs. 

This patch is to apply the check to Beta and Release. Would you mind reviewing it? Thanks!

Comment 24

[Tom Tung [:tt, :ttung]]

Comment on attachment 8940652 [details] [diff] [review]
Bug 1423917 - P2: Test to verify whether QuotaManager remove the unexpected chrome origin during upgrading. r?janv

This patch is to have a xpcshell test to ensure the group 3 (have not been upgraded to 2_1 and have a unexpected chrome storage) won't ruin the upgrade from 2_0 to 2_1 and unparsed'able storages should be removed after the upgrade.

Comment 25

[Tom Tung [:tt, :ttung]]

Comment on attachment 8940651 [details] [diff] [review]
Bug 1423917 - P3: Make originParser recognize the chrome origin and treat them as an obsolete origin except "chrome". r?janv

This patch is a bit complicated, but, basically, it does things as follows:
- Create a hashtable mParsedOrigins for reducing re-parse the same origins
- Move and rename the local function EnsureOriginDirectory() to QuotaManager::EnsureOriginIsParsedable() since it need to access the hashtable held by QuotaManager
- Make OriginParser understand chrome origin and mark them as obsolete origin except "chrome" origin

After applying this patch, we can pass the test in P2. Could you help me to review this patch? Thanks!

Comment 26

[Tom Tung [:tt, :ttung]]

Comment on attachment 8940652 [details] [diff] [review]
Bug 1423917 - P2: Test to verify whether QuotaManager remove the unexpected chrome origin during upgrading. r?janv

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

Sorry, I found I forgot to attach the zip file. I'll re-send the review request again later

Comment 27

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P2: Test to verify whether QuotaManager remove the unexpected chrome origin during upgrading. r?janv

Update the test.

Comment 28

[Marion Daly [:mdaly]]

It's almost there, just needs to be brought to finish. Reassigned.

Comment 29

[Marion Daly [:mdaly]]

Hey Andrew, want to take a look at this?

Comment 30

[Jan Varga [:janv]]

Tom, feel free to take this bug again.
It should be part of the project for addressing all storage initialization issues.
You can add dependencies once we have a bug/wiki for that.

Comment 31

[Tom Tung [:tt, :ttung]]

Andrew, please feel free to take this bug back. I assume that you are fine with me for taking this bug. :)

I'll start with checking all the patches in this bug again.

Comment 32

[Jan Varga [:janv]]

Comment on attachment 8940651 [details] [diff] [review]
Bug 1423917 - P3: Make originParser recognize the chrome origin and treat them as an obsolete origin except "chrome". r?janv

I guess these patches need to be refreshed.

Comment 33

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P2: Test to verify whether QuotaManager remove the unexpected chrome origin during upgrading. r?janv

Hi Jan,

I checked this patch and it's ready right now. This patch is mainly to add a test to verify an outdated profile with an unexpected chrome origin is able to be updated to the next version.  

In this test, the unexcepted chrome origin is expected to be removed. Note that without P3, this test should fail.

Comment 34

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P3: Make originParser recognize the chrome origin and treat them as an obsolete origin except "chrome". r?janv

I rebased this patch and changed a couple of naming to make it a bit readable.

So, this patch is to make our OriginPaser be able to read chrome origin. Only chrome origin named "chrome" is valid, and the rest of them (for instance: chrome://xxx) are invalid. The invalid origins are not allowed to create their directory and will be removed if they were somehow accidentally created. Also, this patch adds a table to track all parsed origin string, so that we don't need to parse a parsed origin again. Note that with this patch, test in P2 should pass.

Comment 35

[Jan Varga [:janv]]

I created a new meta bug for fixing all storage initialization issues, it's bug 1482662.

The patches here fix the origin parser to support chrome uris and other stuff which is fine, but I think we need to do more:

1. Check what happens when the .metadata-v2 is being restored for chrome++++content+browser.xul origin directory, especially during storage initialization.
I'm a bit worried that the whole origin directory is just deleted in this case, .metadata-v2 is not restored and then storage initialization just fails. I didn't test it, but by looking at the code, I think this may happen.

2. Consider to introduce a minor storage upgrade that would delete obsolete origin directories.
I think we need to fix bug 1395102 first, then we should create a new helper for upgrading storage to 2.2 (from the current 2.1 version).
We should probably collect all similar storage initialization failures that need origin parser fixing and which result in having new obsolete origins. So we can delete them all in one storage upgrade.
Doing a minor upgrade shouldn't break users using the same profile for multiple FF branches.
On the other hand, it can happen that some of those invalid origin directories appear again while running an older version. However once the user switches to a version that contains storage initialization fixes, stuff should start working again and invalid origins will be just ignored (not deleted again because storage version is already set to 2.2, they will get deleted at next storage upgrade).
I also think that :asuth should review the storage version change from 2.1 to 2.2, it should be ok, but he implemented the hack for renaming 3.0 to 2.1, so I guess he should also take look once we have patches ready for landing.

Anyway, one thing we could do in the meantime without waiting for bug 1395102 and new storage upgrade.
We want to prevent creation of origin directories that are not supported by our origin parser ASAP.
We already do it, but only when running a nightly. The patch P3 contains changes required for doing the check on beta and release, so I suggest to extract those changes into a new bug, make it block bug 1482662.

Given these comments/suggestions, I'm going to clear the review requests.

Comment 36

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #35)
> 2. Consider to introduce a minor storage upgrade that would delete obsolete
> origin directories.

Side note:
- We might also probe to check how many origins are broken here as a one-time diagnosis.
- We might consider making Cache do a bit recovering mechanism (e.g. re-setting padding files or adding the padding column back for the SQLite file if they were broken)

Comment 37

[Robert AB]

See also: bug 944918, bug 1415792

Comment 38

[Tom Tung [:tt, :ttung]]

Comment on attachment 8940647 [details] [diff] [review]
Bug 1423917 - P1: Expose the check for ensuring the new origin is supported by the OriginParser to Beta and Release. r?janv

This patch has landed in another bug.

Comment 39

[Tom Tung [:tt, :ttung]]

Attachment: P1 - white list dot files (.*) and desktop.ini

Comment 40

[Tom Tung [:tt, :ttung]]

Attachment: P2 - Introduce chrome protocol to origin parser, but only allow "chrome" spec

Comment 41

[Tom Tung [:tt, :ttung]]

Attachment: P3 - A test to verify the storage upgrade does't fail if there is a existing chrome++++content+browser.xul directory

Comment 42

[Tom Tung [:tt, :ttung]]

Attachment: P4 - Bypass invalid files during QM initialization

Comment 43

[Tom Tung [:tt, :ttung]]

Attachment: [WIP] P5 - Introduce a new minor upgrade and have a one time daignosis to probe how many origin in idb and cache are broken

Still WIP, but the main concern so far is that accessing the database files in cache or idb directories might slow down the time for storage initialization. However, it seems to be the only way to know whether origins in cache or idb are broken or not if I want to do that in the upgrade.

Still, need to consider more:
- Consider recovering broken cache profile in this upgrade
- This upgrade might need to be cancelable because it might take a lot of time
- Not sure should I replace the metadata data files by storage.sqlite in this upgrade

Will do, but probably in another patch
- Queue another task in QM if there is a failure during the minor upgrade to avoid blocking foreground task too long.

Comment 44

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #43)
> Still WIP, but the main concern so far is that accessing the database files
> in cache or idb directories might slow down the time for storage
> initialization. However, it seems to be the only way to know whether origins
> in cache or idb are broken or not if I want to do that in the upgrade.

Not sure whether it's too aggressive. We could just initialize the origin (probably not initialize the database) instead. Besides, maybe just add probes when failing to upgrade the database schema to the newest version would be okay enough.

Comment 45

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

Comment on attachment 9012063 [details] [diff] [review]
P1 - white list dot files (.*) and desktop.ini

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

::: dom/quota/ActorsParent.cpp
@@ +1473,5 @@
>  IsOSMetadata(const nsAString& aFileName)
>  {
> +  return aFileName.EqualsLiteral(DSSTORE_FILE_NAME) ||
> +         aFileName.EqualsLiteral(DESKTOP_FILE_NAME) ||
> +         aFileName.EqualsLiteral(DESKTOP_INI_FILE_NAME);

Let's also add "Thumbs.db" that was raised in https://bugzilla.mozilla.org/show_bug.cgi?id=1493262#c11

Comment 46

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

Comment on attachment 9012063 [details] [diff] [review]
P1 - white list dot files (.*) and desktop.ini

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

::: dom/quota/QuotaCommon.h
@@ +21,5 @@
>    using namespace mozilla::dom::quota;
>  
>  #define DSSTORE_FILE_NAME ".DS_Store"
> +#define DESKTOP_FILE_NAME ".desktop"
> +#define DESKTOP_INI_FILE_NAME "desktop.ini"

I believe this is actually "Desktop.ini" so unless we do a case-insensitive check, this won't accomplish what we want.

Comment 47

[Will Elwood (:Will)]

(In reply to Andrew Sutherland [:asuth] from comment #46)
> I believe this is actually "Desktop.ini" so unless we do a case-insensitive
> check, this won't accomplish what we want.

"desktop.ini" is all lower-case on my Win8.1 system, but case-insensitivity for files on a Windows system in general would be good.

Comment 48

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

(In reply to Will Elwood (:Will) from comment #47)
> (In reply to Andrew Sutherland [:asuth] from comment #46)
> > I believe this is actually "Desktop.ini" so unless we do a case-insensitive
> > check, this won't accomplish what we want.
> 
> "desktop.ini" is all lower-case on my Win8.1 system, but case-insensitivity
> for files on a Windows system in general would be good.

Thanks very much for replying and the data-point!  Indeed, it turns out on my Win10 system I have both "desktop.ini" and "Desktop.ini", so for sure we do need to accept both!

Comment 49

[Tom Tung [:tt, :ttung]]

Attachment: P1 - white list dot files (.*) and desktop.ini

(In reply to Andrew Sutherland [:asuth] from comment #46)
(In reply to Will Elwood (:Will) from comment #47)

Thank you both for the information and the feedback!

Comment 50

[Tom Tung [:tt, :ttung]]

Attachment: P1 (v1) whitelist dot-files, desktop.ini, Desktop.ini, Thumbs.db

Comment 51

[Tom Tung [:tt, :ttung]]

Attachment: P2 (v1) Test for p1

Comment 52

[Tom Tung [:tt, :ttung]]

Attachment: P3 (v1) Teach Origin "chrome" and obsolete other chrome origins

Comment 53

[Tom Tung [:tt, :ttung]]

Attachment: P4 (v1) test for p3 (might need to move this to another test for this upgrade instead of using the test for previous upgrade)

Comment 54

[Tom Tung [:tt, :ttung]]

Attachment: P5 (v1) Support obsoleting the moz-safe-about+++home

Comment 55

[Tom Tung [:tt, :ttung]]

Attachment: P6 (v1) Ignore invalid origins during upgrades

Comment 56

[Tom Tung [:tt, :ttung]]

Attachment: P7 (v1) Have a minor upgrade to remove obsolete origins

Eventually, I gave up having a diagnosis of how many origins are broken in this upgrade. The reasons are:
- We will still track this by bug 1436188 and it's follow-up bug 1487779 (exposing those probe in Beta and Release)
- It takes almost 2x times for a Nightly build to upgrade their profile for the first time they run this upgrade. That might be horrible for some extreme users who have already taken minutes to traverse origin directories.

Consider about them, I don't think it's really valuable to do that in this upgrade but noted that the result is still important for us to know the situation of general users.

Comment 57

[Tom Tung [:tt, :ttung]]

Attachment: P4 (v1) Recognize the "moz-safe-about+++home" in OriginParser

Comment 58

[Tom Tung [:tt, :ttung]]

Attachment: P5 (v1) Ignore invalid origins during upgrades

Comment 59

[Tom Tung [:tt, :ttung]]

Attachment: P6 (v1.1) Have a minor upgrade to remove obsolete origins

Comment 60

[Tom Tung [:tt, :ttung]]

Attachment: P7 (v1.1) A test for the minor upgrade

Comment 61

[Tom Tung [:tt, :ttung]]

Comment on attachment 9019717 [details] [diff] [review]
P1 (v1) whitelist dot-files, desktop.ini, Desktop.ini, Thumbs.db

Jan this patch is putting a set of file type into QM's whitelist. For the dot files, it makes QM still generate warnings, but not break the initialization.

Comment 62

[Tom Tung [:tt, :ttung]]

Comment on attachment 9019718 [details] [diff] [review]
P2 (v1) Test for p1

A test for ensuring P1 does put these files into QM's whitelist.

Comment 63

[Tom Tung [:tt, :ttung]]

Comment on attachment 9019721 [details] [diff] [review]
P3 (v1) Teach Origin "chrome" and obsolete other chrome origins

This patch teaches OriginParser only exact "chrome" origin. The other origins that using chrome protocol will be treated as an obsolete origin.

Comment 64

[Tom Tung [:tt, :ttung]]

Comment on attachment 9021151 [details] [diff] [review]
P4 (v1) Recognize the "moz-safe-about+++home" in OriginParser

This patch teaches OriginParser that "moz-safe-about+++home" is an obsolete origin

Comment 65

[Tom Tung [:tt, :ttung]]

Attachment: P5 (v1.1) Have a minor upgrade to remove obsolete origins

Comment 66

[Tom Tung [:tt, :ttung]]

Attachment: P6 (v1.1) A test for the minor upgrade

Comment 67

[Jan Varga [:janv]]

Ok, I'll take a look, thanks.

Comment 68

[Liz Henry (:lizzard) (relman/hg->git project)]

Is this still on the radar for 65? Or should we plan to try landing/enabling for 66?

Comment 69

[Tom Tung [:tt, :ttung]]

Comment on attachment 9019717 [details] [diff] [review]
P1 (v1) whitelist dot-files, desktop.ini, Desktop.ini, Thumbs.db

Andrew would you mind looking this since Jan might be busy in LSNG? This patch is for having a whitelist of files to avoid an error return during initialization.

Comment 70

[Tom Tung [:tt, :ttung]]

Comment on attachment 9019718 [details] [diff] [review]
P2 (v1) Test for p1

This is a test for verifying P1

Comment 71

[Tom Tung [:tt, :ttung]]

Comment on attachment 9019721 [details] [diff] [review]
P3 (v1) Teach Origin "chrome" and obsolete other chrome origins

Patch for handling chrome protocol.

The concern here is that not sure whether it's fine that not finishing parsing entire chrome origin string before return it as an invalid origin.

Comment 72

[Tom Tung [:tt, :ttung]]

Comment on attachment 9021151 [details] [diff] [review]
P4 (v1) Recognize the "moz-safe-about+++home" in OriginParser

Patch for handling "moz-safe-about+++home"

Comment 73

[Jan Varga [:janv]]

Comment on attachment 9019721 [details] [diff] [review]
P3 (v1) Teach Origin "chrome" and obsolete other chrome origins

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

I had one comment for this. Currently the origin parser tries to fully construct obsolete origins and let the caller to handle it.
This patch changes that and just aborts parsing (mError=true) and still returns the ObsoleteOrigin result.
It's quite confusing and might be dangerous, because I remember some code expected that the spec is valid even when ObsoleteOrigin was returned.
I don't have the time to investigate this more right now, just wanted to

Comment 74

[Tom Tung [:tt, :ttung]]

Comment on attachment 9022118 [details] [diff] [review]
P5 (v1.1) Have a minor upgrade to remove obsolete origins

I guess I still need to take one more look before sending it to review. However, it might be great if you have time and I can getting feedback from you! Please feel free to drop it if you don't have bandwidth with it.

This patch is for the minor upgrade. Since the base class of upgrade should have already handled removing the obsolete origins, it basically traverses the directories and recover the metadata files.

Comment 75

[Tom Tung [:tt, :ttung]]

Comment on attachment 9022120 [details] [diff] [review]
P6 (v1.1) A test for the minor upgrade

Patch for verifying the minor upgrade

Comment 76

[Jan Varga [:janv]]

Andrew, you should take a look at bug 1491637 too, it's closely related.

Comment 77

[Andrew Overholt [:overholt]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #68)
> Is this still on the radar for 65? Or should we plan to try landing/enabling
> for 66?

Yes, we're going to try.

Comment 78

[Sergey Olefir]

Please also take a look at bug 1504535 -- this seems to be a closely-related problem and can happen without Firefox upgrade (possibly related to Firefox being disallowed to delete the directory because it is locked by another (backup) process).

Comment 79

[Tom Tung [:tt, :ttung]]

I guess in P1 or another patch, I need to change [1] to use the IsOSMetadata() in dom/quota/ActorsParent.cpp. Besides, the dot files should probably be ignored in indexedDB initialization as well.

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

Comment 80

[Tom Tung [:tt, :ttung]]

Unfortunately, we decided to resolve/land this in FF66

Comment 81

[Tom Tung [:tt, :ttung]]

Attachment: P1 (v1.1) whitelist dot-files, desktop.ini, Desktop.ini, Thumbs.db

Update patch due to rebasing (matching with the new coding style) and applying QuotaManager::IsOSMetadata() to dom/indexedDB/ActorsParent.cpp

Comment 82

[Tom Tung [:tt, :ttung]]

Attachment: P3 (v1.1) Teach Origin "chrome" and obsolete other chrome origins

Update for rebasing and using the "if-else" syntax rather than the "? :" syntax

Comment 83

[Tom Tung [:tt, :ttung]]

Attachment: P7 (v1.2) patch for ignoring invalid origins while upgrade

Comment 84

[Tom Tung [:tt, :ttung]]

Attachment: P1 (v1.2) whitelist dot-files, desktop.ini, Desktop.ini, Thumbs.db

Comment 85

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

Comment on attachment 9032448 [details] [diff] [review]
P1 (v1.2) whitelist dot-files, desktop.ini, Desktop.ini, Thumbs.db

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

I don't think it makes sense to report warnings if we're returning NS_OK or otherwise tolerating the existence of a file, so let's change that.

::: dom/quota/ActorsParent.cpp
@@ +1675,5 @@
>            return NS_OK;
>          }
>  
> +        if (IsDotFile(leafName)) {
> +          UNKNOWN_FILE_WARNING(leafName);

Remove the warning.

@@ +2881,5 @@
> +bool QuotaManager::IsOSMetadata(const nsAString& aFileName) {
> +  return aFileName.EqualsLiteral(DSSTORE_FILE_NAME) ||
> +         aFileName.EqualsLiteral(DESKTOP_FILE_NAME) ||
> +         aFileName.EqualsLiteral(DESKTOP_INI_CAPTAILIZATION_FILE_NAME) ||
> +         aFileName.EqualsLiteral(DESKTOP_INI_FILE_NAME) ||

Let's remove the above and instead just use LowerCaseEqualsLiteral here (for "desktop.ini").

@@ +3769,5 @@
>        if (IsOSMetadata(leafName)) {
>          continue;
>        }
>  
>        UNKNOWN_FILE_WARNING(leafName);

Let's move the warning down so we only invoke it if we're returning NS_ERROR_UNEXPECTED.

@@ +3854,5 @@
>  
>          continue;
>        }
>  
>        UNKNOWN_FILE_WARNING(leafName);

ditto: Let's move the warning down so we only invoke it if we're returning NS_ERROR_UNEXPECTED.

@@ +6562,5 @@
>  
>            continue;
>          }
>  
>          UNKNOWN_FILE_WARNING(leafName);

ditto: Let's move the warning down so we only invoke it if we're returning NS_ERROR_UNEXPECTED.

::: dom/quota/QuotaCommon.h
@@ +24,5 @@
>  #define USING_QUOTA_NAMESPACE using namespace mozilla::dom::quota;
>  
>  #define DSSTORE_FILE_NAME ".DS_Store"
> +#define DESKTOP_FILE_NAME ".desktop"
> +#define DESKTOP_INI_CAPTAILIZATION_FILE_NAME "Desktop.ini"

There's a typo in this constant, but let's just remove this constant and use case-insensitive comparison.

Comment 86

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

Comment on attachment 9019718 [details] [diff] [review]
P2 (v1) Test for p1

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

Nice test!

Comment 87

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

Comment on attachment 9029680 [details] [diff] [review]
P3 (v1.1) Teach Origin "chrome" and obsolete other chrome origins

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

Please fix the commit message to be the same as the prior P3.

Restating: "chrome" is okay as an origin but anything longer than that is not.  So the state machine will error if it sees any tokens after "chrome" and will mark the origin as Obsolete which will cause it to be cleaned up.  This is what we want because the only times Firefox ever should have had an origin like that was due to the devtools storage inspector accidentally creating the origin as a side-effect when opened on chrome-privileged pages.  Thunderbird was synthetically creating a chrome origin that had a path, but that code has already been changed, so this cleanup is good.

Comment 88

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

Comment on attachment 9019721 [details] [diff] [review]
P3 (v1) Teach Origin "chrome" and obsolete other chrome origins

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

I've r+'ed this on the updated patch, but to respond specifically to Jan's concern, I believe the potential concern is that OriginProps::Init invokes OriginParser::ParseOrigin at https://searchfox.org/mozilla-central/rev/c43240cef5829b8a2dec118faff8a5e1fec6ae1b/dom/quota/ActorsParent.cpp#7879 and saves off the spec as `mSpec`, returning NS_OK if ObsoleteOrigin is returned, and leaving `mSpec` void since ParseOrigin early returns without ever initializing the passed in `aSpec`.

Audit-wise, the following code consults OriginProps::mSpec:
- StorageOperationBase::RunOnMainThread but only if mType is eContent.  mType is eObsolete in that case.
- CreateOrUpgradeDirectoryMetadataHelper::PrepareOriginDirectory.  PrepareOriginDirectory is only called by RepositoryOperationBase::ProcessRepository if mType != Obsolete (and errors entirely if eInvalid at the time of this patch).
- CreateOrUpgradeDirectoryMetadataHelper::ProcessOriginDirectory if mPersistent.  ProcessOriginDirectory is only called by StorageOperationBase::ProcessOriginDirectories for mType != eObsolete (with eInvalid never getting that far).

So we're safe, but I would say that if we make further enhancements to the code, we should consider cleaning things up to use mfbt/Variant.h or some other mechanism so that such fields are not available when the return value is eObsolete.

Comment 89

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

Comment on attachment 9021151 [details] [diff] [review]
P4 (v1) Recognize the "moz-safe-about+++home" in OriginParser

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

I am very glad we have tests!

Comment 90

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

Comment on attachment 9022120 [details] [diff] [review]
P6 (v1.1) A test for the minor upgrade

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

Nice test!

Comment 91

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

Comment on attachment 9029689 [details] [diff] [review]
P7 (v1.2) patch for ignoring invalid origins while upgrade

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

Restating: We just pretend like invalid origin directories don't exist.  So from a correctness perspective, it's the same thing as before, except we keep going.

Comment 92

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P1 - Ignore dot files in QM and add .desktop, Desktop.ini, desktop.ini, Thumbs.db into the list of OSMetadata; r=asuth

Addressed comments

Comment 93

[Tom Tung [:tt, :ttung]]

Attachment: interdiff-p1.patch

Andrew, given the current number of failures for Ori_UnexpectedFile [1] is quite high, would you mind me for adding ignoring OS metadata files in origin directories to P1?

Note that updated P1 here is just applying your comment. I still keep P1 with this inter-diff on my local repo.

[1] https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2019-01-20&include_spill=0&keys=internal!__none__!__none__!__none__&max_channel_version=nightly%252F66&measure=QM_INIT_TELEMETRY_ERROR&min_channel_version=nightly%252F63&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2019-01-17&table=0&trim=1&use_submission_date=0

Comment 94

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P2 - Have a test to verify a set of hidden files don't block QuotaManager's initialization and getting usauge; r=asuth

Comment 95

[Tom Tung [:tt, :ttung]]

(In reply to Andrew Sutherland [:asuth] from comment #88)
Thanks for the reviews and detailed explanation!

I've r+'ed this on the updated patch, but to respond specifically to Jan's
concern, I believe the potential concern is that OriginProps::Init invokes
OriginParser::ParseOrigin at
https://searchfox.org/mozilla-central/rev/
c43240cef5829b8a2dec118faff8a5e1fec6ae1b/dom/quota/ActorsParent.cpp#7879 and
saves off the spec as mSpec, returning NS_OK if ObsoleteOrigin is
returned, and leaving mSpec void since ParseOrigin early returns without
ever initializing the passed in aSpec.

Audit-wise, the following code consults OriginProps::mSpec:

• StorageOperationBase::RunOnMainThread but only if mType is eContent.
  mType is eObsolete in that case.
• CreateOrUpgradeDirectoryMetadataHelper::PrepareOriginDirectory.
  PrepareOriginDirectory is only called by
  RepositoryOperationBase::ProcessRepository if mType != Obsolete (and errors
  entirely if eInvalid at the time of this patch).
• CreateOrUpgradeDirectoryMetadataHelper::ProcessOriginDirectory if
  mPersistent. ProcessOriginDirectory is only called by
  StorageOperationBase::ProcessOriginDirectories for mType != eObsolete (with
  eInvalid never getting that far).

So we're safe, but I would say that if we make further enhancements to the
code, we should consider cleaning things up to use mfbt/Variant.h or some
other mechanism so that such fields are not available when the return value
is eObsolete.

I see. Indeed, it's not okay to make caller take care of that. Now, I'm thinking either cleaning things up or just assigning values when it's valid. I'll figure out a way to handle that properly. Thanks!

Comment 96

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

Comment on attachment 9038214 [details] [diff] [review]
Bug 1423917 - P2 - Have a test to verify a set of hidden files don't block QuotaManager's initialization and getting usauge; r=asuth

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

Nice test!

> "adding ignoring OS metadata files in origin directories to P1"
Yes, this makes sense, thanks.

Comment 97

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P1 - Ignore dot files in QM and add .desktop, Desktop.ini, desktop.ini, Thumbs.db into the list of OSMetadata; r=asuth

Update the patch

Comment 98

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P3 - Teach the OriginParser "chrome" origin; r=asuth

Comment 99

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P4 - Support moz-safe-about://home in OriginParser and make it obsolete; r=asuth

Comment 100

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P5 - Introduce a minor upgrade on QuotaManager to clean obsolete origins; r=asuth

Comment 101

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1413917 - P6 - Having a test to verify the version2_1To2_2 upgrade removes obsolete origins and isn't blocked by an invalid origin; r=asuth

Comment 102

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P7 - Ignore invalid origin files during upgrading; r=asuth

Comment 103

[Tom Tung [:tt, :ttung]]

Comment on attachment 9019721 [details] [diff] [review]
P3 (v1) Teach Origin "chrome" and obsolete other chrome origins

Open Bug 1522464 for dealing with this.

Comment 104

[Tom Tung [:tt, :ttung]]

Given we've had telemetry to track the number of failures for initialization and we want to reduce the times of upgrade as less as possible, I intend to push P1 and P2 first to check whether the errors of unexpected files in repositories and origins drop. If the number of failures decreases after a few days, then I will land the rest of the patches (upgrade).

Comment 105

[Tom Tung [:tt, :ttung]]

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

Comment 106

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt, :ttung] from comment #105)

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

Right now it fails on running the test_corruptedDatabase.js, and it seems the cause is related to local storage and it will be fixed in the near future. I'll disable this test when landing the patches for it per discussion on IRC with Jan.

Comment 107

[Tom Tung [:tt, :ttung]]

Plan landing P1 P2 tomorrow and will uplift these two patches to Beta after making sure they are stable. The rest of the patches won't be in the 66 and will land them next week (after checking the telemetry).

Comment 108

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1e94f601466
P1 - Ignore dot files in QM and add .desktop, Desktop.ini, desktop.ini, Thumbs.db into the list of OSMetadata; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/184071b7ccab
P2 - Have a test to verify a set of hidden files don't block QuotaManager's initialization and getting usauge; r=asuth

Comment 109

[Tom Tung [:tt, :ttung]]

Since I intend to push the code for the minor upgrade on QuotaManager later (P3~P7), marking this bug leave-open in the case of being closed accidentally. Also, if P1, P2 are stable, I plan to uplift them to the Beta.

P1 & P2 should decrease the number of failures on Ori_UnexpectedFile and Rep_UnexpectedFile. Note that based on the telemetry data in 1/22~1/28 [1], we got 404k Ori_UnexpectedFile and 143.35k Rep_UnexpectedFile failures.

[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 110

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/c1e94f601466
https://hg.mozilla.org/mozilla-central/rev/184071b7ccab

Comment 111

[Tom Tung [:tt, :ttung]]

Hi Pascal,

I intend to uplift P1 and P2 to 66 after they are stable (I guess at least one ~ two weeks more). Do you happen to know when is the last deadline for the uplifting to 66? Thanks!

Comment 112

[Pascal Chevrel:pascalc]

(In reply to Tom Tung [:tt, :ttung] from comment #111)

Hi Pascal,

I intend to uplift P1 and P2 to 66 after they are stable (I guess at least one ~ two weeks more). Do you happen to know when is the last deadline for the uplifting to 66? Thanks!

Hi Tom, I am moving the question to Liz as she is the 66 owner (I am on 67).

Comment 113

[Liz Henry (:lizzard) (relman/hg->git project)]

The earlier the better for something like this. How about aiming for beta 8?

Comment 114

[Tom Tung [:tt, :ttung]]

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #113)

The earlier the better for something like this. How about aiming for beta 8?

I check the schedule [1] and, If I don't misunderstand that, it's Feb 15. So, sure and I will send a request next Wednesday(Feb 13). Thanks!

[1] https://calendar.google.com/calendar/embed?src=bW96aWxsYS5jb21fZGJxODRhbnI5aTh0Y25taGFiYXRzdHY1Y29AZ3JvdXAuY2FsZW5kYXIuZ29vZ2xlLmNvbQ

Comment 115

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P9 - Remove asmjs client in the 2_1to2_2 upgrade for QuotaManager;

Comment 116

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P10 - Remove .tmp file in idb drectories in the 2_1to2_2 upgrade;

Comment 117

[Tom Tung [:tt, :ttung]]

Comment on attachment 9042949 [details] [diff] [review]
Bug 1423917 - P9 - Remove asmjs client in the 2_1to2_2 upgrade for QuotaManager;

Andrew, would you mind reviewing it? This patch removes asmjs directories in storage.

Luke, would you mind reviewing it as well? With this patch, I guess you still need to remove the "asmjs" entry from quota/Client.h and quota/ActorsParent.cpp, and remove asmjs related .cpp .h files in the code base, but the psychical asmjs directory in users' profiles should be removed after this upgrade.

Comment 118

[Tom Tung [:tt, :ttung]]

Comment on attachment 9042950 [details] [diff] [review]
Bug 1423917 - P10 - Remove .tmp file in idb drectories in the 2_1to2_2 upgrade;

I wanted to do that later because there is only one user report it. However, since we need to traverse origins, I guess we can do it here.

Comment 119

[Tom Tung [:tt, :ttung]]

Comment on attachment 9038794 [details] [diff] [review]
Bug 1423917 - P1 - Ignore dot files in QM and add .desktop, Desktop.ini, desktop.ini, Thumbs.db into the list of OSMetadata; r=asuth

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

None

User impact if declined

The telemetry data for initialization failure shows that P1 & P2 does reduce the number of error for finding unexpected files on repository and origin folders on Nightly Channel. If it's declined, then Beta users having this problem need to wait until 67 to fix.

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 & P2 only add our files into our allow list and bypass those files on initialization

String changes made/needed

Comment 120

[Tom Tung [:tt, :ttung]]

Comment on attachment 9038214 [details] [diff] [review]
Bug 1423917 - P2 - Have a test to verify a set of hidden files don't block QuotaManager's initialization and getting usauge; r=asuth

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

None

User impact if declined

Unit test for P1

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)

String changes made/needed

Comment 121

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt, :ttung] from comment #119)

Comment on attachment 9038794 [details] [diff] [review]
Bug 1423917 - P1 - Ignore dot files in QM and add .desktop, Desktop.ini, desktop.ini, Thumbs.db into the list of OSMetadata; r=asuth

Is this code covered by automated tests?

No

Sorry, but this should be "yes"

Comment 122

[Liz Henry (:lizzard) (relman/hg->git project)]

Are all parts of this landed in m-c?
How was this verified?

Comment 123

[Tom Tung [:tt, :ttung]]

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #122)

Are all parts of this landed in m-c?
How was this verified?

Yes, sorry for not stating that clearly. So, they are in m-c already and for 14 days without reporting any issues. Below are the rev.

(In reply to Raul Gurzau (:RaulGurzau) from comment #110)

https://hg.mozilla.org/mozilla-central/rev/c1e94f601466
https://hg.mozilla.org/mozilla-central/rev/184071b7ccab

Comment 124

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt, :ttung] from comment #123)

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #122)

Are all parts of this landed in m-c?
How was this verified?

Yes, sorry for not stating that clearly. So, they are in m-c already and for 14 days without reporting any issues. Below are the rev.

(In reply to Raul Gurzau (:RaulGurzau) from comment #110)

https://hg.mozilla.org/mozilla-central/rev/c1e94f601466
https://hg.mozilla.org/mozilla-central/rev/184071b7ccab

To be more clear, only P1 & P2 and only these two need to be uplifted

Comment 125

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9038794 [details] [diff] [review]
Bug 1423917 - P1 - Ignore dot files in QM and add .desktop, Desktop.ini, desktop.ini, Thumbs.db into the list of OSMetadata; r=asuth

Ignores some files in profile directories, should help avoid breaking profiles during update. OK for uplift to beta 8.

Comment 126

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9038214 [details] [diff] [review]
Bug 1423917 - P2 - Have a test to verify a set of hidden files don't block QuotaManager's initialization and getting usauge; r=asuth

Adds tests, OK for uplift to beta 8.

Comment 127

[Narcis Beleuzu [:NarcisB]]

There are conflicts while uplifting to beta:

grafting 523813:c1e94f601466 "Bug 1423917 - P1 - Ignore dot files in QM and add .desktop, Desktop.ini, desktop.ini, Thumbs.db into the list of OSMetadata; r=asuth"
merging dom/indexedDB/ActorsParent.cpp
warning: conflicts while merging dom/indexedDB/ActorsParent.cpp! (edit, then use 'hg resolve --mark')

:ttung , can you please provide a patch for beta?

Comment 128

[Tom Tung [:tt, :ttung]]

Attachment: P1 for beta

Here is the patch. I didn't find a conflict while importing it to Beta locally, though.

Comment 129

[Tom Tung [:tt, :ttung]]

Attachment: P2 for beta

Comment 130

[Tom Tung [:tt, :ttung]]

attachment 9043808 [details] [diff] [review] and attachment 9043809 [details] [diff] [review] are the P1 & P2. I imported them to the newest beta without conflicts.

I haven't had an experience like this, so should I ask uplift request again? If not, please let me know if they still don't work. Thank you!

Comment 131

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/4b73d443d771
https://hg.mozilla.org/releases/mozilla-beta/rev/d6e47c340828

Comment 132

[Tom Tung [:tt, :ttung]]

Patches are uplifted to beta, so I guess I can remove the ni.

Comment 133

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P3 - Teach the OriginParser "chrome" origin; r=asuth

This patch makes OriginParser only treats "chrome" origin as a valid origin.
For the rest kinds of origins with chrome schema, the OriginParser would just
treat them as obsolete origins.

Comment 134

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P4 - Support moz-safe-about://home in OriginParser and make it obsolete; r=asuth

Depends on D21727

Comment 135

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P5 - Introduce a minor upgrade on QuotaManager to clean obsolete origins; r=asuth

Depends on D21728

Comment 136

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P6 - Having a test to verify the version2_1To2_2 upgrade removes obsolete origins and isn't blocked by an invalid origin; r=asuth

Depends on D21729

Comment 137

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P7 - Ignore invalid origin files during upgrading; r=asuth

After this patch, invalid origin shouldn't block upgrade anymore. However, we
should be aware of that if an invild origin somehow can be recoverd to a valid
origin, then upgrade that origin is required.

Depends on D21730

Comment 138

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P9 - Remove asmjs client in the 2_1to2_2 upgrade for QuotaManager; r?asuth r=luke

Depends on D21731

Comment 139

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P10 - Remove .tmp file in idb drectories in the 2_1to2_2 upgrade; r?asuth

Depends on D21732

Comment 140

[Tom Tung [:tt, :ttung]]

Comment on attachment 9038795 [details] [diff] [review]
Bug 1423917 - P3 - Teach the OriginParser "chrome" origin; r=asuth

I'm moving all the patches to Phabricator since I received a notification about disabling the review requests on Bugzilla.

Comment 141

[Tom Tung [:tt, :ttung]]

The last try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f4c82eb4cc540c8f4494cb620f416243fa148e4&selectedJob=235155873

Comment 142

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3af1bcb7348
P3 - Teach the OriginParser "chrome" origin; r=asuth
https://hg.mozilla.org/integration/autoland/rev/497e09d55cc5
P4 - Support moz-safe-about://home in OriginParser and make it obsolete; r=asuth
https://hg.mozilla.org/integration/autoland/rev/1a87608f65c3
P5 - Introduce a minor upgrade on QuotaManager to clean obsolete origins; r=asuth
https://hg.mozilla.org/integration/autoland/rev/330622caa8b7
P6 - Having a test to verify the version2_1To2_2 upgrade removes obsolete origins and isn't blocked by an invalid origin; r=asuth
https://hg.mozilla.org/integration/autoland/rev/5584de41d293
P7 - Ignore invalid origin files during upgrading; r=asuth
https://hg.mozilla.org/integration/autoland/rev/f285d37402e2
P9 - Remove asmjs client in the 2_1to2_2 upgrade for QuotaManager; r=luke
https://hg.mozilla.org/integration/autoland/rev/10b750eab816
P10 - Remove .tmp file in idb drectories in the 2_1to2_2 upgrade; r=asuth

Comment 143

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/a3af1bcb7348
https://hg.mozilla.org/mozilla-central/rev/497e09d55cc5
https://hg.mozilla.org/mozilla-central/rev/1a87608f65c3
https://hg.mozilla.org/mozilla-central/rev/330622caa8b7
https://hg.mozilla.org/mozilla-central/rev/5584de41d293
https://hg.mozilla.org/mozilla-central/rev/f285d37402e2
https://hg.mozilla.org/mozilla-central/rev/10b750eab816

Comment 144

[Calixte Denizet (:calixte)]

There are 5 crashes in nightly 68 with buildid 20190322012300 and the moz_crash_reason is:
MOZ_RELEASE_ASSERT(!IsDeprecatedClient(aText))

Comment 145

[Tom Tung [:tt, :ttung]]

(In reply to Calixte Denizet (:calixte) from comment #144)

There are 5 crashes in nightly 68 with buildid 20190322012300 and the moz_crash_reason is:
MOZ_RELEASE_ASSERT(!IsDeprecatedClient(aText))

It looks like it's because some users are still stuck in the version2_0 so that they crashed while doing the 2_0To2_1 upgrade. I will make a patch for that.

Comment 146

[Jan Varga [:janv]]

They can just use a profile template or something like that, so the upgrade from 2.0 to 2.1 happens every time they start the app.

Comment 147

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #146)

They can just use a profile template or something like that, so the upgrade from 2.0 to 2.1 happens every time they start the app.

I see. Thanks for explaining the reason which makes that happen!

Anyway, this crash happens because I didn't take care of users on the older storage version. It seems to me that we should just check the storage version right before the assertion for the deprecated client check.

Comment 148

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P11 - Make the deprecated client check only enable after upgrading to the newest version;

The previous patches didn't take care of the case there might have an asmjs
folder in the older version. To fix that, this patch makes Client allow to have
asmjs folders in the older version by requesting the callee of TypeFromText()
for passing the current storage version. If the version is lower than the
deprecate version, then the assertion won't be enabled.
The test verfies the fix by adding the older profile an asmjs folder.

Comment 149

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/99a6defccec0
P11 - Make the deprecated client check only enable after upgrading to the newest version; r=asuth

Comment 150

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/99a6defccec0

Comment 151

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1423917 - P12 - Return an error if it failed to remove the asmjs directory and move the deprecated client check to InitializeOrigin;

The reasons of this patch:

• P9 didn't handle the deletion of deprecated client well. It shouldn't continue
  if the client is failed to be removed. This patch correct this behavior.

• Meanwhile, it's not really good to just crash while finding a deprecated
  client. At least, the client should be removed while finding it before crashing
  the Firefox.

• Besides, if we eventaully only check the deprecated client during
  the initialization. It makes the code simpler to make the deprecated check to
  InitializeOrigin.

Therefore, this patch is a little bit against the P11, but I think it's a right
thing to do here.

Comment 152

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/55c57409e365
P12 - Return an error if it failed to remove the asmjs directory and move the deprecated client check to InitializeOrigin; r=asuth

Comment 153

[Calixte Denizet (:calixte)]

An other crash on the same assertion (MOZ_RELEASE_ASSERT(!IsDeprecatedClient(aText))) after the last patch landed:
https://crash-stats.mozilla.com/report/index/76ab1c7a-46de-4e43-ab46-467ef0190325

Comment 154

[Tom Tung [:tt, :ttung]]

(In reply to Calixte Denizet (:calixte) from comment #153)

An other crash on the same assertion (MOZ_RELEASE_ASSERT(!IsDeprecatedClient(aText))) after the last patch landed:
https://crash-stats.mozilla.com/report/index/76ab1c7a-46de-4e43-ab46-467ef0190325

Sorry, it was expected because I still want to check if all the issues are captured, but, hopefully, the number will drop to zero if the solution on P12 works.

Note that the problem should be fixed if it crashed with mozilla::dom::quota::QuotaManager::InitializeOrigin. Users are not expected to crash in the second try.

If the number of crashes for mozilla::dom::quota::QuotaManager::InitializeOrigin won't drop down in days, I will check if there are other issues on the patches.

Comment 155

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/55c57409e365

Comment 156

[Julien Cristau [:jcristau]]

No crashes for the signatures in this bug in the last couple of days.

Comment 157

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/projects/ash/rev/99a6defccec0caa61a65fddb3b9ea4b416133662
Bug 1423917 - P11 - Make the deprecated client check only enable after upgrading to the newest version; r=asuth

https://hg.mozilla.org/projects/ash/rev/55c57409e36522f685d463910e36bef56fb25869
Bug 1423917 - P12 - Return an error if it failed to remove the asmjs directory and move the deprecated client check to InitializeOrigin; r=asuth

Comment 158

[Tom Tung [:tt, :ttung]]

All the patches have been pushed into m-c and, IIUC, all the reported issues on this bug has been resolved so I'm going to close this bug.