Open Bug 1697135 Opened 4 years ago Updated 2 months ago

Downloading Files fires Filesystem created event twice

Categories

(Firefox :: File Handling, defect, P3)

Product:
Firefox
Component:
File Handling
Version:
Firefox 86
Type:
defect

Tracking

()

People

(Reporter: info, Unassigned, NeedInfo)

References

(Depends on 1 open bug)

Details

(Keywords: regressionwindow-wanted, Whiteboard: QA-not-reproducible)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/88.0.4324.190 Safari/537.36

Steps to reproduce:

Write a small program which listens to Filesystem.created event in the download folder.

Use firefox and auto download all files into the download folder.

OS: Windows 10

Actual results:

09.03.2021 12:46: Getting Event from Filesystem...
09.03.2021 12:46: Filename: M1-99139_21030211_TNT_EU.txt
09.03.2021 12:46: Getting Event from Filesystem...
09.03.2021 12:46: Filename: M1-99139_21030211_TNT_EU.txt

Expected results:

I expect to retrieve the created event from the filesystem only 1 time.

Firefox 79 works as expected and fires the event once.

Setting a component for this issue in order to get the dev team involved.
If you feel it's an incorrect one please feel free to change it to a more appropriate one.

Component: Untriaged → DOM: Events
Product: Firefox → Core

Don't think it's the appropiate one, is there any list of components?

The Event triggered by Firefox is the Windows Filesystem Created-Event.
Seems like Firefox is creating those downloaded files twice and therefore Windows fires the created event two times.

Write a small program which listens to Filesystem.created event in the download folder.

Could you share the affected small program if you have one, as that would be greatly helpful the investigation? Thanks!

Flags: needinfo?(info)

Sorry for the delay.

Create a .Net-Core Console Application:

using System;
using System.IO;

namespace FirefoxTest
{
    class Program
    {
        static void Main(string[] args)
        {
            // Create new FileSystemWatcher object
            FileSystemWatcher watcher = new FileSystemWatcher();
            // Set path for watcher object
            watcher.Path = "<FilePath>";

            // Register an event which is raised, when new objects are created in the specified folder
            watcher.Created += new FileSystemEventHandler(watcher_Created);
            // Turn on the raising of events
            watcher.EnableRaisingEvents = true;

            Console.ReadLine();
        }

        static void watcher_Created(object sender, FileSystemEventArgs e)
        {
            Console.WriteLine(e.Name);

        }
    }
}

If you Download Files the event is raised twice.

Flags: needinfo?(info)

Peanut gallery observation: this might be happening because of the safe output stream interface.
https://searchfox.org/mozilla-central/source/xpcom/io/nsISafeOutputStream.idl

What happens is that Firefox when using the safe file output stream will write to a temporary location first, then overwrite the actual destination with the temporary file.

So you might simply have to match the file name you wanted exactly.

Matching the filename exactly will not do the job, cause the filename of two created events is the same on our end.

The ouput will show the following:

ABIN129578.pdf
ABIN129578.pdf

Is there any way to "check" if it's a empty file? Earlier the file was created as .pdf.part. Which could be matched by filename.

I've tried it with an exclusive file access check, which doesn't fail on the temporary file.
The other way is to check, if the file has got a size > 0, which is sometime 4096 bytes.

Would be really helpfull if the file handles for the temporary file is kept open until the "replacement" starts.

If the behavior has changed, using https://mozilla.github.io/mozregression/ could hopefully find when the change was done.
Could you try to find the regression range?

Severity: -- → S3
Flags: needinfo?(info)
Keywords: regressionwindow-wanted
Priority: -- → P3
QA Whiteboard: [qa-regression-triage]

I would like to help in providing results of a regression investigation (finding out if this is a new change or not and determining the pushlog that caused it to happen), but unfortunately, I do not have the required skills to apply those steps described or how to use the app in comment 5.

Could you help us investigate? I'll do my best to guide you through useing mozregression app.
Otherwise, maybe you can help me do it by writing some detailed steps I could take in order to properly reproduce this issue.

Awaiting your response! Thanks!

Hi,
my it-service colleague is on vacation until next week.
He will try to provide you the neccessary information.

Flags: needinfo?(info)

Reminder to address comment 10.

Flags: needinfo?(info)

Hsin-Yi Tsai, could you point me to someone who can help me reproduce this issue (how to use the application in commen 5)?

QA Whiteboard: [qa-regression-triage]
Flags: needinfo?(htsai)
Whiteboard: QA-not-reproducible
QA Whiteboard: [qa-regression-triage]

