Bug 344606

How does AR react if you delete the file it has opened?
(Because that'd be the next step - if it segfaults, that's probably a no-go solution)

Seems like https://bugreports.qt.io/browse/QTBUG-10856 is very relevant and I'm afraid Qt doesn't provide a convenient way here. Got some suggestions on how to handle this correctly?

gerrit as soon as we know this is actually a possible solution (ie. Adobe's bloatware doesn't happily abort if you delete the file)

It basically deletes the tmpfile object at once (causing a close), then opens the file and schedules the file removal (only) - afaics, there's no other (portable, no ::close(FD) on windows) way for this.

----------------------

diff --git a/src/Common/DeleteAfter.cpp b/src/Common/DeleteAfter.cpp
index 593b69a..9eacf9b 100644
--- a/src/Common/DeleteAfter.cpp
+++ b/src/Common/DeleteAfter.cpp
@@ -22,15 +22,32 @@
 
 #include "DeleteAfter.h"
 #include <QCoreApplication>
-#include <QTimer>
+#include <QFile>
 
 namespace Common
 {
 
-DeleteAfter::DeleteAfter(QObject *child, const int msec): QObject(qApp)
+DeleteAfter::DeleteAfter(QObject *o, const int msec): QTimer(qApp)
+, m_object(o)
 {
-    child->setParent(this);
-    QTimer::singleShot(msec, this, SLOT(deleteLater()));
+    m_object->setParent(this);
+    connect(this, SIGNAL(timeout()), SLOT(deleteLater()));
+    start(msec);
+}
+
+DeleteAfter::DeleteAfter(QString path, const int msec): QTimer(qApp)
+, m_path(path)
+, m_object(0)
+{
+    connect(this, SIGNAL(timeout()), SLOT(deleteLater()));
+    start(msec);
+}
+
+DeleteAfter::~DeleteAfter()
+{
+    delete m_object;
+    if (!m_path.isEmpty())
+        QFile::remove(m_path);
 }
 
 }
diff --git a/src/Common/DeleteAfter.h b/src/Common/DeleteAfter.h
index 1a36cc4..7254dfe 100644
--- a/src/Common/DeleteAfter.h
+++ b/src/Common/DeleteAfter.h
@@ -23,17 +23,22 @@
 #ifndef TROJITA_COMMON_DELETEAFTER_H
 #define TROJITA_COMMON_DELETEAFTER_H
 
-#include <QObject>
+#include <QTimer>
 
 namespace Common
 {
 
-/** @short Delete a QObject after a specified timeout */
-class DeleteAfter: public QObject
+/** @short Delete a QObject or File after a specified timeout */
+class DeleteAfter: public QTimer
 {
     Q_OBJECT
 public:
-    DeleteAfter(QObject *child, const int msec);
+    DeleteAfter(QObject *object, const int msec);
+    DeleteAfter(QString path, const int msec);
+    ~DeleteAfter();
+private:
+    QString m_path;
+    QObject *m_object;
 };
 
 }
diff --git a/src/Gui/AttachmentView.cpp b/src/Gui/AttachmentView.cpp
index 38565a9..77a2ec5 100644
--- a/src/Gui/AttachmentView.cpp
+++ b/src/Gui/AttachmentView.cpp
@@ -213,6 +213,9 @@ void AttachmentView::slotFileNameRequestedOnOpen(QString *fileName)
     Q_ASSERT(!m_tmpFile);
     m_tmpFile = new QTemporaryFile(QDir::tempPath() + QLatin1String("/trojita-attachment-XXXXXX-") +
                                    fileName->replace(QLatin1Char('/'), QLatin1Char('_')));
+    // NOTICE: the file must NOT be autoremoved w/ deletion, because we need to delete it
+    // in order to have the FH closed, bug #344606
+    m_tmpFile->setAutoRemove(false);
     m_tmpFile->open();
     *fileName = m_tmpFile->fileName();
 }
@@ -242,12 +245,18 @@ void AttachmentView::openDownloadedAttachment()
     // Make sure that the file is read-only so that the launched application does not attempt to modify it
     m_tmpFile->setPermissions(QFile::ReadOwner);
 
-    QDesktopServices::openUrl(QUrl::fromLocalFile(m_tmpFile->fileName()));
+    const QUrl url = QUrl::fromLocalFile(m_tmpFile->fileName());
 
-    // This will delete the temporary file in ten seconds. It should give the application plenty of time to start and also prevent
-    // leaving cruft behind.
-    new Common::DeleteAfter(m_tmpFile, 10000);
+    // NOTICE: we need to delete the file *first* so that the handle is closed before
+    // we open it w/ a system program
+    // the file is NOT automagically removed for setAutoRemove(false) on creation, bug #344606
+    delete m_tmpFile;
     m_tmpFile = 0;
+    QDesktopServices::openUrl(url);
+
+    // This will delete the temporary file in ten seconds.
+    // It should give the application plenty of time to start and also prevent leaving cruft behind.
+    new Common::DeleteAfter(url.path(), 10000);
     m_openAttachment->setEnabled(true);
 }

Adobe Reader doesn't crash, but it prevents you from deleting the file by locking it on its own. This also applies to Adobe Acrobat. You actually have to wait until the PDF is closed in the viewer.

(In reply to bugs.kde.org from comment #4)
> You actually have to wait until the PDF is closed in the viewer.
*That* would have to be deemed a "other side" bug, I fear.

We can (obviously) not guarantee to have Trojitá still running when you close the reader - and I assume Jan will refuse trying to delete the file on a poll basis ;-)

