Last Comment Bug 682838 - Unable to open/save from/to GIO remote location
: Unable to open/save from/to GIO remote location
Status: ASSIGNED
[necko-would-take]
:
Product: Core
Classification: Components
Component: Networking: File (show other bugs)
: Other Branch
: All Linux
: -- normal with 2 votes (vote)
: mozilla14
Assigned To: Jan Horak
:
Mentors:
Depends on: 121059 713802
Blocks: 378425
  Show dependency treegraph
 
Reported: 2011-08-29 07:33 PDT by Jan Horak
Modified: 2016-05-15 07:57 PDT (History)
25 users (show)
vseerror: needinfo? (jhorak)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip patch 1 - may be close to final (80.77 KB, patch)
2012-03-19 06:04 PDT, Jan Horak
bzbarsky: feedback-
Details | Diff | Review
Patch which allows to save files/page to gio location 1 (37.51 KB, patch)
2012-05-30 06:39 PDT, Jan Horak
no flags Details | Diff | Review
Patch which allows to save files/page to gio location 1 (35.58 KB, patch)
2012-05-30 06:41 PDT, Jan Horak
bzbarsky: review-
Details | Diff | Review
Support for NewNetworkOutputStream v1 (19.84 KB, patch)
2012-06-06 05:45 PDT, Jan Horak
bzbarsky: review-
Details | Diff | Review
Add nsGIOOutputStream to nsGIOProtocolHandler (10.24 KB, patch)
2012-06-06 05:46 PDT, Jan Horak
no flags Details | Diff | Review
Toolkit and browser part (19.55 KB, patch)
2012-06-06 05:48 PDT, Jan Horak
no flags Details | Diff | Review
Support for NewNetworkOutputStream v2 (8.39 KB, patch)
2012-06-11 05:27 PDT, Jan Horak
bzbarsky: review+
Details | Diff | Review
Add nsGIOOutputStream to nsGIOProtocolHandler v2 (11.07 KB, patch)
2012-06-11 05:29 PDT, Jan Horak
no flags Details | Diff | Review
Support for NewNetworkOutputStream v3 (6.83 KB, patch)
2012-06-12 06:44 PDT, Jan Horak
jhorak: review+
Details | Diff | Review
Add nsGIOOutputStream to nsGIOProtocolHandler v2 (11.07 KB, patch)
2012-06-12 06:45 PDT, Jan Horak
no flags Details | Diff | Review
wip - Support for saving attachments to GIO locations (32.39 KB, patch)
2012-06-18 08:06 PDT, Jan Horak
mozilla: feedback+
Details | Diff | Review
Browser part v2 (8.29 KB, patch)
2012-07-16 02:34 PDT, Jan Horak
no flags Details | Diff | Review
Toolkit part v2 (33.53 KB, patch)
2012-07-16 02:40 PDT, Jan Horak
no flags Details | Diff | Review
Add nsGIOOutputStream, expand nsGIOProtocolHandler v3 (14.02 KB, patch)
2012-07-16 02:44 PDT, Jan Horak
no flags Details | Diff | Review
Support for NewNetworkOutputStream v4 (34.97 KB, patch)
2012-07-16 02:50 PDT, Jan Horak
no flags Details | Diff | Review
Toolkit part v3 (34.26 KB, patch)
2012-07-16 09:41 PDT, Jan Horak
no flags Details | Diff | Review
mailnews patch v1 (78.04 KB, patch)
2012-07-17 08:32 PDT, Jan Horak
no flags Details | Diff | Review
All-in-one patch for feedback (114.42 KB, patch)
2012-07-30 08:09 PDT, Jan Horak
no flags Details | Diff | Review

