1900521 - Download errors caused by invalid temp folder

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect, P2)

Description

[08xjcec48]

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

[:Gijs (he/him)]

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

[Marco Bonardo [:mak]]

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

[Yannis Juglaret [:yannis]]

: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

[:Gijs (he/him)]

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

[Yannis Juglaret [:yannis]]

(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

[BugBot [:suhaib / :marco/ :calixte]]

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

[Marco Bonardo [:mak]]

This is potentially breaking user workflow when the temp folder is unaccessible, and the workaround is not easily discoverable => S2

Things to address:

1. create the check file in the final destination (unless browser.download.start_downloads_in_tmp_dir is set, I assume).
2. 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?
3. 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

[:Gijs (he/him)]

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

1. create the check file in the final destination (unless browser.download.start_downloads_in_tmp_dir is set, I assume).
2. 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?
3. 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...)

Comment 9

[Marco Bonardo [:mak]]

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.