Closed Bug 1423917 Opened 7 years ago Closed 6 years ago

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

Categories

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

Product:
Core
Component:
Storage: Quota Manager
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox-esr60 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox65 --- wontfix
firefox66 + fixed
firefox67 --- fixed
firefox68 --- fixed

People

(Reporter: Kwan, Assigned: tt)

References

(Blocks 3 open bugs)

Details

(Keywords: dataloss, regression)

Crash Data

Attachments

(14 files, 39 obsolete files)

The two responsible folders zipped up
(deleted), application/zip
Details
Bug 1423917 - P2 - Have a test to verify a set of hidden files don't block QuotaManager's initialization and getting usauge; r=asuth
(deleted), patch
tt
: review+
lizzard
: approval-mozilla-beta+
Details | Diff | Splinter 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
(deleted), patch
tt
: review+
lizzard
: approval-mozilla-beta+
Details | Diff | Splinter Review
P1 for beta
(deleted), patch
Details | Diff | Splinter Review
P2 for beta
(deleted), patch
Details | Diff | Splinter Review
Bug 1423917 - P3 - Teach the OriginParser "chrome" origin; r=asuth
(deleted), text/x-phabricator-request
Details
Bug 1423917 - P4 - Support moz-safe-about://home in OriginParser and make it obsolete; r=asuth
(deleted), text/x-phabricator-request
Details
Bug 1423917 - P5 - Introduce a minor upgrade on QuotaManager to clean obsolete origins; r=asuth
(deleted), text/x-phabricator-request
Details
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
(deleted), text/x-phabricator-request
Details
Bug 1423917 - P7 - Ignore invalid origin files during upgrading; r=asuth
(deleted), text/x-phabricator-request
Details
Bug 1423917 - P9 - Remove asmjs client in the 2_1to2_2 upgrade for QuotaManager; r?asuth r=luke
(deleted), text/x-phabricator-request
Details
Bug 1423917 - P10 - Remove .tmp file in idb drectories in the 2_1to2_2 upgrade; r?asuth
(deleted), text/x-phabricator-request
Details
Bug 1423917 - P11 - Make the deprecated client check only enable after upgrading to the newest version;
(deleted), text/x-phabricator-request
Details
Bug 1423917 - P12 - Return an error if it failed to remove the asmjs directory and move the deprecated client check to InitializeOrigin;
(deleted), text/x-phabricator-request
Details
Attached file The two responsible folders zipped up (deleted) —
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)
Andrew, any ideas here?
Flags: needinfo?(bugmail)
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?
Component: DOM: IndexedDB → DOM: Quota Manager
Flags: needinfo?(bugmail) → needinfo?(jvarga)
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! ;)
Flags: needinfo?(ttung)
(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).
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.
Flags: needinfo?(moz-ian)
(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.
Flags: needinfo?(moz-ian)
(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 ?
Flags: needinfo?(jvarga)
(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
(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.
Priority: -- → P2
I'd like to take this bug since Andrew doesn't have time to fix this now. :p
Assignee: nobody → ttung
Jan suggested me that I might need a hash table for improving the performance after removing the #ifndef and #endif.
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.
status-firefox57: affected → wontfix
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.
Flags: needinfo?(jvarga)
What, another parent process with QuotaManager ?! That sounds terrible. We should stop doing that or implement an inter-process locking which also sounds terrible.
Flags: needinfo?(jvarga)
How in the world can actually two separate Gecko instances share the same profile ?
(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.
(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.
Fix adding extra empty lines to align the coding format
Attachment #8940650 - Attachment is obsolete: true
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!
Attachment #8940647 - Flags: review?(jvarga)
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.
Attachment #8940652 - Flags: review?(jvarga)
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!
Attachment #8940651 - Flags: review?(jvarga)
Attachment #8940647 - Flags: review?(jvarga) → review+
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
Attachment #8940652 - Flags: review?(jvarga)
Update the test.
Attachment #8940652 - Attachment is obsolete: true
Attachment #8943456 - Flags: review?(jvarga)
Assignee: ttung → nobody
Assignee: nobody → jvarga
It's almost there, just needs to be brought to finish. Reassigned.
Assignee: jvarga → bugmail
Hey Andrew, want to take a look at this?
Flags: needinfo?(bugmail)
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.
Flags: needinfo?(bugmail) → needinfo?(shes050117)
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.
Assignee: bugmail → shes050117
Flags: needinfo?(shes050117)
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.
Attachment #8940651 - Flags: review?(jvarga)
Attachment #8943456 - Flags: review?(jvarga)
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.
Attachment #8943456 - Attachment is obsolete: true
Attachment #8998777 - Flags: review?(jvarga)
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.
Attachment #8940651 - Attachment is obsolete: true
Attachment #8998785 - Flags: review?(jvarga)
Blocks: 1482662
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.
Attachment #8998777 - Flags: review?(jvarga)
Attachment #8998785 - Flags: review?(jvarga)
(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)
Blocks: Session_managers
See also: bug 944918, bug 1415792
Blocks: 944918
No longer blocks: 1290481
Depends on: 1395102
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.
Attachment #8940647 - Attachment is obsolete: true
Attachment #8998777 - Attachment is obsolete: true
Attachment #8998785 - Attachment is obsolete: true
Attached patch P1 - white list dot files (.*) and desktop.ini (obsolete) (deleted) — — Splinter Review
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.
(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 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 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.
(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.
(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!
Attached patch P1 - white list dot files (.*) and desktop.ini (obsolete) (deleted) — — Splinter Review
(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!
Attachment #9012063 - Attachment is obsolete: true
Attachment #9017154 - Attachment is obsolete: true
Attached patch P2 (v1) Test for p1 (obsolete) (deleted) — — Splinter Review
Attachment #9012064 - Attachment is obsolete: true
Attached patch P6 (v1) Ignore invalid origins during upgrades (obsolete) (deleted) — — Splinter Review
Attachment #9012066 - Attachment is obsolete: true
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.
Attachment #9012070 - Attachment is obsolete: true
Attachment #9019723 - Attachment is obsolete: true
Attached patch P5 (v1) Ignore invalid origins during upgrades (obsolete) (deleted) — — Splinter Review
Attachment #9019724 - Attachment is obsolete: true
Attachment #9019731 - Attachment is obsolete: true
Attached patch P7 (v1.1) A test for the minor upgrade (obsolete) (deleted) — — Splinter Review
Attachment #9019722 - Attachment is obsolete: true
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.
Attachment #9019717 - Flags: review?(jvarga)
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.
Attachment #9019718 - Flags: review?(jvarga)
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.
Attachment #9019721 - Flags: review?(jvarga)
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
Attachment #9021151 - Flags: review?(jvarga)
Attachment #9021152 - Attachment is obsolete: true
Attachment #9021153 - Attachment is obsolete: true
Attached patch P6 (v1.1) A test for the minor upgrade (obsolete) (deleted) — — Splinter Review
Ok, I'll take a look, thanks.
Is this still on the radar for 65? Or should we plan to try landing/enabling for 66?
Flags: needinfo?(shes050117)
Flags: needinfo?(overholt)
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.
Attachment #9019717 - Flags: review?(jvarga) → review?(bugmail)
Comment on attachment 9019718 [details] [diff] [review] P2 (v1) Test for p1 This is a test for verifying P1
Flags: needinfo?(shes050117)
Attachment #9019718 - Flags: review?(jvarga) → review?(bugmail)
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.
Attachment #9019721 - Flags: review?(jvarga) → review?(bugmail)
Comment on attachment 9021151 [details] [diff] [review] P4 (v1) Recognize the "moz-safe-about+++home" in OriginParser Patch for handling "moz-safe-about+++home"
Attachment #9021151 - Flags: review?(jvarga) → review?(bugmail)
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 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.
Attachment #9022118 - Flags: feedback?(bugmail)
Comment on attachment 9022120 [details] [diff] [review] P6 (v1.1) A test for the minor upgrade Patch for verifying the minor upgrade
Attachment #9022120 - Flags: feedback?(bugmail)
Attachment #9021154 - Attachment is obsolete: true
Andrew, you should take a look at bug 1491637 too, it's closely related.
(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.
Flags: needinfo?(overholt)
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).
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
Unfortunately, we decided to resolve/land this in FF66
status-firefox65: ? → wontfix
tracking-firefox65: ? → ---
Update patch due to rebasing (matching with the new coding style) and applying QuotaManager::IsOSMetadata() to dom/indexedDB/ActorsParent.cpp
Update for rebasing and using the "if-else" syntax rather than the "? :" syntax
Attachment #9019717 - Attachment is obsolete: true
Attachment #9029675 - Attachment is obsolete: true
Attachment #9019717 - Flags: review?(bugmail)
Attachment #9032448 - Flags: review?(bugmail)
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.
Attachment #9032448 - Flags: review?(bugmail) → review+
Comment on attachment 9019718 [details] [diff] [review] P2 (v1) Test for p1 Review of attachment 9019718 [details] [diff] [review]: ----------------------------------------------------------------- Nice test!
Attachment #9019718 - Flags: review?(bugmail) → review+
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.
Attachment #9029680 - Flags: review+
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.
Attachment #9019721 - Flags: review?(bugmail)
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!
Attachment #9021151 - Flags: review?(bugmail) → review+
Attachment #9022118 - Flags: feedback?(bugmail) → review+
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!
Attachment #9022120 - Flags: feedback?(bugmail) → review+
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.
Attachment #9029689 - Flags: review+
Attached patch interdiff-p1.patch (obsolete) (deleted) — — Splinter Review

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

Attachment #9038212 - Flags: review?(bugmail)

(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 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.
Attachment #9038212 - Flags: review?(bugmail) → review+
Attachment #9029680 - Attachment is obsolete: true
Attachment #9038794 - Attachment description: Bug 1423917 - P1 - Ignore dot files in QM and add .desktop, Desktop.ini, desktop.ini, Thumbs.db into the list of OSMetadata; r=asut → Bug 1423917 - P1 - Ignore dot files in QM and add .desktop, Desktop.ini, desktop.ini, Thumbs.db into the list of OSMetadata; r=asuth
Attachment #9038214 - Attachment description: Bug 1423917 - P2 - Have a test to verify a set of hidden files don't block QuotaManager's initialization and getting usauge; r=asut → Bug 1423917 - P2 - Have a test to verify a set of hidden files don't block QuotaManager's initialization and getting usauge; r=asuth
Attachment #9029689 - Attachment is obsolete: true
Blocks: 1522464
Comment on attachment 9019721 [details] [diff] [review] P3 (v1) Teach Origin "chrome" and obsolete other chrome origins Open Bug 1522464 for dealing with this.
Attachment #9019721 - Attachment is obsolete: true
Attachment #9038795 - Flags: review+
Attachment #9038796 - Flags: review+
Attachment #9038797 - Flags: review+
Attachment #9038798 - Flags: review+
Attachment #9038799 - Flags: review+
Attachment #9038794 - Flags: review+

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

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

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

Pushed by shes050117@gmail.com: 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

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

Keywords: leave-open

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!

Flags: needinfo?(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).

Flags: needinfo?(pascalc) → needinfo?(lhenry)

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

Flags: needinfo?(shes050117)

(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

Flags: needinfo?(shes050117)
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.
Attachment #9042949 - Flags: review?(luke)
Attachment #9042949 - Flags: review?(bugmail)
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.
Attachment #9042950 - Flags: review?(bugmail)
Attachment #9042949 - Flags: review?(luke) → review+
Blocks: 1520931

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

Attachment #9038794 - Flags: approval-mozilla-beta?

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

Attachment #9038214 - Flags: approval-mozilla-beta?

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

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

Flags: needinfo?(shes050117)

(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

Flags: needinfo?(shes050117)

(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 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.
Attachment #9038794 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Attachment #9038214 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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?

Flags: needinfo?(shes050117)
Attached patch P1 for beta (deleted) — — Splinter Review

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

Attached patch P2 for beta (deleted) — — Splinter Review

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!

Flags: needinfo?(shes050117) → needinfo?(nbeleuzu)

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

Flags: needinfo?(nbeleuzu)

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.

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 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.
Attachment #9038795 - Attachment is obsolete: true
Attachment #9038796 - Attachment is obsolete: true
Attachment #9038797 - Attachment is obsolete: true
Attachment #9038798 - Attachment is obsolete: true
Attachment #9038799 - Attachment is obsolete: true
Attachment #9042949 - Attachment is obsolete: true
Attachment #9042949 - Flags: review?(bugmail)
Attachment #9042950 - Attachment is obsolete: true
Attachment #9042950 - Flags: review?(bugmail)
Pushed by shes050117@gmail.com: 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

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

Crash Signature: [@ mozilla::dom::quota::Client::TypeFromText]

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

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.

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

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.

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

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.

Pushed by shes050117@gmail.com: 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

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

Crash Signature: [@ mozilla::dom::quota::Client::TypeFromText] → [@ mozilla::dom::quota::Client::TypeFromText] [@ <name omitted> | mozilla::dom::quota::QuotaManager::InitializeOrigin]

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

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

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

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.

Status: NEW → RESOLVED
Closed: 6 years ago
Keywords: leave-open
Resolution: --- → FIXED
Blocks: IndexedDB-SM
No longer blocks: Session_managers
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: