Disable DOS device path syntax for quota storage
Categories
(Core :: Storage: Quota Manager, task, P1)
Tracking
()
People
(Reporter: tt, Assigned: tt)
References
Details
Attachments
(2 files)
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
2.30 KB,
patch
|
Details | Diff | Splinter Review |
Quote from https://bugzilla.mozilla.org/show_bug.cgi?id=1536796#c116
Following the most recent update to Nightly - Build ID 20200401212659 - storage seems to be completely broken. All addons have stopped working.
I ran a mozregression and it pointed to this bug.
Pushlog
2020-04-02T13:25:32: INFO : application_version: 76.0a1
2020-04-02T13:25:32: INFO : platform_buildid: 20200401122401
2020-04-02T13:25:32: INFO : platform_changeset: 876a51b67aca75334dbe942e425d4eab26919b6b
2020-04-02T13:25:32: INFO : platform_repository: https://hg.mozilla.org/integration/autoland
2020-04-02T13:25:32: INFO : platform_version: 76.0a1
2020-04-02T13:25:41: INFO : [Parent 2028, Gecko_IOThread] WARNING: file /builds/worker/checkouts/gecko/ipc/chromium/src/base/process_util_win.cc, line 166
2020-04-02T13:25:50: INFO : Narrowed integration regression window from [39c389ff, 990288d7] (3 builds) to [876a51b6, 990288d7] (2 builds) (~1 steps left)
2020-04-02T13:25:50: DEBUG : Starting merge handling...
2020-04-02T13:25:50: DEBUG : Using url: https://hg.mozilla.org/integration/autoland/json-pushes?changeset=990288d76a16f51a970b512ef9d97990205b312d&full=1
2020-04-02T13:25:52: DEBUG : Found commit message:
Bug 1536796 - P6 - Changes on dom/quota unit test to verify the fix; r=dom-workers-and-storage-reviewers,janv
Differential Revision: https://phabricator.services.mozilla.com/D60872
2020-04-02T13:25:52: DEBUG : Did not find a branch, checking all integration branches
2020-04-02T13:25:52: INFO : The bisection is done.
2020-04-02T13:25:52: INFO : Stopped
Assignee | ||
Comment 1•5 years ago
|
||
Assignee | ||
Comment 2•5 years ago
|
||
Assignee | ||
Comment 3•5 years ago
|
||
Hi Pulse,
Sorry for causing the storage to be broken and thank you for reporting that!
D69318 should be able to disable the change I made in Bug 1536796.
Would you mind running a debug build and sharing the log that is related to "QuotaManager", "IndexedDB"? Thanks in advance!
Comment 4•5 years ago
|
||
We should have a pref for this and probably address bug 1624802.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 5•5 years ago
|
||
This will need to be requested to uplift, I think.
Comment 6•5 years ago
|
||
Ok, I checked the failures on try. I believe a deadlock is causing these failures.
Normally, the first localStorage access is synchronously blocking the main thread and the parent side is asked to prepare a datastore. QuotaManager is involved in this process as well. We fixed QuotaManager and QM clients to not use the main thread because that can cause deadlocks when LSNG is enabled.
However, the new pref has mirroring set to once
. Prefs like that need to call MaybeInitOncePrefs
https://searchfox.org/mozilla-central/rev/7fba7adfcd695343236de0c12e8d384c9b7cd237/modules/libpref/init/StaticPrefListBegin.h#35
That may dispatch a runnable to the main thread and synchronously block the current thread:
https://searchfox.org/mozilla-central/source/modules/libpref/Preferences.cpp#5297
All in all, if an xpcshell-test hasn't initialized "once" prefs yet and we get a localStorage call, we end up in a deadlock situation.
We should probably force initialization of prefs on the main thread somewhere here:
https://searchfox.org/mozilla-central/rev/7fba7adfcd695343236de0c12e8d384c9b7cd237/dom/quota/ActorsParent.cpp#3151
Comment 7•5 years ago
|
||
Assignee | ||
Comment 8•5 years ago
|
||
With Jan's patch in comment 7, if I move the check outside of #ifdef XP_WIN
#endif
, all xpchsell tests for localstorage pass on my Linux machine.
./mach test toolkit/components/antitracking/test/xpcshell/test_purge_trackers.js
failed still, but it fails without my patches.
Comment 9•5 years ago
|
||
On my Mac, with Tom's original patch, but without my patch, I see the same deadlock when I run test_purge_trackers.js.
On my Mac, with Tom's original patch and with my patch, test_purge_trackers.js passes successfully.
Assignee | ||
Comment 10•5 years ago
|
||
try for all xpcshell tests with my patch (the one that uses mirror: once
) and with Jan's patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=521d532d7b55c9d75c8ded6b284ad6e582e20f82&selectedJob=295958135
The dom/quota/test/unit/test_validOrigins.js
failed, but I believe we can solve this by moving the code for setting the pref before do_get_profile();
try for all tests on Windows with my current patch (the one that uses mirror: always
): https://treeherder.mozilla.org/#/jobs?repo=try&revision=0df63a8e683e81a979e575f31077adc23c527af4&selectedJob=295954604
Assignee | ||
Comment 11•5 years ago
|
||
(In reply to Tom Tung [:tt, :ttung] from comment #10)
The
dom/quota/test/unit/test_validOrigins.js
failed, but I believe we can solve this by moving the code for setting the pref before do_get_profile();
try to verify my guess: https://treeherder.mozilla.org/#/jobs?repo=try&revision=04e8ef586fcec7f22680ad288b44faa5b62b77d7
Comment 12•5 years ago
|
||
(In reply to Tom Tung [:tt, :ttung] from comment #11)
(In reply to Tom Tung [:tt, :ttung] from comment #10)
The
dom/quota/test/unit/test_validOrigins.js
failed, but I believe we can solve this by moving the code for setting the pref before do_get_profile();try to verify my guess: https://treeherder.mozilla.org/#/jobs?repo=try&revision=04e8ef586fcec7f22680ad288b44faa5b62b77d7
Even if this is green, I recommend doing a full try push, just in case the additional MaybeInitOncePrefs
affects something else.
Assignee | ||
Comment 13•5 years ago
|
||
(In reply to Tom Tung [:tt, :ttung] from comment #11)
try to verify my guess: https://treeherder.mozilla.org/#/jobs?repo=try&revision=04e8ef586fcec7f22680ad288b44faa5b62b77d7
It's green.
(In reply to Jan Varga [:janv] from comment #12)
Even if this is green, I recommend doing a full try push, just in case the additional
MaybeInitOncePrefs
affects something else.
full try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e689849aa6ee97aa018539223cfcc3e8885569a
Assignee | ||
Comment 14•5 years ago
|
||
(In reply to Tom Tung [:tt, :ttung] from comment #13)
full try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e689849aa6ee97aa018539223cfcc3e8885569a
Hmm, this one looks suspicious. It could be because of the additional MaybeInitOncePrefs
change. (We cause the initialization for prefs earlier in some cases). I will take a look tomorrow.
Comment 15•5 years ago
|
||
In that case, we shouldn't use the Once pref, it seems to be designed primarily for graphics.
Comment 16•5 years ago
|
||
Assignee | ||
Comment 17•5 years ago
|
||
(In reply to Jan Varga [:janv] from comment #16)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd4daf432c618961b61ecd2b551b79f5b653a914
Note to me:
- I was trying to use
Atomic<bool>
, butAtomic<uint32_t>
is much better since it can differentiate between set and not-set. - I was worried about whether should I call
init
to ensure every the pref is set, but callinginit
would highly likely to cause some test failures (even if we callreset
right after that) and it cannot happen that QuotaManager cache the different value between itself and tests. Thus, the worry is wrong.
Comment 18•5 years ago
|
||
Comment 19•5 years ago
|
||
Assignee | ||
Comment 20•5 years ago
|
||
I tested the perf by download an opt build from autoland on a Window8.1 via VirtualBox.
What I did was:
- Create a profile and have a long file name so that the path length for metadata-v2 file is 267 chars for "https://firefox-storage-test.glitch.me"
Current NIghtly (don't have the patch in this Bug):
- All storage tests on the website are fully operational
Downloaded an opt build with the default value (false
) for the pref:
- All storage tests failed (expected)
Downloaded an opt build with a flipped value (true
) for the pref:
- All storage tests are fully operational
Comment 21•5 years ago
|
||
(In reply to Tom Tung [:tt, :ttung] from comment #20)
- All storage tests on the website are fully operational
Isn't that wonderful ?
Assignee | ||
Comment 22•5 years ago
|
||
(In reply to Jan Varga [:janv] from comment #21)
Isn't that wonderful ?
Yeah, but I meant I have no idea what causes the extensions of the user that reported the issue to fail since the storage test succeeds for me. I was trying to find the causes and was thinking maybe I can find something on my virtual machine by visiting that website.
Comment 23•5 years ago
|
||
Tom, can you link the original report regarding that extension here?
Assignee | ||
Comment 24•5 years ago
|
||
(In reply to Jens Stutte [:jstutte] from comment #23)
Tom, can you link the original report regarding that extension here?
https://bugzilla.mozilla.org/show_bug.cgi?id=1536796#c116
The user didn't provide which extensions failed. They said all add-ons have stopped working and they found bug 1536796 through mozregression.
I have needinfo'ed them in this bug but they haven't replied yet.
Comment 25•5 years ago
|
||
bugherder |
Assignee | ||
Comment 26•5 years ago
|
||
We will need to uplift this to Beta and decide if the pref should be off by default or not a few days later.
Updated•5 years ago
|
Assignee | ||
Comment 27•5 years ago
|
||
Comment on attachment 9137662 [details]
Bug 1626846 - Disable useDOSDevicePathSyntax for QuotaStorage on Windows;
Beta/Release Uplift Approval Request
- User impact if declined: This bug disables the functionalities on Bug 1536796, which is on 76 already. A user reports that there were some failures on their Windows after appling patches in Bug 1536796.
If declined, the functionalities on Bug 1536796 will keep being enabled on Beta. - Is this code covered by automated tests?: Yes
- 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 is small.
It only does:
- Add a pref on Windows for protecting impacts on bug 1536796
- The pref is off by default
- String changes made/needed:
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 28•5 years ago
•
|
||
Tom, this doesn't have to be reopened if you only ask for beta uplift. The status always reflects m-c status. Status on other branches should be reflected by tracking flags.
Assignee | ||
Comment 29•5 years ago
|
||
(In reply to Jan Varga [:janv] from comment #28)
Tom, this doesn't have to be reopened if you only ask for beta uplift. The status always reflects m-c status. Status on other branches should be reflected by tracking flags.
Ah, I see. Thanks for telling me that! I was worried if the request would be overlooked if the status is close, but that is incorrect.
Comment 30•5 years ago
|
||
Comment on attachment 9137662 [details]
Bug 1626846 - Disable useDOSDevicePathSyntax for QuotaStorage on Windows;
Disables the feature to avoid possible bugs. Approved for 76.0b3.
Comment 31•5 years ago
|
||
bugherder uplift |
Comment 32•4 years ago
|
||
Just booted into Windows 10 for the first time in a couple of days, updated to build 20200423214309 and the issue is back. If I set dom.quotaManager.useDOSDevicePathSyntax
to false and restart the problem goes away.
Comment 33•4 years ago
|
||
Update: This seems to be a problem with certain addons. So far, uMatrix and Temporary Containers are involved. With uMatrix active and the pref set to true, tabs stop loading. Temporary Containers throws and error. The browser console has many many of the following:
IndexedDB UnknownErr: ActorsParent.cpp:570
UnknownError: IndexedDB: main/anti-tracking-url-decoration getLastModified() IndexedDB: open() The operation failed for reasons unrelated to the database itself and not covered by any other error code. Database.jsm:29:5
IndexedDB UnknownErr: ActorsParent.cpp:570
UnknownError: The operation failed for reasons unrelated to the database itself and not covered by any other error code. ExtensionStorageIDB.jsm:831
UnknownError: The operation failed for reasons unrelated to the database itself and not covered by any other error code. ExtensionStorageIDB.jsm:831
Unknown category for SetEventRecordingEnabled: fxmonitor
IndexedDB: main/hijack-blocklists getLastModified() IndexedDB: open() The operation failed for reasons unrelated to the database itself and not covered by any other error code. Database.jsm:29
IndexedDB: main/search-config getLastModified() IndexedDB: open() The operation failed for reasons unrelated to the database itself and not covered by any other error code. Database.jsm:29
IndexedDB: main/hijack-blocklists getLastModified() IndexedDB: open() The operation failed for reasons unrelated to the database itself and not covered by any other error code.
Assignee | ||
Comment 35•4 years ago
•
|
||
(In reply to Pulse from comment #32)
Just booted into Windows 10 for the first time in a couple of days, updated to build 20200423214309 and the issue is back. If I set
dom.quotaManager.useDOSDevicePathSyntax
to false and restart the problem goes away.
Thanks for reporting back! From your comment in #33. It looks like an issue in QuotaManager/IndexedDB
I couldn't reproduce the issue mentioned in comment 32 with uMatrix
on my Windows 8.1 running in virtual machine. The build_id is 20200423214309.
I browsed into https://firefox-storage-test.glitch.me/ and https://www.facebook.com and these two websites work fine.
I verified that uMatrix is still working by
- Install the uMatrix
- Disable "firefox-storage-test-oth.glitch.me" in "https://firefox-storage-test.glitch.me". After that, Cache API failed and I saw "Network error" in "Debug Info" (note that other storage APIs work functionally)
- Disable "fbcdn.net" in "https//www.facebook.com". After that, the page stop loading.
Enabling the disabled items and reload these pages again, these pages loaded successfully and there are no storage failures that showed up in the browser console.
I will test if I can reproduce the issue with Temporary Containers
. If not, maybe it's a Windows 10 only issue?
Edit: I couldn't reproduce the issue for Temporary Containers
with 20200423214309 Nightly in my Window8.1 running in virtual machine.
I tested both https://firefox-storage-test.glicth.me and https://www.facebook.com and both pages work fine. (I need to enter the account and password again and the storage test shows "This is your first visit" so I assume the extension work fine)
Edit: Correct the typo (s/glicth/glitch/g). (Thanks for the reminder from Jens)
Assignee | ||
Updated•4 years ago
|
Comment 36•4 years ago
|
||
Attention, there is a typo in the glitch.me URL: Should read https://firefox-storage-test.glitch.me/.
Assignee | ||
Comment 37•4 years ago
|
||
(In reply to Tom Tung [:tt, :ttung] from comment #35)
I will test if I can reproduce the issue with
Temporary Containers
. If not, maybe it's a Windows 10 only issue?
I ask a colleague (Alpahn) in the Berlin office who has a Windows 10 by hand and tested what I had tested in comment 35 and he couldn't be able to reproduce the issue either.
I wonder whether there are some other factors involved so that we could not reproduce the issue.
Pulse, did you use a fresh profile when you found these issues? Could you re-do the STR with a debug build and maybe sharing the log here or private email if you don't mind. So that we can narrow down the issue with a specific file or origin.
If not, could you check the origin directory name [1] in your profile and share the longest string length of the file paths?
I suspected that maybe you have a file with a long file name in your profile and maybe they are related to this issue.
[1] You can find them in "about:profiles", click the "show in folder", check the longest string length of file paths for files under ${profile}/storage/defult/*
Comment 38•4 years ago
|
||
I'm currently not able to reproduce the Temporary Containers issue in a new profile. I suspect it's a combination of my addons and the quota pref. I'll keep trying. However, I used my existing Nightly profile with the debug build and I had the same failures noted above. Here's a paste if it helps.
Later today I'll do some more testing and report back.
FWIW My addons are:
uBlock Origin
uMatrix
Clear URLs
Containerise
Temporary Containers
Decentraleyes
Forget Me Not
Old Reddit
Reddit Enhancement Suite
Reddit Comment Highlights
Reddit Comment Collapser
Smart Referer
Swift Selection Search
To Google Translate
Google Search Link Fix
Good Twitter
Comment 39•4 years ago
|
||
Thanks for providing the debugging information, we finally see where it's failing:
[Parent 5204, IPDL Background] WARNING: 'NS_FAILED(rv)', file /builds/worker/checkouts/gecko/dom/quota/QuotaCommon.cpp, line 100
However, we need to add additional output to see exact file path that is causing this.
Assignee | ||
Comment 40•4 years ago
•
|
||
That means NS_NewLocalFile(aPath, /* aFollowLinks */ false, getter_AddRefs(file))
failed.
And it probably implies there is a failure if either of these
Edit: So I guess if we can differentiate the error code then we can narrow down the issue.
Meanwhile, I will start by checking IsBlockedUNCPath
because that's the most suspicious next Monday.
Comment 41•4 years ago
|
||
I think we should add additional warning message that prints exact file path.
Assignee | ||
Comment 42•4 years ago
|
||
(In reply to Jan Varga [:janv] from comment #41)
I think we should add additional warning message that prints exact file path.
That sounds good to me. I will file a bug and provide a patch there today.
Comment 43•4 years ago
|
||
Hmm, I read here that there is a sanity check for ':' as second character. I assume, that (at least) this test would fail with the long file path syntax of Windows? Maybe we should check less sanity here (and leave it to the OS to give us an error in case) ?
Assignee | ||
Comment 44•4 years ago
•
|
||
(In reply to Jens Stutte [:jstutte] from comment #43)
Hmm, I read here that there is a sanity check for ':' as second character. I assume, that (at least) this test would fail with the long file path syntax of Windows? Maybe we should check less sanity here (and leave it to the OS to give us an error in case) ?
Jens, did you mean because the long file path starts with \\?\
? Then it shouldn't fit the condition because of secondChar != L'\\' || firstChar != L'\\'
Comment 45•4 years ago
•
|
||
(In reply to Tom Tung [:tt, :ttung] from comment #44)
Jens, did you mean because the long file path starts with
\\?\
? Then it shouldn't fit the condition because ofsecondChar != L'\\' || firstChar != L'\\'
Oh, I just not read the entire condition, sorry for the noise. Still I am wondering, if sanity checks are a good thing to do at all, but the first step as you all already pointed out is to know, what the file path looks like, that is failing.
Assignee | ||
Comment 46•4 years ago
•
|
||
Pulse, would you mind doing the STR again with the latest Nightly and sharing the log from the browser console here since bug 1633326 is fixed.
The log should look like: Failed to construct a file for path (${file_path})
.
And if you run with a debug build, it should appear right after [xxx, IPDL Background] WARNING: 'NS_FAILED(rv)', file /builds/worker/checkouts/gecko/dom/quota/QuotaCommon.cpp, line 100
.
From my understanding, the operation would fail if the file_path
looks like either:
- Empty
- Contains
/
- Neither in
^[A-Za-z]:.*
nor^\\.*
(regex) - Cannot get the drive for
^[A-Za-z]:.*
cases
Also, if the file path doesn't look suspicious, could you also check whether network.file.disable_unc_paths
is defined in your "about:config". The pref is mainly used for the Tor browser and it would block also UNC file path including the \\?\
if it's not in the allow list.
Edit: What Jan had mentioned in comment 47 is correct, so I updated this comment accordingly.
Comment 47•4 years ago
|
||
QM_Warning
outputs to the browser console as well, so we should see it normal (optimized) builds too.
Assignee | ||
Comment 49•4 years ago
|
||
(In reply to Pulse from comment #48)
Here's a a paste from the latest debug build:
Thanks!!
So, the file path itself looks fine.
It doesn't look like the content of the file path matches the cases I mentioned about, so I lean to that it's probably because an extension causes IsBlockedUNCPath
to block the file.
Comment 50•4 years ago
|
||
The question is if a normal path should be considered as UNC path if prefixed with \\?\
.
I already mentioned that even UNC paths can be prefixed with \\?\
, but it gets a bit more complicated:
https://docs.microsoft.com/en-us/dotnet/standard/io/file-path-formats
There is a specific link for UNCs that is called, not surprisingly, UNC. For example:
\\.\UNC\Server\Share\Test\Foo.txt \\?\UNC\Server\Share\Test\Foo.txt
Another question is why the base directory is "D:\Windows\Downloads\target\aaa".
Maybe it's overiden by NS_APP_INDEXEDDB_PARENT_DIR which is not in the whitelist for blocked UNC paths:
https://searchfox.org/mozilla-central/rev/2bfe3415fb3a2fba9b1c694bc0b376365e086927/xpcom/io/FilePreferences.cpp#118
So it seems there are multiple things to get this right completely.
Updated•4 years ago
|
Assignee | ||
Comment 51•4 years ago
|
||
(In reply to Jan Varga [:janv] from comment #50)
The question is if a normal path should be considered as UNC path if prefixed with
\\?\
.
I already mentioned that even UNC paths can be prefixed with\\?\
, but it gets a bit more complicated:
https://docs.microsoft.com/en-us/dotnet/standard/io/file-path-formatsThere is a specific link for UNCs that is called, not surprisingly, UNC. For example: \\.\UNC\Server\Share\Test\Foo.txt \\?\UNC\Server\Share\Test\Foo.txt
I think they should be considered as UNC paths, but I'm not so sure if they should be blocked in the IsBlockedUNCPaths
. That function was mainly created to prevent cases for the smb url. And, I guess if a file started with a disk designator and backslash before it's prepended, then it is not in the case to be blocked.
Maybe, we can try to remove the prefix like:
\\?\C:\home\foo.txt -> C:\home\foo.txt
\\?\UNC\Server\Share\Test\Foo.txt -> \\Server\Share\Test\Foo.txt
Then, check the path without the prefix in that function.
But there are also cases like: \\?\server1\e:\utilities\\filecomparer\
and that needs more investigation.
Another question is why the base directory is "D:\Windows\Downloads\target\aaa".
Maybe it's overiden by NS_APP_INDEXEDDB_PARENT_DIR which is not in the whitelist for blocked UNC paths:
https://searchfox.org/mozilla-central/rev/2bfe3415fb3a2fba9b1c694bc0b376365e086927/xpcom/io/FilePreferences.cpp#118So it seems there are multiple things to get this right completely.
I wonder if it's still NS_APP_USER_PROFILE_50_DIR
. If NS_APP_USER_PROFILE_50_DIR
didn't have the prefix when it's added, then it shouldn't be put in the allow list because it's not an unc path.
Perhaps, an option here is to put these three paths with the prefix so that they can be added into the allow list. And thus, the file path, in this case, might not be blocked.
Comment 52•4 years ago
|
||
Please file a new bug for that.
Comment 53•4 years ago
|
||
(In reply to Pulse from comment #32)
Just booted into Windows 10 for the first time in a couple of days, updated to build 20200423214309 and the issue is back. If I set
dom.quotaManager.useDOSDevicePathSyntax
to false and restart the problem goes away.
Is this fixed? I did not see it get unmarked after this comment. Should it get unmarked?
Assignee | ||
Comment 54•4 years ago
|
||
(In reply to Worcester12345 from comment #53)
(In reply to Pulse from comment #32)
Just booted into Windows 10 for the first time in a couple of days, updated to build 20200423214309 and the issue is back. If I set
dom.quotaManager.useDOSDevicePathSyntax
to false and restart the problem goes away.Is this fixed? I did not see it get unmarked after this comment. Should it get unmarked?
Hi,
We had unmarked the pref in this patch D69318 in this bug, but revert the change in bug 1632133.
Do you still have an issue around that pref? If you still have an issue with dom.quotaManager.useDOSDevicePathSyntax
, to help us to do the initial investigation, could you:
- Restart firefox
- Visit "https://firefox-storage-test.glitch.me"
- Share the result at "about:telemetry#search=QM_FIRST_INITIALIZATION_ATTEMPT" here?
- Share the log in the browser console here as well?
Then, we will see if we should reopen this bug or file another one. Thanks in advance!
Comment 55•4 years ago
|
||
(In reply to Tom Tung [:tt, :ttung] from comment #54)
Do you still have an issue around that pref?
No, I'm good. Thanks.
Description
•