So I did a PTO-mode investigation, in Rust 😛: https://github.com/saschanaz/filesystemwatcher-win-rs/

The behavior changes with and without browser.download.improvements_to_download_panel (which is enabled after the initial report).

Without the flag it shows exactly what the reporter says: the file is added, then removed, and then re-added.

File added
Name: firefox-99.0a1.en-US.linux-i686.awsy.tests(6).tar.gz
File removed
Name: firefox-99.0a1.en-US.linux-i686.awsy.tests(6).tar.gz
File added
Name: firefox-99.0a1.en-US.linux-i686.awsy.tests(6).tar.gz

With the flag enabled it's also kinda similar: It's added first with a temporary name, and then the file with the correct name is added, then is removed, and then the one with the temporary name is renamed with the correct name.

File added
Name: b3zHTnIi.tar.part
File added
Name: firefox-99.0a1.en-US.linux-i686.awsy.tests(5).tar.gz
File removed
Name: firefox-99.0a1.en-US.linux-i686.awsy.tests(5).tar.gz
File renamed from
Name: b3zHTnIi.tar.part
File renamed to
Name: firefox-99.0a1.en-US.linux-i686.awsy.tests(5).tar.gz

Technically this is nothing to do with DOM, so maybe change the component?

Probably a better component since this is related to the download flag. Or XPCOM since the file things are in xpcom/io/?

Component: DOM: Events → Downloads Panel
Product: Core → Firefox

Please triage again, thanks!

Severity: S3 → --
Priority: P3 → --