Description Jan Horak 2011-08-29 07:33:47 PDT
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.
Comment 1 Jan Horak 2011-12-19 06:59:04 PST
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.
Comment 2 Jan Horak 2012-03-19 06:04:38 PDT
Created attachment 607138 [details] [diff] [review]
wip patch 1 - may be close to final

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.
Comment 3 Gerald 2012-05-01 03:41:50 PDT
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.
Comment 4 Jan Horak 2012-05-04 05:36:38 PDT
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.
Comment 5 Karl Tomlinson (ni?:karlt) 2012-05-06 23:24:34 PDT
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?
Comment 6 Gerald 2012-05-06 23:49:34 PDT
@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.
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-09 18:45:17 PDT
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?
Comment 8 Benjamin Smedberg [:bsmedberg] 2012-05-10 06:58:02 PDT
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.
Comment 9 Jan Horak 2012-05-10 07:32:14 PDT
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.
Comment 10 Jan Horak 2012-05-10 07:32:34 PDT
And thanks for feedback!
Comment 11 Benjamin Smedberg [:bsmedberg] 2012-05-10 09:52:30 PDT
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.
Comment 12 Karl Tomlinson (ni?:karlt) 2012-05-10 15:59:28 PDT
(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 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-10 22:45:31 PDT
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...
Comment 14 Jan Horak 2012-05-11 12:45:19 PDT
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.
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-11 13:00:29 PDT
> 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.
Comment 16 Jan Horak 2012-05-15 04:30:01 PDT
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?
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-21 20:43:58 PDT
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....
Comment 18 Jan Horak 2012-05-30 06:39:02 PDT
Created attachment 628323 [details] [diff] [review]
Patch which allows to save files/page to gio location 1

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.
Comment 19 Jan Horak 2012-05-30 06:41:44 PDT
Created attachment 628325 [details] [diff] [review]
Patch which allows to save files/page to gio location 1

Ops, removed some irrelevant portions.
Comment 20 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-30 07:48:11 PDT
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?
Comment 21 Jan Horak 2012-05-31 03:06:52 PDT
(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.
Comment 22 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-31 07:10:23 PDT
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 23 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-03 19:14:03 PDT
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);
Comment 24 Jan Horak 2012-06-06 05:42:50 PDT
(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?
Comment 25 Jan Horak 2012-06-06 05:45:08 PDT
Created attachment 630526 [details] [diff] [review]
Support for NewNetworkOutputStream v1

Thanks for reviewing, I've chopped the patch to smaller sections to easy review process.
Comment 26 Jan Horak 2012-06-06 05:46:32 PDT
Created attachment 630529 [details] [diff] [review]
Add nsGIOOutputStream to nsGIOProtocolHandler
Comment 27 Jan Horak 2012-06-06 05:48:12 PDT
Created attachment 630530 [details] [diff] [review]
Toolkit and browser part
Comment 28 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-06 08:06:15 PDT
> Should I include nsStringAPI.h or nsReadableUtils.h?

nsReadableUtils.h
Comment 29 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-06 13:26:25 PDT
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 30 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-08 19:35:49 PDT
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?
Comment 31 Jan Horak 2012-06-11 05:27:51 PDT
Created attachment 631863 [details] [diff] [review]
Support for NewNetworkOutputStream v2

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.
Comment 32 Jan Horak 2012-06-11 05:29:45 PDT
Created attachment 631864 [details] [diff] [review]
Add nsGIOOutputStream to nsGIOProtocolHandler v2

By introducing nsISupportsNetworkOutputStreams changes were also required to this attachment.
Comment 33 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-11 20:39:58 PDT
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.
Comment 34 Jan Horak 2012-06-12 06:44:54 PDT
Created attachment 632233 [details] [diff] [review]
Support for NewNetworkOutputStream v3

Do we need superreview for this?
Comment 35 Jan Horak 2012-06-12 06:45:31 PDT
Created attachment 632235 [details] [diff] [review]
Add nsGIOOutputStream to nsGIOProtocolHandler v2
Comment 36 Jan Horak 2012-06-12 06:48:17 PDT
Comment on attachment 631864 [details] [diff] [review]
Add nsGIOOutputStream to nsGIOProtocolHandler v2

Marking as obsolete du to changes to Support for NewNetworkOutputStream patch.
Comment 37 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-12 11:09:42 PDT
Comment on attachment 632233 [details] [diff] [review]
Support for NewNetworkOutputStream v3

Probably worth Benjamin giving this a once-over
Comment 38 Benjamin Smedberg [:bsmedberg] 2012-06-14 11:17:57 PDT
I will not be able to look at this until next week.
Comment 39 Jan Horak 2012-06-18 08:06:49 PDT
Created attachment 634041 [details] [diff] [review]
wip - Support for saving attachments to GIO locations

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.
Comment 40 Mark Banner (:standard8) 2012-06-18 08:27:54 PDT
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.
Comment 41 David :Bienvenu 2012-06-18 16:13:28 PDT
jhorak, I don't suppose there's any way to test this on windows?
Comment 42 Jan Horak 2012-06-19 00:28:04 PDT
I doubt it, only if you could build with --enable-gio and --enable-extensions=gio options and use GTK as ui toolkit.
Comment 43 David :Bienvenu 2012-06-26 08:06:36 PDT
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 :-)
Comment 44 Jan Horak 2012-07-16 02:34:45 PDT
Created attachment 642509 [details] [diff] [review]
Browser part v2

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.
Comment 45 Jan Horak 2012-07-16 02:40:25 PDT
Created attachment 642510 [details] [diff] [review]
Toolkit part v2

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).
Comment 46 Jan Horak 2012-07-16 02:44:33 PDT
Created attachment 642512 [details] [diff] [review]
Add nsGIOOutputStream, expand nsGIOProtocolHandler v3

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).
Comment 47 Jan Horak 2012-07-16 02:50:42 PDT
Created attachment 642514 [details] [diff] [review]
Support for NewNetworkOutputStream v4

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.
Comment 48 Jan Horak 2012-07-16 09:41:07 PDT
Created attachment 642614 [details] [diff] [review]
Toolkit part v3

