Open Bug 682838 Opened 13 years ago Updated 2 years ago

Unable to open/save from/to GIO remote location

Categories

(Core :: Networking: File, defect, P5)

Other Branch
All
Linux
defect

Tracking

()

ASSIGNED
mozilla14

People

(Reporter: jhorak, Assigned: jhorak)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [necko-would-take])

Attachments

(6 files, 12 obsolete files)

8.29 KB, patch
Details | Diff | Splinter Review
14.02 KB, patch
Details | Diff | Splinter Review
34.97 KB, patch
Details | Diff | Splinter Review
34.26 KB, patch
Details | Diff | Splinter Review
78.04 KB, patch
Details | Diff | Splinter Review
114.42 KB, patch
Details | Diff | Splinter Review
GIO remote locations are not showed in file open/save dialog so user is not allowed to open and save to remote location (like sftp://, smb://, obex:// etc) mounted by GIO.
This bug is still valid and I'm thinking how to fix it. The FilePicker dialog return nsILocalFile. This file differs from GIO file in many ways. You can't get FILE or PRFileDesc of file on remote host (thru GIO). Also load() doesn't make a sense with files stored on remote host. However nsGIOFile can be derived from nsIFile, all required methods can be bound to GIO functions.
Problem is that FilePicker returns nsILocalFile (by GetFile method) and not nsIFile. So I'm unsure how to proceed with implementation of GIO for Mozilla products.

Two approaches came into my mind:
1. Derive nsGIO from nsIFile, make GetFile return nsIFile, rework all usage of FilePicker class by checking whenever nsIFile is nsILocalFile or nsGIOFile.
2. Derive nsGIOFile from nsILocalFile, mark irrelevant methods of parent class nsILocalFile as not implemented, in some cases change FilePicker usage to check if nsILocalFile returned by GetFile() can be cast to nsGIOFile. However I'm not sure if nsILocalFile methods are required for open/save dialog, setting download location, import/export bookmarks, etc.

Ideas welcomed.
Product: Firefox → Core
QA Contact: file.handling → file-handling
Blocks: 378425
I've managed to create a patch finally for which I'd like to get a feedback. I've used nsILocalFile as base for nsGIOFile. I also implemented nsGIOOutputStream and fixed nsGIOInputStream a bit.
Assignee: nobody → jhorak
Status: NEW → ASSIGNED
Attachment #607138 - Flags: feedback?(karlt)
Hi, thanks for the work regarding this bug. Is it already clear in what version of Thunderbird the patch gets?
I am looking forward to a solution for Bug 378425 which depends on this one here.
Karl, could you let me know if this approach is valid (wip patch 1), please? I'm not sure if I should continue working on this by this way. There is still some work to do, like fixing download manager. This feature is wanted by our user base, so that's why I'm bothering you again.
I'm finding it hard to provide feedback here as I'm not familiar with the relevant interfaces.

Some initial comments:

Since bug 682360, nsIFile and nsILocalFile are the same interface.

Part of this bug is very similar to bug 121059, where there is a patch using quite a different approach, but that patch didn't get to a state that was considered acceptable because it doesn't yet avoid blocking the main thread on network activity.

Bug 412822 added the nsIURL interface to nsIFilePicker together with filterAllowURLs to deal with the misfit with nsILocalFile or nsIFileURL.

(In reply to jhorak from comment #4)
> This feature is wanted by our user base

Can you describe the most common situation(s) where this feature is desired, please?
Is it downloading to an SMB network file system that is not mounted, or downloading via obex to mobile device, or something else?
Component: File Handling → Networking: File
Depends on: 121059
QA Contact: file-handling → networking.file
Target Milestone: --- → mozilla14
Version: Trunk → Other Branch
@Karl: Since this bug blocks bug 378425 which I am very much interested in, I describe a typical situation where a solution is absolutely necessary: 
In many working groups most (or all) of the relevant documents are stored in an SMB network file system. Hence, attaching files to an email from a SMB share and saving an attachment to a SMB share is an absolute must. 
I look forward to a solution.
Attachment #607138 - Flags: feedback?(karlt) → feedback?(bzbarsky)
So my gut reaction is that having those ifdefs all over is pretty suboptimal and that this will completely fall down as soon as someone tries to use a filepicker from JS.

nsIFile and nsILocalFile _used_ to be separate precisely because the former could be a remote file as here...  But of course all consumers probably assumed it was an nsILocalFile anyway...

I'm really not sure what a sane setup here is.  :(  Benjamin, any ideas?
The current form of nsIFile is only intended for files which can be opened by the OS using open() or equivalent.

I lack any context about what kind of UI maps to GIO remote locations are or whether we want to support that. It seems that other OSes (Windows and Linux) actually mount network shares to the local filesystem (either via SMB paths like \\server\foo or by actually mounting them to Z:/ or to /Volumes).

If this "thing" on Linux is actually using URIs instead of files, then we shouldn't try to make the nsIFile API fit it. But it's also completely unclear to me why we would need "setSupportsFileURL" in this patch.
I'm not proud of setSupportsFileURL. It is kind of a hack which forces nsStandardURL to treat nsGIOFile as nsILocalFile. Without this nsStandardURL couldn't be cast to nsIFileURL and call GetFile() or SetFile which is later used for uploading files from html form. See:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsStandardURL.cpp#971
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLInputElement.cpp#415

About Linux and GIO situation: There's fallback mode for applications which doesn't support GIO directly. It's in $HOME/.gvfs directory and it is mostly disliked by users (too slow, etc). There's strong tendency to port all GTK apps to GIO.
See:
https://bugzilla.redhat.com/show_bug.cgi?id=668448
https://bugzilla.redhat.com/show_bug.cgi?id=666929
GIO has now api for pretty much everything for file manipulation on remote locations. For users it allows to work seamlessly with local or remote located files without actually noticing it. That's what https://bugzilla.mozilla.org/show_bug.cgi?id=378425 is all about.

I can rework the patch completely, but I need some guidance here. I'm not sure where to start different approach.
And thanks for feedback!
I think the takeaway here is that nsGIOFile is almost certainly the incorrect solution to this problem, since it will not behave like files and in general we assume that all nsIFile are nsLocalFile (and we even static_cast then in the impl, IIRC). We should be doing something like bug 121059 and just treating all of these as URLs.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> It seems that other OSes (Windows and
> Linux) actually mount network shares to the local filesystem (either via SMB
> paths like \\server\foo or by actually mounting them to Z:/ or to /Volumes).

I assume the reason to use URLs and GIO/GVFS instead of a kernel mount is that authentication is user-specific and doesn't need privileges at system level.
Comment on attachment 607138 [details] [diff] [review]
wip patch 1 - may be close to final

I'm going to call this feedback-, for lack of anything better...
Attachment #607138 - Flags: feedback?(bzbarsky) → feedback-
So after looking into sourcecode I've found out this:
- Opening gio locations is just fine. This is solved by gio extension already. It only requires to add filterAllowURLs to FilePicker filters in couple of .js files. I also need to fix LastOpenDirectory

- To upload files from gio locations I need to rework nsDOMFileFile like it was proposed in bug 121059. But I'm not sure about 'sync-loading' thing mentioned in bug (that's why it didn't get a review). Is loading of remote content done in main thread so it blocks UI?

- To save page/image/whatever to gio location I will have to rework nsDownloadManager and contentUtils.js. The nsDownloadManager expect that target nsURI is also nsIFileURL which works only for 'file://...' specs.

There should be more changes in Thunderbird, I'll check that later.

Thanks for feedbacks.
> Is loading of remote content done in main thread so it blocks UI?

nsDOMFile::GetAsBinary has to do just that, yes: block UI until the data is loaded.

I believe the real issue with the sync bits was that the patch in bug 121059 spun an event loop to make an async load look like a sync one, and could therefore cause code to be reentered that did not expect to be reentered....  Precisely because network access does NOT normally block UI, while the web-exposed API there is expected to do so.

Or put another way, the DOM File API expects that files are just that: files.
I'm working on saving content to GIO location and I stuck on nsWebBrowserPersist::MakeOutputStreamFromURI, particulary here:
http://mxr.mozilla.org/mozilla-central/source/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp#2332

I'd like to create nsGIOOutputStream here instead of nsIStorageStream just for case we're trying to save to GIO location. But I miss the right parent class which allows me to init output stream with nsIURI. Using nsIFileOutputStream makes no sense now since it requires nsIFile in Init() method. Any hint here? Should I create new subclass of nsIOutputStream like nsINetworkOutputStream with Init(nsIURI) and inherit nsGIOOutputStream from it or is there better approach?
Well, one option would be to simply implement upload (in the nsIUploadChannel sense) for GIO channels, and then that code would Just Work, I think.  The only caveat is that it would buffer up the data in memory before calling AsyncOpen() on your channel and setting the upload stream.

But yes, the other option is to change MakeOutputStream or MakeOutputStreamFromURI to just create some subclass of nsIOutputStream (however you decide to do that) and initialize it with your URI (however you do that).  I'm not sure whether this is better, since presumably writes to this stream would still either buffer up in memory or block on network, right?  I guess you could read the data out of the stream async on a separate thread or something....
Attaching first version of patch which allows to save to remote locations (GIO). What doesn't work is remembering last save dir for GIO locations.
Attachment #607138 - Attachment is obsolete: true
Attachment #628323 - Flags: review?(bzbarsky)
Ops, removed some irrelevant portions.
Attachment #628323 - Attachment is obsolete: true
Attachment #628323 - Flags: review?(bzbarsky)
Attachment #628325 - Flags: review?(bzbarsky)
I'm probably not going to get to this today, and you'll need another reviewer for the browser/ changes.

That said, from a quick glance, why are you hand-writing QueryInterface functions instead of using NS_IMPL_ISUPPORTS macros?
(In reply to Boris Zbarsky (:bz) from comment #20)
> I'm probably not going to get to this today, and you'll need another
> reviewer for the browser/ changes.
> 
> That said, from a quick glance, why are you hand-writing QueryInterface
> functions instead of using NS_IMPL_ISUPPORTS macros?

Because in:
https://bugzilla.mozilla.org/attachment.cgi?id=628325&action=diff#a/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp_sec2
I'm doing QueryInterface of NS_GIOPROTOCOL_HANDLER_IMPL_IID to determine if I'm handling gio protocol handler and should create nsGIOOutput stream instead of nsIStorageStream.
I don't know how to do it more clear by using macros.
But you actually added an IID to nsGIOProtocolHandler, no?  So you should just be able to do:

  NS_IMPL_ISUPPORTS3(nsGIOProtocolHandler, nsIProtocolHandler, nsIObserver,
                     nsGIOProtocolHandler)

I would think.
Comment on attachment 628325 [details] [diff] [review]
Patch which allows to save files/page to gio location 1

>+++ b/browser/components/downloads/content/downloads.js

Like I said, this needs review from a browser peer.  Maybe Gavin?

>+++ b/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp
>+ * This is helper method for creating nsINetworkOutputStream for saving files to
>+ * remote locations. Only GIO backend is supported right now. At first it checks
>+ * if aURI's scheme is supported by nsGIOProtocolHandler.

So I think this would be less hardcoded for GIO if we required that protocol handlers that want to enable network output streams just implement an interface which hands out an nsIOutputStream given a URI.  Then this code could get the protocol for the given scheme, QI to this interface, and get the output stream.  No hardcoding of GIO involved.

>+  nsIProtocolHandler *tmp_handler;
>+  rv = handler->QueryInterface(NS_GIOPROTOCOL_HANDLER_IMPL_IID, (void **) &tmp_handler);

An no manual QI calls involved.  Which would be good, since this call here leaks the handler when the QI succeeds...

>+  nsCOMPtr<nsISupports> output_stream = do_CreateInstance(NS_NETWORK_PROTOCOL_CONTRACTID_PREFIX"moz-gio-output-stream", &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsINetworkOutputStream> network_os = do_QueryInterface(output_stream, &rv);

For future reference, this could have been written simply as:

  nsCOMPtr<nsINetworkOutputStream> network_os = do_CreateInstance(NS_NETWORK_PROTOCOL_CONTRACTID_PREFIX"moz-gio-output-stream", &rv);

because do_CreateInstance will do a QI to the target interface anyway.

> nsWebBrowserPersist::MakeOutputStreamFromURI(
>+    if (MakeNetworkOutputStreamFromURI(aURI, aOutputStream) == NS_OK)

Probably better to test NS_SUCCEEDED.

>+++ b/extensions/gio/nsGIOProtocolHandler.cpp
>+class nsGIOOutputStream: public nsINetworkOutputStream

I'm not really competent to review the gtk bits here.  Maybe karlt?

That said, a few general comments:

1)  Does calling write() and the like after close() return an error or crash?  It should do the former, so if the various GTK bits are not safe with a null mGIOStream we should be null-checking it up front.

2)  You probably do want an implementation of WriteFrom at some point.

3)  Returning NOT_IMPLEMENTED from WriteSegments is fine if there is no underlying buffer, as here.

4)  false for IsNonBlocking looks right to me.


>+++ b/netwerk/base/public/nsINetworkStreams.idl

MPL2, please.  The license boilerplate is at http://www.mozilla.org/MPL/headers/

But if you take the other approach I suggest above, you may not even need this interface.

>+++ b/toolkit/components/downloads/nsDownloadManager.cpp

This and the other toolkit code should probably get review from a toolkit peer (again Gavin?).

>+  if (rv == NS_OK) {

NS_SUCCEEDED(rv)

>+      dl->mDisplayName = NS_ConvertUTF8toUTF16(filename);

  CopyUTF8ToUTF16(filename, dl->mDisplayName);
Attachment #628325 - Flags: review?(karlt)
Attachment #628325 - Flags: review?(gavin.sharp)
Attachment #628325 - Flags: review?(bzbarsky)
Attachment #628325 - Flags: review-
(In reply to Boris Zbarsky (:bz) from comment #23)
> >+      dl->mDisplayName = NS_ConvertUTF8toUTF16(filename);
> 
>   CopyUTF8ToUTF16(filename, dl->mDisplayName);
Should I include nsStringAPI.h or nsReadableUtils.h?
Thanks for reviewing, I've chopped the patch to smaller sections to easy review process.
Attachment #628325 - Attachment is obsolete: true
Attachment #628325 - Flags: review?(karlt)
Attachment #628325 - Flags: review?(gavin.sharp)
Attachment #630526 - Flags: review?(bzbarsky)
Attachment #630529 - Flags: review?(karlt)
Attached patch Toolkit and browser part (obsolete) — Splinter Review
Attachment #630530 - Flags: review?(gavin.sharp)
> Should I include nsStringAPI.h or nsReadableUtils.h?

nsReadableUtils.h
Hmm.  I'm torn between the approach in the patch and putting the newNetworkOutputStream thing on a separate interface that most protocol handlers just wouldn't QI to....
Comment on attachment 630526 [details] [diff] [review]
Support for NewNetworkOutputStream v1

I think a separate interface plays better with extension-implemented protocols.  Can you please do that?
Attachment #630526 - Flags: review?(bzbarsky) → review-
I hope that's what you have in mind. It seems to be more subtle change than adding nsIOutputStream::NewNetworkOutputStream, I like it. I'm not sure about interface name, feel free to suggest better one.
Attachment #630526 - Attachment is obsolete: true
Attachment #631863 - Flags: review?(bzbarsky)
By introducing nsISupportsNetworkOutputStreams changes were also required to this attachment.
Attachment #630529 - Attachment is obsolete: true
Attachment #630529 - Flags: review?(karlt)
Attachment #631864 - Flags: review?(karlt)
Comment on attachment 631863 [details] [diff] [review]
Support for NewNetworkOutputStream v2

>+#include "nsISupportsNetworkOutputStreams.h"

How about nsINetworkOutputProtocolHandler ?

>+            /* Without set mCancel to true call of SendErrorStatusChange will
>+             * end in endless loop when we're saving to remote location */

  If we don't set mCancel to true, calling SendErrorStatusChange will result
  in an endless loop when we're saving to remote location

> Only GIO backend is supported right now.

No reason to mention that here.  What's supported is any backend that implements the right interface.

>+nsWebBrowserPersist::MakeNetworkOutputStreamFromURI(nsIURI *aURI,
>+  nsCString scheme;

Declare this right before it's used?

>+  *aOutputStream = NULL;

I don't think that line is needed.

>+++ b/netwerk/base/public/nsISupportsNetworkOutputStreams.idl

MPL2 license, please.  And see my suggested name above.

r=me with those nits picked.
Attachment #631863 - Flags: review?(bzbarsky) → review+
Do we need superreview for this?
Attachment #631863 - Attachment is obsolete: true
Attachment #632233 - Flags: review+
Attachment #632235 - Flags: review?(karlt)
Comment on attachment 631864 [details] [diff] [review]
Add nsGIOOutputStream to nsGIOProtocolHandler v2

Marking as obsolete du to changes to Support for NewNetworkOutputStream patch.
Attachment #631864 - Attachment is obsolete: true
Attachment #631864 - Flags: review?(karlt)
Comment on attachment 632233 [details] [diff] [review]
Support for NewNetworkOutputStream v3

Probably worth Benjamin giving this a once-over
Attachment #632233 - Flags: superreview?(benjamin)
I will not be able to look at this until next week.
I'm working on support for saving attachment and messages from Thunderbird to remote locations using GIO backend. I happened to run into several usage of nsIFile and I'm looking at opportunity to use nsIFileURL instead. I'd like to get some feedback on this subject to let me know how wrong is my approach from mailnews guys. Thanks in forward.
Attachment #634041 - Flags: feedback?(mbanner)
Comment on attachment 634041 [details] [diff] [review]
wip - Support for saving attachments to GIO locations

I suspect David might be able to give more immediate feedback than myself.
Attachment #634041 - Flags: feedback?(mbanner) → feedback?(dbienvenu)
jhorak, I don't suppose there's any way to test this on windows?
I doubt it, only if you could build with --enable-gio and --enable-extensions=gio options and use GTK as ui toolkit.
Comment on attachment 634041 [details] [diff] [review]
wip - Support for saving attachments to GIO locations

In this code, is aFileURI always a remote file uri? Or really, just a remote URL? I think it might make the code easier to understand if the variable names reflect that.

We try to use nsnull instead of NULL.

I could be wrong, but I feel like there's some opportunity to avoid passing two parameters around all the time, one nsIFile and one nsIURI. A couple methods just convert the nsIFile to an nsIURI if the nsIFile parameter is non-null. If possible, I'd like to avoid that. If you need a helper method that converts an nsIFile to an nsIURI and then calls SaveAttachment(s) with all the other params, that would be better than passing two parameters all the time.

A few style things:

+      if (NS_FAILED(rv)) {
+        localFile = nsnull;
+      }

we don't use braces for single line if's.

+      } else {

else goes on it's own line, with the {

i.e.,
}
else {

pretty sure Neil won't be happy with the + string stuff :-)
Attachment #634041 - Flags: feedback?(dbienvenu) → feedback+
Attached patch Browser part v2Splinter Review
Due to some updates to GIO thunderbird patch, I'm attaching next version of patches:
- allows attaching files from remote locations
- allows opening of remote locations from File/Open and Open location
- Downloads: can open files and directories on remote locations
Please have a look.
Attachment #630530 - Attachment is obsolete: true
Attachment #630530 - Flags: review?(gavin.sharp)
Attachment #642509 - Flags: review?(gavin.sharp)
Attached patch Toolkit part v2 (obsolete) — Splinter Review
Support for downloading to remote locations.
- resume
- cancel
- Using local temp file when downloading to remote locations. When download is finished, file is uploaded to remote location (this would require change of mTempFile to mTempFileURI and couple additional changes).
Attachment #642510 - Flags: review?(gavin.sharp)
A nsGIOProtocolHandler is also derived from nsINetworkOutputProtocolHandler and it implements few methods required for saving to remote locations (check if file specified by URI exists and is a directory, remove file by URI and moving files by using two URIs).
Attachment #632235 - Attachment is obsolete: true
Attachment #632235 - Flags: review?(karlt)
Attachment #642512 - Flags: review?(karlt)
I'm re-requesting once again because I've added few more methods to nsINetworkOutputStream. I also changed mFinalFileDestination to mFinalFileDestinationURI in nsExternalHelperAppService and targetFile to mTargetFileURI in nsIHelperAppLauncherDialog.
Attachment #632233 - Attachment is obsolete: true
Attachment #632233 - Flags: superreview?(benjamin)
Attachment #642514 - Flags: review?(bzbarsky)
Attached patch Toolkit part v3Splinter Review
Fixed some minor issues.
Attachment #642510 - Attachment is obsolete: true
Attachment #642510 - Flags: review?(gavin.sharp)
Attachment #642614 - Flags: review?(gavin.sharp)
Comment on attachment 642514 [details] [diff] [review]
Support for NewNetworkOutputStream v4

The new methods look like they require sync I/O on the main thread.  That seems somewhat suboptimal.
First version of TB patch:
 - In nsIMsgMailNewsUrl.idl changed messageFile to destinationFileURI
 - Converted usage of nsIFile to nsIURI where required
 - Get/SetLastSaveDirectory doesn't work for remote locations
Attachment #634041 - Attachment is obsolete: true
Attachment #642967 - Flags: review?(mozilla)
(In reply to Boris Zbarsky (:bz) from comment #49)
> Comment on attachment 642514 [details] [diff] [review]
> Support for NewNetworkOutputStream v4
> 
> The new methods look like they require sync I/O on the main thread.  That
> seems somewhat suboptimal.
Thanks for looking. Yeah, it may cause some UI freeze but only when user is operating with remote locations (like trying to open downloaded file or starting download). This might be expected when using remote locations. Do you have any hint how to avoid this problem?
Well, the canonical way would be to make those functions return things asynchronously and make the callers deal with that.
(In reply to Boris Zbarsky (:bz) from comment #52)
> Well, the canonical way would be to make those functions return things
> asynchronously and make the callers deal with that.
And what construction would you recommend? Observer, PRThreads, nsRunnable or something else?
Sorry, I didn't have a chance to do async IO in mozilla yet.
From the API standpoint you would hand in some callback interface and it would get notified when the operation is done, right?

From an implementation standpoint, seems like a separate GIO thread that you communicate with via runnables is the way to go....
That seems to be a bit complicated to change for example nsDownload to work with events from other threads. Are all IO operations required (ie. remove, isDir, exists) to be implemented as async requests? 
Actually GIO have async version for most of file operations but the problem is how to propagate results of these async calls to mozilla-central code (where the code requires result for further actions and it used sync calls). A lot of changes (and high possibility of bringing new bugs), at least it seems to me. Maybe I see it too much complicated.
Generally we're moving to making all I/O be async as much as possible, yes.  I can't speak to the situation with nsDownload; I'm not familiar with that code.  If it's already doing sync file operations, that might be OK, I guess.
Given that nsIFile has synchronous I/O APIs, I guess that means we're moving nsIFile usage off the main thread?
We generally have been, yes.
(In reply to Boris Zbarsky (:bz) from comment #58)
> We generally have been, yes.
Is there a bug for that (to have some inspiration)? So far I've come to conclusion the easiest way to do GIO in async is to move nsDownloadManager from main to new thread.
> Is there a bug for that (to have some inspiration)?

There's a bunch of different bugs; I haven't been tracking it that carefully.

Are the sync parts only called from the download manager?  Are they absolutely required for this stuff to work at all?
(In reply to Karl Tomlinson (:karlt) from comment #57)
> Given that nsIFile has synchronous I/O APIs, I guess that means we're moving
> nsIFile usage off the main thread?

yes and we are also moving away from nsIFile(it performs poorly by design).

572459 tracks bad io patterns. bug 563742 tracks nsIFile alternative for JS code.

We are[will soon] moving download manager off main thread in bug 699854
(In reply to Boris Zbarsky (:bz) from comment #60)
> > Is there a bug for that (to have some inspiration)?
> 
> There's a bunch of different bugs; I haven't been tracking it that carefully.
> 
> Are the sync parts only called from the download manager?  Are they
> absolutely required for this stuff to work at all?
Hm, I was mostly able to get rid of RemoteTarget* calls in DownloadManager and ExternalHelperAppService. I can also run MoveRemoteTargetFromURIToURI as async with couple of changes to DownloadManager and ExternalHelperAppService.

I was a little bit surprised when I've realized that on download resume operation is done on main thread (and thus is blocking by calling nsGIOOutputStream::Write). I'll have to check  further how to run nsGIOOutputStream::Write somehow else. Looks like GIO file operations must be called from main thread too (at least they are not thread safe, they have async variants of blocking IO functions).
I've made some changes to DownloadManager and ExternalHelperAppService. MoveRemoteTarget is now calling async version of copy and use RemoteMoveTaskFinishedExternal nsIRunnable to dispatch finish event to main thread. Please let me know if this is viable approach.
I'm thinking about using temp files on local filesystem for nsGIOOutputStream and 'uploading' temp file to remote location on calling Flush() or Close() by async way (like MoveTempFile in  All-in-one patch for feedback).

Or should I implement nsGIOOutputStream as nsISafeOutputStream? It is possible to use something like nsOutputStreamTransport instead of calling nsIOutputStream::Write() to somehow sink nsIInputStream to nsIOutputStream in background?
Depends on: 713802
I am glad to see that jhorak takes care of this bug, particularly because it blocks bug 378425. Is there any chance that it will be fixed in the next version of Thunderbird? Thank a lot!
Attachment #642514 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky (:bz) from comment #33)
> Comment on attachment 631863 [details] [diff] [review]
> Support for NewNetworkOutputStream v2
> 
> >+#include "nsISupportsNetworkOutputStreams.h"
> 
> How about nsINetworkOutputProtocolHandler ?

Perhaps it could be nsIOutputProtocolHandler and newOutputStream, so
that a file protocol handler, for example, could be used with the same code paths?
One of the things making this difficult is that Mozilla is trying to avoid
blocking file I/O on the main thread and yet doesn't AFAIK have an XPCOM API
for doing async file operations.  The current approach, as in bug 789932, is
to use nsIFile et al on a separate thread.

Using a GFile for a remote file is not so different from using a local file.
If any code is performing sync file operations on the main thread, then I
don't see it as morally worse if that same code initially uses a GFile in the
same way.

However, we want to do things in a way that will allow code to be modified in
such a way that the GFile I/O no longer blocks the main thread.

There are two possible approaches.

1) Enable all I/O and other file operations to be async.

2) Provide an object (or objects) that can perform file operations on a
   different thread.

Remote file operations seem similar enough to local file operations, that we
should look for a solution that doesn't require if/else code everywhere.
Ideally the same base interface should be a solution for both remote and local
files.

Approach 1 above probably would be the preferred, but the approach so far has
been 2, so it may be easier to use that approach.

Comment 8 and comment 11 recommend against using nsIFile.  Perhaps
implementing another kind of nsIFile wouldn't be so bad if the nsIFile is used
for performing file operations rather than for carrying path info, and doesn't
try to be an nsILocalFile.  That I assume would not require a GIO nsIFileURL.

Perhaps nsGIOProtocolHandler could implement
nsIFileProtocolHandler::getFileFromURLSpec to provide its own nsIFile.

There are people who have spent much more time thinking about this than I
here, so take what I say as suggestions to consider rather than gospel.

In reply to jhorak from comment #62)
> Looks like GIO file operations must
> be called from main thread too (at least they are not thread safe, they have
> async variants of blocking IO functions).

I would have expected GLib, including GIO, objects to be thread safe unless
otherwise indicated.  What makes you think the sync operations are not thread
safe?

(In reply to jhorak from comment #63)
> Created attachment 647177 [details] [diff] [review]
> All-in-one patch for feedback
> 
> I've made some changes to DownloadManager and ExternalHelperAppService.
> MoveRemoteTarget is now calling async version of copy and use
> RemoteMoveTaskFinishedExternal nsIRunnable to dispatch finish event to main
> thread. Please let me know if this is viable approach.

Using async methods on nsIOutputProtocolHandler or another interface might be
a viable approach, but I'm not the right person to make suggestions here.
Perhaps nsIFileProtocolHandler might be a better interface - I don't know.
The convention in necko seems to be to use callbacks rather than runnables.

The people who know the existing structures best are busy people, so the best
way to get feedback is to make it as easy as possible for them.  I'd suggest
uploading a patch with just the interface proposal and perhaps modifications
to one part of code to use the interface.  Request feedback? from someone.

((In reply to jhorak from comment #64)
> I'm thinking about using temp files on local filesystem for
> nsGIOOutputStream and 'uploading' temp file to remote location on calling
> Flush() or Close() by async way (like MoveTempFile in  All-in-one patch for
> feedback).

My gut feeling is that temp files won't solve much because the write to the
temp file will still block.

> Or should I implement nsGIOOutputStream as nsISafeOutputStream? It is
> possible to use something like nsOutputStreamTransport instead of calling
> nsIOutputStream::Write() to somehow sink nsIInputStream to nsIOutputStream
> in background?

nsISafeOutputStream sounds good.  It may make the MoveRemoteTargetFromURIToURI
method unnecessary.  A flag like O_EXCL for newOutputStream may make the
RemoteTargetExists method unnecessary.

I don't know about nsOutputStreamTransport.
Comment on attachment 642512 [details] [diff] [review]
Add nsGIOOutputStream, expand nsGIOProtocolHandler v3

I don't think there's much point me reviewing this until we have review on the
interfaces, but some comments from a quick look:

nsGIOOutputStream::Init uses g_file_query_info which would block, but Init
would be called on the main thread, I assume.

Looks like it is not safe to ref-count nsGIOOutputStream on another thread,
which would make it hard to use on another thread.  Should it use  
NS_IMPL_THREADSAFE_ISUPPORTS1?
Attachment #642512 - Flags: review?(karlt)
Comment on attachment 642614 [details] [diff] [review]
Toolkit part v3

Unfortunately these patches have been bit rotten. If this work is revived I'll do a better job of getting you a review.
Attachment #642614 - Flags: review?(gavin.sharp)
Attachment #642509 - Flags: review?(gavin.sharp)
Comment on attachment 642967 [details] [diff] [review]
mailnews patch v1

note, bienvenu is no longer doing reviews
Attachment #642967 - Flags: review?(mozilla)
Flags: needinfo?(jhorak)
It does feel like there is merely a UI issue with latest version of GNOME.

1. In Fedora 21 (FF 42), I could bookmark a remote location, mount it in Nautilus, then it shows up in FF's open file dialogue.

2. In Fedora 22 (I don't have the computer with me to tell FF version), I could use the remote location 'bookmark' and manually type a location and could open files there. The bookmarks are all gone though.
Whiteboard: [necko-would-take]
With the switch to Gtk3 by default, should this be revived/reconsidered ?
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P5
See Also: → 1187870

I'm having the same problem with KDE's KIO

(In reply to Maverick from comment #75)

I'm having the same problem with KDE's KIO

While on Plasma, and using it's file open/save dialog, this has now been fixed! see bug https://bugs.kde.org/show_bug.cgi?id=444457

Maybe we could switch to native file dialogs even for non-flatpak. It generally works for me. What could go wrong possibly is to start the download, then ending the session and restart the download without having the location mounted. But I would not bother with that. The download is most likely then shown as failed.

Also setting the download location to remote host could also trigger that.

You can test native file dialogs by: widget.use-xdg-desktop-portal.file-picker = 1 prefs.

Flags: needinfo?(jhorak)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.