I have some guesses as to what might cause this (specifically, the code around https://searchfox.org/mozilla-central/rev/3de1b6791ea569cdf7773f7cd512cf2e6cc1d3f0/uriloader/exthandler/nsExternalHelperAppService.cpp#1606-1621 ). What I don't understand is why it is a problem? That is, it is potentially superfluous IO, so from a performance perspective it's not ideal. But I'd like to better understand why this behaviour causes issues for you, because the comments so far don't seem to care about performance, but hint that this is somehow causing correctness issues in other software... without understanding what those are, it's hard for me to be sure what is the right way of addressing this, and/or making sure that other changes to our filesystem management for downloads don't break your stuff in other ways.

That said... we recently created a separate API that wouldn't require the file to exist, in order for this check to work, namely https://searchfox.org/mozilla-central/rev/3de1b6791ea569cdf7773f7cd512cf2e6cc1d3f0/toolkit/components/reputationservice/nsIApplicationReputation.idl#73, so perhaps we can swap that out and it'd resolve the issue here? The difference is that on *nix and macOS, the executable-ness is tied to permissions, for which we'd need to create a file... except that the existing check seems to cause problems there anyway (cf. bug 1664646). So perhaps this would be a positive change regardless...

the other confusion here is that apparently this didn't happen in 79, which is confusing because the code I linked to is much, much older than that. A regression window would still be helpful.

Flags: needinfo?(krosylight)

What I don't understand is why it is a problem?

That's a good question, and I'm honestly not sure. I can only imagine that some sync tools (including cloud storage services e.g. DropBox/OneDrive/etc.) may do redundant works and generate redundant changelog, but I haven't tried that.

The reporter may have some story behind this. (Already NI'ed)

the other confusion here is that apparently this didn't happen in 79, which is confusing because the code I linked to is much, much older than that. A regression window would still be helpful.

I still see this in version 79 from mozregression (and thus cannot provide a good regression window).

File added
Name: firefox-99.0a1.en-US.win32.awsy.tests.tar.gz
File removed
Name: firefox-99.0a1.en-US.win32.awsy.tests.tar.gz
File added
Name: firefox-99.0a1.en-US.win32.awsy.tests.tar.gz
Flags: needinfo?(krosylight)

Hi Guys,
sorry for the latency. Really busy...

Okay, what I'm doing right now:

We are shipping products to customers. For that, we auto-download documents from our website, when we ship.
Those are labels (zpl), invoices, delivery forms (pdf) and so on.
A self-developed software registers as event listener for files created in the download directory and if the filename matches to a regex it's going to perform actions. Like copy into another directory, open acrobat and print, copy to zpl printer.
While the file-system event (created) is received in the software, firefox finished the download process and it's going to perform the action twice.
If the file is a larger one, the first file-event will run into an error and the second one will complete without any errors.
The file is not exclusive opened at that time from firefox. So I don't have any possibility to check if the download is completed, to prevent doing things twice and running into an error.

Flags: needinfo?(info)

(In reply to Benedikt Lenzen from comment #19)

Hi Guys,
sorry for the latency. Really busy...

Okay, what I'm doing right now:

Thanks so much for this explanation!

We are shipping products to customers. For that, we auto-download documents from our website, when we ship.
Those are labels (zpl), invoices, delivery forms (pdf) and so on.
A self-developed software registers as event listener for files created in the download directory and if the filename matches to a regex it's going to perform actions. Like copy into another directory, open acrobat and print, copy to zpl printer.
While the file-system event (created) is received in the software, firefox finished the download process and it's going to perform the action twice.
If the file is a larger one, the first file-event will run into an error and the second one will complete without any errors.
The file is not exclusive opened at that time from firefox. So I don't have any possibility to check if the download is completed, to prevent doing things twice and running into an error.

Maybe I'm naive but can't you check if the filesize of the file is 0 and ignore it if so? That seems like the most straightforward solution...

Flags: needinfo?(info)

As I experienced, it's not everytime exactly 0... But I can check again.

Flags: needinfo?(htsai)

Dan, would you have any concerns/thoughts about switching the executable check here with just nsIApplicationReputationService.isExecutable check, as comment 17 points out?

That would change the outcome of the Save As dialog, that with the new experience we're not showing anymore, to enable Open and OK immediately (it checks .targetFileIsExecutable).
Similarly alwaysAsk would not be enforce to true here https://searchfox.org/mozilla-central/rev/3de1b6791ea569cdf7773f7cd512cf2e6cc1d3f0/uriloader/exthandler/nsExternalHelperAppService.cpp#2025
That means it could potentially be easier on Unix platforms to execute files that have executable flags set, we'd not show anymore our executable warning dialog. On the other side, as comment 17 pointed out, our current approach is creating other problems like Bug 1664646.

Not sure there's a clean solution satisfying both cases, maybe we could take a different approach based on platform.

This doesn't seem like a critical issue anyway, the third party app could be modified to handle this case, maybe awaiting some time when it receives the created event for a deleted event, and then ignoring the event.
Moving to File Handling because the file creation is actually in nsExternalHelperAppService.cpp

Severity: -- → S3
Status: UNCONFIRMED → NEW
Component: Downloads Panel → File Handling
Ever confirmed: true
Priority: -- → P3

Sorry I forgot the needinfo, please check comment 22

Flags: needinfo?(dveditz)

Clear a needinfo that is pending on an inactive user.

Inactive users most likely will not respond; if the missing information is essential and cannot be collected another way, the bug maybe should be closed as INCOMPLETE.

For more information, please visit auto_nag documentation.

Flags: needinfo?(info)

(In reply to Marco Bonardo [:mak] from comment #22)

Dan, would you have any concerns/thoughts about switching the executable check here with just nsIApplicationReputationService.isExecutable check, as comment 17 points out?

It looks like that call only uses the sExecutableExts list from nsLocalFileCommon and skips the (misnamed?) kBinaryFileExtensions list which is where we find the extensions for Linux shells (.sh, .csh, etc) and scripting languages (.py, .rb, .applescript). On Windows if you "open" a file like that (e.g. "foo.bat") from our download list it would have executed it if we didn't have that in the sExecutableExts list. What would happen on Linux if you open a .sh from our list? Assume it has executable permission, since this is proposing we stop checking that.

That would change the outcome of the Save As dialog, that with the new experience we're not showing anymore, to enable Open and OK immediately (it checks .targetFileIsExecutable).

Some people still see the Save As dialog all the time :-) (It's even a user-accessible option in about:preferences)

What happens after the user clicks Open if the file really was an executable? Then we would run it through the full SafeBrowsing application reputation and only fail if that thinks it's malware? Shouldn't that have been done before we get to showing the Save As dialog?

Why would checking .targetFileIsExecutable behave differently from now? If the final name exists it uses that, and if it doesn't it checks mTempFileIsExecutable. I thought in Gijs' suggestion you'd still set that, you'd just do it using the application reputation service rather than creating the file (which does give different results depending on the extension, but generally "works")

Similarly alwaysAsk would not be enforce to true here https://searchfox.org/mozilla-central/rev/3de1b6791ea569cdf7773f7cd512cf2e6cc1d3f0/uriloader/exthandler/nsExternalHelperAppService.cpp#2025

I think mTempFileIsExecutable still gets set, just using a different list.

Flags: needinfo?(dveditz) → needinfo?(mak)
Depends on: 1904024
You need to log in before you can comment on or make changes to this bug.