(In reply to Thomas Lübking from comment #3)
> afaics, there's no other (portable, no ::close(FD) on windows) way for this.

Microsoft is always funny in this way and all such functions starts with underscore. So try to call _close(FD)

(In reply to bugs.kde.org from comment #4)
> Adobe Reader doesn't crash, but it prevents you from deleting the file by
> locking it on its own. This also applies to Adobe Acrobat. You actually have
> to wait until the PDF is closed in the viewer.

And what happen if I try to delete file? Application crash or what? Looks like windows implement also unlink unix equvalent via _unlink(path) function... Will unlink work?

(In reply to Jan Kundrát from comment #2)
> Seems like https://bugreports.qt.io/browse/QTBUG-10856 is very relevant and
> I'm afraid Qt doesn't provide a convenient way here. Got some suggestions on
> how to handle this correctly?

Anyway, this is just windows related problem, right? And qt does not want to fix? Then I think #ifdef Q_OS_WIN32 with some windows magic could be used...

(In reply to Thomas Lübking from comment #5)
> (In reply to bugs.kde.org from comment #4)
> > You actually have to wait until the PDF is closed in the viewer.
> *That* would have to be deemed a "other side" bug, I fear.

As a programmer, I understand your point of view. I'd say it's a missing feature in the Windows file system routines. The Reader has to keep a handle on the file to avoid having to load large PDFs into main memory all at once, so it isn't negotiable on their side. But deleting a file which is currently in use is just not allowed by Windows.

As a user, I locate the bug in the program which behaves different from what I know from similar programs. And most programs under Windows just dump their temporary downloads to the temp directory and don't care about cleaning up. Given the ratio between typical drive sizes and typical email attachments, this goes unnoticed for a long time. And as soon as the drive fills up, Windows suggests deleting all the temporary files automatically.

This feels wrong to software developers who are used to manage their own resources, but not being able to view PDFs from an email client feels even more wrong to users... Consider it an awkward kind of garbage collector if you need a justification for supporting this behavior under Windows.

(In reply to Pali Rohár from comment #6)
> (In reply to bugs.kde.org from comment #4)
> > Adobe Reader doesn't crash, but it prevents you from deleting the file by
> > locking it on its own. This also applies to Adobe Acrobat. You actually have
> > to wait until the PDF is closed in the viewer.
> 
> And what happen if I try to delete file? Application crash or what?

Windows won't let you delete it. This might seem strange for linux developers, but that's how it works on this platform. You have to wait until all file handles are closed before you can delete, rename or move a file. If you really want to delete a file, register a task to delete it on the next reboot... But this is mostly used by setup programs to deinstall drivers and such, it's not common for applications. They just leave the files behind and don't care.

(In reply to Pali Rohár from comment #6)
> Microsoft is always funny in this way and all such functions starts with
> underscore. So try to call _close(FD)

Actually, MS considers _close(FD) ISO:
"The POSIX name for this item is deprecated. Instead, use the ISO C++ conformant name: _close"

However, it's probably not ideal to close the file bypassing QTemporaryFile.
 
> like windows implement also unlink unix equvalent via _unlink(path)
> function... Will unlink work?
Seems _unlink() is equivalent to ::DeleteFile (what QFile ultimately calls)
http://stackoverflow.com/questions/27270374/deletefile-or-unlink-calls-succeed-but-doesnt-remove-file
 
(In reply to bugs.kde.org from comment #7)
>  most programs under Windows just dump their temporary downloads to the temp directory 
> and don't care about cleaning up.

This is what this patch would end up doing on Windows, yes.
Sounds totally stupid to me ;-)

-------
Interestingly, the flash plugin does (have used to?) delete temporary files right after creation and relies on the inodes being preserved by the system until it's closed (what allowed one to store any flash movie out of /proc ;-)
I always though that was their windows "trick" to stash the file (and they just ignored it wouldn't work on linux)

There's a patch which implements this at [1]. An example installer lives at [2].

[1] https://gerrit.vesnicky.cesnet.cz/r/943
[2] http://ci-logs.kde.flaska.net/binaries/trojita/win32/check/Trojita-Setup-945-a621aef9ce924861bfe9f0faf5536dda.exe

Git commit 4cecef196024ffb31fc6d938ea616d17834c2d6b by Jan Kundrát.
Committed on 10/09/2017 at 14:38.
Pushed by gerrit into branch 'master'.

Do not auto remove direct-opened attachments

Previously, Trojita tried to wait for a while and remove the temporary
file on disk 10 seconds after the application has been launched. That
was a possible race, and worse, some applications attempt to be smart
and detect this file removal and somehow handle that. An example is
Gwenview which auto-switches to a next file in the directory. Another
example is Ark which won't let you extract/preview additional files from
the archive once the downloaded file has been removed.

It's 2017, and it seems that we can now mostly rely on a system behaving
"sanely" and cleaning /tmp automatically. That's probably better than a
random ad-hoc timeout of 10 seconds which we used to have in Trojita.

Another bonus is that this fixes an interoperability problem with Adobe
Reader on Windows. That app complained when the file to be openened was
locked by another application -- Trojita in this case.
Related: bug 316981
Change-Id: Ic150c6d2976ac1a414a6b34a41008aed104c7098

M  +2    -5    src/Gui/AttachmentView.cpp

https://commits.kde.org/trojita/4cecef196024ffb31fc6d938ea616d17834c2d6b