Download errors caused by invalid temp folder
Categories
(Toolkit :: Downloads API, defect, P2)
Tracking
()
People
(Reporter: 08xjcec48, Unassigned)
References
(Blocks 1 open bug)
Details
Steps to reproduce:
Please investigate these reports. I wasn't affected by this issue, but some users are reportedly having to edit their environment variables:
https://old.reddit.com/r/firefox/comments/1d5luvh/downloads_instantly_failing/
https://old.reddit.com/r/firefox/comments/1d61qwl/downloading_files_and_adons_fail/
https://old.reddit.com/r/firefox/comments/1d7mbsi/cant_download_anything/
|
||
Comment 1•1 year ago
|
||
I've replied to the most active reddit thread. Without more info from folks on reddit there likely isn't much we can do - Firefox won't have removed its own access to TEMP and if we suddenly started writing to random other bits of users' file systems if we couldn't write to TEMP, I bet people would have something to say about that... but perhaps we can get to the bottom of why this is happening to folks and/or what we could do in response. Leaving self-needinfo to come back to this in a while as I don't check reddit very often.
|
||
Comment 2•1 year ago
|
||
Printing a meaningful error message (either on the download itself or in an infobar) may be beneficial, at least the user would have a hint about why downloads are failing suddenly.
|
||
Comment 3•1 year ago
•
|
||
:gijs figured out through discussing with users that this seems indeed caused by users losing access to their temp folder although the reason for that is unclear. Then it was also still unclear why that would make downloads fail. I was able to reproduce the issue by manually removing my user's permissions on my temp folder in a VM. The call stack for the denied Windows call points to the creation of a dummy file in nsExternalAppHandler::SetUpTempFile:
00 xul!OpenFile+0xc1 [/builds/worker/checkouts/gecko/xpcom/io/nsLocalFileWin.cpp @ 566]
01 xul!do_create+0x33 [/builds/worker/checkouts/gecko/xpcom/io/nsLocalFileWin.cpp @ 1230]
02 xul!nsLocalFile::CreateUnique+0xde [/builds/worker/checkouts/gecko/xpcom/io/nsLocalFileCommon.cpp @ 197]
03 xul!nsExternalAppHandler::SetUpTempFile+0x248 [/builds/worker/checkouts/gecko/uriloader/exthandler/nsExternalHelperAppService.cpp @ 1412]
04 xul!nsExternalAppHandler::OnStartRequest+0x57b [/builds/worker/checkouts/gecko/uriloader/exthandler/nsExternalHelperAppService.cpp @ 1634]
05 xul!nsDocumentOpenInfo::OnStartRequest+0x379 [/builds/worker/checkouts/gecko/uriloader/base/nsURILoader.cpp @ 169]
06 xul!mozilla::net::ParentProcessDocumentOpenInfo::OnDocumentStartRequest+0x4a [/builds/worker/checkouts/gecko/netwerk/ipc/DocumentLoadListener.cpp @ 304]
07 xul!mozilla::net::nsHttpChannel::CallOnStartRequest+0x618 [/builds/worker/checkouts/gecko/netwerk/protocol/http/nsHttpChannel.cpp @ 1807]
08 xul!mozilla::net::nsHttpChannel::ContinueProcessNormal+0x110 [/builds/worker/checkouts/gecko/netwerk/protocol/http/nsHttpChannel.cpp @ 2814]
09 xul!mozilla::net::nsHttpChannel::ProcessNormal+0x22 [/builds/worker/checkouts/gecko/netwerk/protocol/http/nsHttpChannel.cpp @ 2749]
0a xul!mozilla::net::nsHttpChannel::ContinueProcessResponse3+0xde [/builds/worker/checkouts/gecko/netwerk/protocol/http/nsHttpChannel.cpp @ 0]
0b xul!mozilla::net::nsHttpChannel::ContinueProcessResponse2+0x21 [/builds/worker/checkouts/gecko/netwerk/protocol/http/nsHttpChannel.cpp @ 2375]
0c xul!mozilla::net::nsHttpChannel::ContinueProcessResponse1+0x81b [/builds/worker/checkouts/gecko/netwerk/protocol/http/nsHttpChannel.cpp @ 2348]
0d xul!mozilla::net::nsHttpChannel::ProcessResponse+0x1c3 [/builds/worker/checkouts/gecko/netwerk/protocol/http/nsHttpChannel.cpp @ 2253]
0e xul!mozilla::net::nsHttpChannel::OnStartRequest+0x6ec [/builds/worker/checkouts/gecko/netwerk/protocol/http/nsHttpChannel.cpp @ 7469]
0f xul!nsInputStreamPump::OnStateStart+0x114 [/builds/worker/checkouts/gecko/netwerk/base/nsInputStreamPump.cpp @ 504]
10 xul!nsInputStreamPump::OnInputStreamReady+0x505 [/builds/worker/checkouts/gecko/netwerk/base/nsInputStreamPump.cpp @ 409]
11 xul!CallbackHolder::CallbackHolder::<lambda_1>::operator()+0x15 [/builds/worker/checkouts/gecko/xpcom/io/nsPipe3.cpp @ 86]
12 xul!NS_NewCancelableRunnableFunction<`lambda at /builds/worker/checkouts/gecko/xpcom/io/nsPipe3.cpp:84:35'>::FuncCancelableRunnable::Run+0x1f [/builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h @ 639]
13 xul!mozilla::RunnableTask::Run+0x21 [/builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp @ 578]
14 xul!mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal+0x757 [/builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp @ 905]
15 xul!mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal+0xb [/builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp @ 728]
16 xul!mozilla::TaskController::ProcessPendingMTTask+0x56 [/builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp @ 514]
17 xul!mozilla::TaskController::TaskController::<lambda_5>::operator()+0xc [/builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp @ 235]
18 xul!mozilla::detail::RunnableFunction<`lambda at /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:235:7'>::Run+0xc [/builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.h @ 548]
19 xul!nsThread::ProcessNextEvent+0x188a [/builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp @ 1199]
1a xul!NS_ProcessNextEvent+0x18e2 [/builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp @ 480]
1b xul!mozilla::ipc::MessagePump::Run+0x451 [/builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp @ 107]
1c xul!MessageLoop::RunInternal+0x16 [/builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc @ 370]
1d xul!MessageLoop::RunHandler+0x2f [/builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc @ 364]
1e xul!MessageLoop::Run+0x40 [/builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc @ 345]
1f xul!nsBaseAppShell::Run+0xa2 [/builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp @ 148]
20 xul!nsAppShell::Run+0x32 [/builds/worker/checkouts/gecko/widget/windows/nsAppShell.cpp @ 822]
21 xul!nsAppStartup::Run+0x41 [/builds/worker/checkouts/gecko/toolkit/components/startup/nsAppStartup.cpp @ 297]
22 xul!XREMain::XRE_mainRun+0xccf [/builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp @ 5746]
23 xul!XREMain::XRE_main+0x33f [/builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp @ 5958]
24 xul!XRE_main+0x85 [/builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp @ 6015]
25 firefox!do_main+0xce [/builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp @ 227]
26 firefox!NS_internal_main+0x4d0 [/builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp @ 445]
27 firefox!wmain+0x24f8 [/builds/worker/checkouts/gecko/toolkit/xre/nsWindowsWMain.cpp @ 151]
28 firefox!invoke_main+0x22 [D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl @ 90]
29 firefox!__scrt_common_main_seh+0x10c [D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl @ 288]
2a KERNEL32!BaseThreadInitThunk+0x1d
2b ntdll!RtlUserThreadStart+0x28
The dummy file is created in the temp folder and there is no fallback if that fails.
|
|
||
Comment 4•1 year ago
|
||
Many thanks to Yannis for figuring this out.
So the right fix is likely to update that code to use the user-preferred destination folder to create the "check" file. Though it's worth noting that bug 1664646 sort of suggests that on Unix machines, testing for executable-ness isn't the right thing to do, and on Windows the checks should be based on file extensions, so perhaps we can just remove the file creation entirely instead.
The other thing we should ideally improve is that if download creation fails in OnStartRequest, there is some kind of user feedback and/or error logging somewhere.
|
|
||
Comment 5•1 year ago
•
|
||
(In reply to :Gijs (he/him) from comment #4)
Though it's worth noting that bug 1664646 sort of suggests that on Unix machines, testing for executable-ness isn't the right thing to do
I would definitely agree that it's not the right thing to do here - especially since we're setting the permissions on the dummy file ourselves to 0600. I believe we'll only find an executable bit if storing to a file system where permissions don't exist and are RWX for all files, which I think could explain bug 1664646. Note that nsLocalFile::IsExecutable does more than just checking the executable bit even in the nsLocalFileUnix.cpp implementation - but I think checking the executable bit on the dummy file is only causing bug 1664646 without providing any benefit.
|
||
Comment 6•1 year ago
|
||
The severity field is not set for this bug.
:mak, could you have a look please?
For more information, please visit BugBot documentation.
|
|
||
Comment 7•1 year ago
|
||
This is potentially breaking user workflow when the temp folder is unaccessible, and the workaround is not easily discoverable => S2
Things to address:
- create the check file in the final destination (unless
browser.download.start_downloads_in_tmp_diris set, I assume). - Pass-through a better error from nsExternalAppHandler::OnStartRequest. The download code is not using the message field from onStatusChange. We could at a minimum put the error message into a tooltip on a failed download and file a follow-up for a better solution with UX involvement?
- Evaluate completely removing the creation of the check file, that may potentially help bug 1664646
I suspect if we split this bug into parts it will be able to progress faster, and fixing 1. will be sufficient to fix the worst problem for the users.
I'm happy to file the follow-ups, and I can probably find some time for 1. and investigate the tooltip solution, unless you have someone available to address the whole problem (implementing 2 and 3 immediately).
I see UX needs for 2. and potential behavior change risks for 3., thus I probably won't have the time to investigate them soon.
Thoughts?
|
|
||
Comment 8•1 year ago
|
||
(In reply to Marco Bonardo [:mak] from comment #7)
This is potentially breaking user workflow when the temp folder is unaccessible, and the workaround is not easily discoverable => S2
Things to address:
- create the check file in the final destination (unless
browser.download.start_downloads_in_tmp_diris set, I assume).- Pass-through a better error from nsExternalAppHandler::OnStartRequest. The download code is not using the message field from onStatusChange. We could at a minimum put the error message into a tooltip on a failed download and file a follow-up for a better solution with UX involvement?
- Evaluate completely removing the creation of the check file, that may potentially help bug 1664646
I suspect if we split this bug into parts it will be able to progress faster, and fixing 1. will be sufficient to fix the worst problem for the users.
I'm happy to file the follow-ups, and I can probably find some time for 1. and investigate the tooltip solution, unless you have someone available to address the whole problem (implementing 2 and 3 immediately).
I see UX needs for 2. and potential behavior change risks for 3., thus I probably won't have the time to investigate them soon.
Thoughts?
This all sounds great to me - yes please to tackling (1) here. Thank you!
(my only slight nervous-ness here is that (1) may end up entailing (3) if we find issues with creating it in the target destination... but I guess we'll find out soon enough... Also I wonder if that's the explanation for bug 1697135 which I remember puzzled us at the time...)
|
|
||
Updated•1 year ago
|
|
|
||
Comment 9•1 year ago
|
||
The problem should now be addressed, first by actually creating the initial dummy file in the right folder, and second by showing a more meaningful error message in the download panel when hovering the download.
I'm resolving this as consequence of dependencies. If we get more reports it would be good to know if they are still due to an inaccessible temp folder, or something else, and likely track in a new bug.
What's left is Bug 1904024 for which it may be necessary to analyze a plan of attack that may also solve Bug 1664646, but likely will require changes to nsLocalFile, and thus is a longer term improvement.
|
Reporter | |
Comment 10•10 months ago
|
||
A user has reported that the installation of the OpenH264 and Widevine plugins gets stuck at "will be installed shortly" when Firefox has no access to your temp folder.
All his temp folder permissions had been removed for some reason. After fixing those permissions, he was able to install the plugins.
|
|
||
Comment 11•10 months ago
|
||
(In reply to 08xjcec48 from comment #10)
A user has reported that the installation of the OpenH264 and Widevine plugins gets stuck at "will be installed shortly" when Firefox has no access to your temp folder.
All his temp folder permissions had been removed for some reason. After fixing those permissions, he was able to install the plugins.
I think that should be tracked apart. While it may use the same code, it is a different kind of internal download with potentially different fixes (a different fallback folder, or better error reason shown in the UI). In this bug we handled the user downloads when it was trying to use Temp even when it was not supposed to. And we are showing a better error message in the other cases (we can't fix settings nor the Temp permissions).
I found bug 1359723, though that seems more related to adblockers. And bug 1869070 is more related to IO errors.
It looks like bug 1922798, so I'll reopen that one, if the user could refer to it and report their info there, it may be useful.
Description
•