Fixed some minor issues.
Comment 49 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-07-16 21:17:31 PDT
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.
Comment 50 Jan Horak 2012-07-17 08:32:39 PDT
Created attachment 642967 [details] [diff] [review]
mailnews patch v1

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
Comment 51 Jan Horak 2012-07-17 08:39:01 PDT
(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?
Comment 52 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-07-17 08:54:28 PDT
Well, the canonical way would be to make those functions return things asynchronously and make the callers deal with that.
Comment 53 Jan Horak 2012-07-18 08:25:53 PDT
(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.
Comment 54 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-07-18 09:11:11 PDT
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....
Comment 55 Jan Horak 2012-07-19 04:18:26 PDT
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.
Comment 56 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-07-19 11:55:02 PDT
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.
Comment 57 Karl Tomlinson (ni?:karlt) 2012-07-19 12:00:15 PDT
Given that nsIFile has synchronous I/O APIs, I guess that means we're moving nsIFile usage off the main thread?
Comment 58 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-07-19 12:08:15 PDT
We generally have been, yes.
Comment 59 Jan Horak 2012-07-23 05:08:35 PDT
(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.
Comment 60 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-07-23 06:09:39 PDT
> 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?
Comment 61 (dormant account) 2012-07-23 15:19:11 PDT
(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
Comment 62 Jan Horak 2012-07-27 05:00:47 PDT
(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).
Comment 63 Jan Horak 2012-07-30 08:09:44 PDT
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.
Comment 64 Jan Horak 2012-08-02 01:40:22 PDT
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?
Comment 65 Gerald 2012-10-04 03:06:51 PDT
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!
Comment 66 Karl Tomlinson (ni?:karlt) 2012-12-11 23:25:22 PST
(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?
Comment 67 Karl Tomlinson (ni?:karlt) 2012-12-11 23:25:44 PST
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 68 Karl Tomlinson (ni?:karlt) 2012-12-11 23:27:12 PST
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?
Comment 69 :Gavin Sharp [email: gavin@gavinsharp.com] 2014-02-28 17:55:23 PST
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.
Comment 70 Wayne Mery (:wsmwk, NI for questions) 2014-09-01 06:29:43 PDT
Comment on attachment 642967 [details] [diff] [review]
mailnews patch v1

note, bienvenu is no longer doing reviews
Comment 71 Feng Yu 2016-01-11 11:57:41 PST
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.
Comment 72 Landry Breuil (:gaston) 2016-05-15 07:57:57 PDT
With the switch to Gtk3 by default, should this be revived/reconsidered ?

Note You need to log in before you can comment on or make changes to this bug.