Unable to select folders/directories with the XUL file picker
Categories
(SeaMonkey :: Download & File Handling, defect)
Tracking
(Not tracked)
People
(Reporter: njsg, Unassigned, NeedInfo)
Details
In SeaMonkey 2.53.3 on GNU/Linux, with ui.allow_platform_file_picker set to false (so that the XUL file picker is used), I can't save two or more files at the same time.
There are at least two ways to reproduce this: in Mail&News by saving two or more attachments at once and in the Navigator Page Info dialog Media tab, also by saving two or more items at the same time.
This may have been introduced by bug 1335539, where the use of wrapDOMFile in nsFilePicker.js was replaced with createFromNsIFile[1]. This seems to fail silently when the file picker is used to choose a directory. Adding a catch() to these promises, I get NS_ERROR_FILE_IS_DIRECTORY.
If I'm understanding the code correctly, CreateFromNsIFile in dom/file/File.cpp calls FileCreatorHelper::CreateFile in FileCreatorHelper.cpp, which calls CreateFileInternal, which calls CreateBlobImpl. This last one calls InitializeChromeFile in MultipartBlobImpl.cpp, which will fail for directories (line 360 for 2.53.3 final).
|
Reporter | |
Comment 1•3 years ago
|
||
Just a note: when this happens in Mail&News, the seamonkey process will start using ~100% CPU and won't terminate after quitting.
|
||
Comment 2•3 years ago
|
||
Reproduced with Mail&News on CentOS-7 .
Strace shows a loop with zero-timeout poll(), followed by two EAGAIN'ed recvfrom() calls.
|
|
||
Comment 3•3 years ago
|
||
Reproduced with Firefox build over SM-2.53.5 tree ("Media" test). Thus not SM-related issue.
The ability to use XUL file picker and the preference "ui.allow_platform_file_picker" was removed in bug #1284391 (FF59 time). So this feature will not be present in upcoming SM-2.57 line anyway.
I beileve all the old problems with gtk file picker should gone nowadays (probably since the switch to gtk3). If something remains for some use cases, a workaround should already exists anyway.
So a time to change habits a bit...
|
||
Comment 4•3 years ago
|
||
As stated the file picker was removed in Gecko 60. Given that it is implemented using rdf and this is going away for sure. It would need to be fully reimplemented. We can leave this bug open if someone wants to fix it for 2.53 only but don't hold your breath. This is basically non default and unsupported.
|
|
||
Comment 5•3 years ago
|
||
It seems there are two issues.
First, selecting a folder/directory under mailer always leads to 100% cpu, even with the platform file picker (gtk on Linux).
Second, selecting folder/directory with the old XUL platform picker does nothing.
Thus, under mailer: 100% cpu + not working with XUL picker, and 100% cpu (which is gone after the final clicking) and all is working with GTK picker.
Under browser: no cpu hogging, XUL picker does not work, GTK picker works.
The issue for mail part was inspired by applying (backporting) the bug #1341211 , which introduces new ShowPicker() method. The problem place is here:
while (!cb->mPickerDone) {
NS_ProcessPendingEvents(thread);
}
When only one attachment is selected to save, the cycle is performed once. When more than one -- the infinite loop is started here.
Note, there is no any similar place anywhere in the whole code -- searching shows that when NS_ProcessPendingEvents() is used inside a loop, it always accompanied by some other locking/timeout stuff...
The simple work-around for mailer is just to revert the change for the SaveAllAttachments() function:
--- comm/mailnews/base/src/nsMessenger.cpp.orig 2021-05-30 03:26:03.245400282 +0300
+++ comm/mailnews/base/src/nsMessenger.cpp 2021-05-30 03:25:46.555542964 +0300
@@ -952,9 +952,9 @@ nsMessenger::SaveAllAttachments(uint32_t
rv = GetLastSaveDirectory(getter_AddRefs(lastSaveDir));
if (NS_SUCCEEDED(rv) && lastSaveDir)
filePicker->SetDisplayDirectory(lastSaveDir);
- rv = ShowPicker(filePicker, &dialogResult);
+ rv = filePicker->Show(&dialogResult);
if (NS_FAILED(rv) || dialogResult == nsIFilePicker::returnCancel)
return rv;
rv = filePicker->GetFile(getter_AddRefs(localFile));
No cpu hog and selecting of several attachments now works. (Both XUL and GTK pickers).
No idea whether it spoils something in other places.
It could be interesting to test whether Thunderbird (with GTK picker) consumes 100% cpu too at some stage when saving several attachments simultaneously.
|
|
||
Comment 6•3 years ago
|
||
rv = filePicker->Show(&dialogResult);
This is the old sync method and only left in 2.53 for add-on compatibility. Removed from 2.57 too. The new is async and the code probably polls if I remember. Other than cpu 100% is something broken here with the platform file picker?
FRG
|
|
||
Comment 7•3 years ago
|
||
(In reply to Frank-Rainer Grahl (:frg) from comment #6)
is something broken here with the platform file picker?
Under Linux platform file picker (ie. GTK) with selecting multiple attachments, right after you type "Save As...", the proper picker window appears, and the same time cpu starts to utilize 100%. It is gone after you type OK button (the files are saved properly anyway).
|
|
||
Comment 8•3 years ago
|
||
That is because the event loop is polled. The ShowPicker simulates the sync file picker:
// Spin the event loop until the callback was called.
nsCOMPtr<nsIThread> thread(do_GetCurrentThread());
while (!cb->mPickerDone) {
NS_ProcessPendingEvents(thread);
}
Still in current TB code. Would need a big refactoring to do it differently.
|
|
||
Comment 9•3 years ago
|
||
Well, platform picker takes 100% cpu even when saving onle only attachment...
Still in current TB code.
Whether TB behaves the same here? Ie. 100% cpu too on a gtk dialog about saving attachment(s).
It is an ugly design of code anyway, I hope TB works properly atleas nowadays (maybe fixed something in some another places)...
|
|
||
Comment 10•3 years ago
|
||
Hmm, yet another work-around for XUL picker (works with both multiple and single case):
--- toolkit/components/filepicker/nsFilePicker.js-OK 2021-05-31 01:20:53.235204204 +0300
+++ toolkit/components/filepicker/nsFilePicker.js 2021-05-31 01:44:27.510542122 +0300
@@ -238,17 +238,17 @@ nsFilePicker.prototype = {
for (let i = 0; i < this.mFilesEnumerator.mFiles.length; ++i) {
if (this.mFilesEnumerator.mFiles[i].exists()) {
let promise =
this.mParentWindow.File.createFromNsIFile(
this.mFilesEnumerator.mFiles[i]).then(file => {
this.mDOMFilesEnumerator.mFiles.push(file);
});
- promises.push(promise);
+// promises.push(promise);
}
}
}
Promise.all(promises).then(() => {
if (aFilePickerShownCallback) {
aFilePickerShownCallback.done(result);
}
Maybe an extra push of a promise (and then an infinite loop somewhere)?
Regarding platform GTK -- why 100% cpu with gtk only, but NOT with XUL picker?
The while (!cb->mPickerDone) {...} cycle is the same for both...
|
|
Reporter | |
Comment 11•3 years ago
|
||
I may be mistaken, and I need to test this again, but I think that, at that point, the problem is that the promise never completes (as InitializeChromeFile() fails), and so .done() is never executed.
|
|
||
Comment 12•3 years ago
|
||
Probably something like this should be most correct:
--- toolkit/components/filepicker/nsFilePicker.js-OK 2021-05-31 01:20:53.235204204 +0300
+++ toolkit/components/filepicker/nsFilePicker.js 2021-05-31 03:17:28.798491377 +0300
@@ -216,9 +216,9 @@ nsFilePicker.prototype = {
let promises = [];
// Let's create the DOMFileEnumerator right now because it requires some
// async operation.
- if (this.mFilesEnumerator) {
+ if (this.mFilesEnumerator && this.mMode == nsIFilePicker.modeOpenMultiple) {
this.mDOMFilesEnumerator = {
QueryInterface: XPCOMUtils.generateQI([Ci.nsISimpleEnumerator]),
mFiles: [],
It seems that "mDOMFilesEnumerator" is used by domFileOrDirectoryEnumerator() only, and there is a note that it "only works in the modeOpenMultiple mode" .
Besides that, there is some mention that "we don't support directories, yet" for a related "domFileOrDirectory()".
Anyway, disabling the correspond block such a way seems to fix the issue with XUL picker.
Could be fine if anybody more familiar with such a stuff will look on it further...
|
|
Reporter | |
Comment 13•3 years ago
|
||
With the change from Comment 12, I can now save multiple files using the XUL file picker in 2.53.9b1pre 20210530141020 (be it attachments in M&N or images in the page info media tab).
|
|
||
Comment 14•3 years ago
|
||
Could you test whether the changed XUL picker works properly in other situations?
For now, it works with "modeSave" (when one file) and "modeGetFolder" (selecting a folder when several files to save). Probably trying to add one or more attachments to a sending mail will touch the other modes "modeOpen" and "modeOpenMultiple".
Just to make sure whether the problem code block is needed at all, and if so -- for what modes exactly.
|
|
||
Comment 15•3 years ago
|
||
It seems that if (this.mFilesEnumerator && false) {will work too.
Still do not find anything not working when this block is commented out completely. (At least sending email with one or more attachments works fine).
Perhaps this is some extra code introduced "just in case", that became toxic after a series of subsequent changes?
|
|
||
Comment 16•3 years ago
|
||
this.mDOMFilesEnumerator was introduced in a commit of the bug #832923, when they played with b2g (what was it?). Probably that change was needed for b2g only, and no more needed since b2g is gone.
So maybe the removing of if (this.mFilesEnumerator) { block completely is a good idea, but then you need to test all the <input type="file" possible combinations anywhere.
Thus either remove it completely, or leave for some modes only (kinda "modeOpenMultiple" if some tests with <input type="file"> will fail etc.).
In the mail attachments context, all works fine without this block in all possible combinations I can find for now.
|
|
||
Comment 17•3 years ago
|
||
Something is wrong with GTK file picker.
Both Win/Mac widget/{cocoa,windows}/nsFilePicker.* have no explicit ::Open() method definiton.
Thus when ShowPicker() calls aPicker->Open(), it actually calls widget/nsBaseFilePicker.cpp:nsBaseFilePicker::Open(), which creates an event and dispatches it to the main thread. Event's AsyncShowFilePicker() properly calls the correspond ::Show() method. The whole algorithm seems clean enough.
The same way is used by the old XUL picker: the open method creates a stuff (which then used "show") and dispatches it to the main thread. (Certainly don't forget to remove the problem block there first :) )
GTK file picker uses a different way. Not "Open() uses Show()", but "Show() runs Open()" and wait for g_main_context_iteration() (which probably explains why the change in the comment 5 drops the cpu hog). The Open() itself does not any dispatching to the main thread etc.
Thus, on Windows, Mac and old XUL, the proper event is created by the ShowPicker()'s aPicker->Open() call. Then the loop:
while (!cb->mPickerDone) {
NS_ProcessPendingEvents(thread);
}
is run just once, because NS_ProcessPendingEvents() runs this event, which then properly wait for the user input, and then !cb->mPickerDone become false.
On GTK systems, "aPicker->Open()" does not creates such an event. Instead, it shows the file chooser immediately and relies on some standard internal gtk/glib mechanisms ("response" callback ). Then the loop with NS_ProcessPendingEvents() runs all the time until the glib library catch the "response" from user and "!cb->mPickerDone" become false.
This issue does not appear when such GTK's Open() method is called throw javascript (in construction like "filePicker.open(rv => {...something...})". Thus it does not appear when add attachments to the sending mail (as well as there are no related bugs reported for browser part etc.).
So, in short: Saving attachments on GTK systems leads to 100% cpu usage while the file chooser is shown to the user. The reason is the way how widget/gtk/nsFilePicker.cpp:nsFilePicker::Open() method was implemented, and not so optimal way of how comm/mailnews/base/src/nsMessenger.cpp:ShowPicker() function tryes to handle it.
It could be very fine to try to reproduce this issue with Thunderbird.
|
|
||
Comment 18•3 years ago
|
||
Bug #1714222 filed for GTK issue.
|
|
||
Comment 19•3 years ago
|
||
NS_ProcessPendingEvents(thread) is actually NS_ProcessPendingEvents(thread, PR_INTERVAL_NO_TIMEOUT), and calls internally ProcessNextEvent() with the "false" aMayWait argument. According to backtracing, this "false" then goes further up to underlying g_main_context_iteration() call.
OTOH, in the widget/gtk/nsFilePicker.cpp:nsFilePicker::Show() method implementation, g_main_context_iteration() is called with true.
Since there is no 100% cpu usage with work-around from comment 5, I would try:
--- comm/mailnews/base/src/nsMessenger.cpp.orig 2021-06-03 02:28:44.359765243 +0300
+++ comm/mailnews/base/src/nsMessenger.cpp 2021-06-03 02:29:20.381458622 +0300
@@ -341,7 +341,7 @@
// Spin the event loop until the callback was called.
nsCOMPtr<nsIThread> thread(do_GetCurrentThread());
while (!cb->mPickerDone) {
- NS_ProcessPendingEvents(thread);
+ NS_processNextEvent(thread, true);
}
*aResult = cb->mResult;
|
|
||
Comment 20•3 years ago
|
||
Sorry for a typo in the previoud comment :)
But anyway, exactly this change fixes the issue:
--- comm/mailnews/base/src/nsMessenger.cpp.orig 2021-06-03 02:28:44.359765243 +0300
+++ comm/mailnews/base/src/nsMessenger.cpp 2021-06-03 02:29:20.381458622 +0300
@@ -341,7 +341,7 @@
// Spin the event loop until the callback was called.
nsCOMPtr<nsIThread> thread(do_GetCurrentThread());
while (!cb->mPickerDone) {
- NS_ProcessPendingEvents(thread);
+ NS_ProcessNextEvent(thread, true);
}
*aResult = cb->mResult;
Now no more 100% cpu usage while GTK file choose window is shown.
Besides that, no more 100% cpu usage with old XUL picker too. Certainly without the removing of the problem extra block (see comment 16) it still does not work when a directory (for saving multiple attachments) should be selected. But now it behaves the same manner as in browser (when saving media parts) -- ie. does not save anything, except SM remains running after the exit (and should be killed manually), but now without cpu hog.
|
|
||
Comment 21•3 years ago
|
||
There is yet another independent issue with downloads of attachments. Error console shows message like:
Timestamp: 6/6/21, 4:16:12 AM GMT+3
Error: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIAnnotationService.setPageAnnotation]" nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: resource://gre/modules/DownloadHistory.jsm :: updateMetaData :: line 112" data: no]
for each saved attachment.
Tracking the code shows that the problem is a false check here in toolkit/components/places/nsAnnotationService.cpp in StartSetAnnotation() (called from SetAnnotationStringInternal()). Some logging shows that when we save something from browser, SetAnnotationStringInternal() is called twice -- first with aName == "downloads/destinationFileURI", second with aName == downloads/metaData" . But when saving attachment(s), only the second one is in use. So probably something remains uninitialized in this case.
|
|
||
Comment 22•3 years ago
|
||
Well, there is toolkit/components/places/nsNavHistory.cpp:CanAddURI(), called from toolkit/components/places/History.cpp:CanAddURI(), which prevents urls with schemes like "imap:", "mailbox:" etc. to be stored in the places history (NOT download history). This explains why aName == "downloads/destinationFileURI" above is not appeared when saving mail attachments.
But the comm branch has a call to DownloadHistory.updateMetaData(), which seems performed without this check. So it always tries to call setPageAnnotation() with aName == "downloads/metaData", which produces the exception because nothing was stored for such urls in the places.sqlite.
To avoid the issue, anadditional canAddURI() check should be added:
--- comm/suite/components/downloads/DownloadsCommon.jsm 2010-01-01 00:00:00.000000000 +0300
+++ comm/suite/components/downloads/DownloadsCommon.jsm-OK 2021-06-26 00:42:23.345370248 +0300
@@ -34,6 +34,7 @@ XPCOMUtils.defineLazyModuleGetters(this,
NetUtil: "resource://gre/modules/NetUtil.jsm",
PluralForm: "resource://gre/modules/PluralForm.jsm",
DownloadHistory: "resource://gre/modules/DownloadHistory.jsm",
+ PlacesUtils: "resource://gre/modules/PlacesUtils.jsm",
Downloads: "resource://gre/modules/Downloads.jsm",
DownloadUIHelper: "resource://gre/modules/DownloadUIHelper.jsm",
DownloadUtils: "resource://gre/modules/DownloadUtils.jsm",
@@ -704,10 +705,11 @@ DownloadsDataCtor.prototype = {
// Store the end time that may be displayed by the views.
download.endTime = Date.now();
- // This state transition code should actually be located in a Downloads
- // API module (bug 941009).
- // This might end with an exception if it is an unsupported uri scheme.
- DownloadHistory.updateMetaData(download);
+ if (PlacesUtils.history.canAddURI(PlacesUtils.toURI(download.source.url))) {
+ // This state transition code should actually be located in a Downloads
+ // API module (bug 941009).
+ DownloadHistory.updateMetaData(download);
+ }
if (download.succeeded) {
this.playDownloadSound();
No more exception then.
(The alternate way is to put this check immediately into toolkit/components/jsdownloads/src/DownloadHistory.jsm:updateMetadata(), but then we alter a moz branch without a proper need for this).
|
|
||
Comment 23•3 years ago
|
||
Yet another issue.
Try to save several attachments simultaneously when some of them was already saved previously.
When the next "file already exists" message appears, the progress window for the previous file download remains unclosed, and overrides the "already exists" prompt. When you choose the prompt and type OK, the stalled window closed too.
In the case of full "download manager" window, the previous download is shown unfinished until you type OK in the prompt for the next file.
The reason is probably a wrong order in comm/mailnews/base/src/nsMessenger.cpp:nsSaveMsgListener::OnStopRequest(), since it seems that m_messenger->PromptIfFileExists() is called before the previous download is finished by mTransfer->OnStateChange().
Could be interesting whether it can be reproduced by Thunderbird.
|
|
||
Comment 24•3 years ago
|
||
Yes, the fixing of the wrong order resolves the issue:
--- comm/mailnews/base/src/nsMessenger.cpp.orig 2021-06-03 02:28:44.359765243 +0300
+++ comm/mailnews/base/src/nsMessenger.cpp 2021-06-27 01:17:37.124375314 +0300
@@ -1930,6 +1930,14 @@ nsSaveMsgListener::OnStopRequest(nsIRequ
m_outputStream = nullptr;
}
+ if(mTransfer)
+ {
+ mTransfer->OnProgressChange64(nullptr, nullptr, mMaxProgress, mMaxProgress, mMaxProgress, mMaxProgr
ess);
+ mTransfer->OnStateChange(nullptr, nullptr, nsIWebProgressListener::STATE_STOP |
+ nsIWebProgressListener::STATE_IS_NETWORK, NS_OK);
+ mTransfer = nullptr; // break any circular dependencies between the progress dialog and use
+ }
+
if (m_saveAllAttachmentsState)
{
m_saveAllAttachmentsState->m_curIndex++;
@@ -1994,14 +2002,6 @@ nsSaveMsgListener::OnStopRequest(nsIRequ
}
}
- if(mTransfer)
- {
- mTransfer->OnProgressChange64(nullptr, nullptr, mMaxProgress, mMaxProgress, mMaxProgress, mMaxProgr
ess);
- mTransfer->OnStateChange(nullptr, nullptr, nsIWebProgressListener::STATE_STOP |
- nsIWebProgressListener::STATE_IS_NETWORK, NS_OK);
- mTransfer = nullptr; // break any circular dependencies between the progress dialog and use
- }
-
if (mUrlHasStopped && mListener)
mListener->OnStopRunningUrl(mListenerUri, rv);
|
|
||
Comment 25•3 years ago
•
|
||
I am unable to reproduce the 100% cpu under Windows so it seems to be a gtk only bug. Using NS_ProcessNextEvent(thread, true); probably polls endlessly and is not something I would want for all os. If a patch is attached could be made conditionally for Linux only.
For the other issues really hard to follow so if not done already separate bugs should be opened.
Description
•