Last Comment Bug 494163 - (gio / gvfs) Port gnomevfs extension to GIO/GVFS
: (gio / gvfs) Port gnomevfs extension to GIO/GVFS
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: x86 Linux
: -- enhancement with 2 votes (vote)
: mozilla5
Assigned To: Jan Horak
:
:
Mentors:
Depends on: post2.0
Blocks:
  Show dependency treegraph
 
Reported: 2009-05-21 06:24 PDT by Jan Horak
Modified: 2014-08-25 13:17 PDT (History)
21 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Initial extension patch (39.63 KB, patch)
2009-05-21 06:28 PDT, Jan Horak
karlt: review-
roc: superreview+
Details | Diff | Splinter Review
Patch 0.2, fixed according to review comments (35.71 KB, patch)
2009-09-18 06:36 PDT, Jan Horak
no flags Details | Diff | Splinter Review
Patch 0.3, switched from gtk_mount_operation back to nsIAuthPrompt (46.98 KB, patch)
2010-01-12 05:25 PST, Jan Horak
no flags Details | Diff | Splinter Review
Patch 0.3b, switched from gtk_mount_operation back to nsIAuthPrompt (46.00 KB, patch)
2010-01-12 06:52 PST, Jan Horak
no flags Details | Diff | Splinter Review
Patch 0.4, removed configure.in and autoconf.mk.in diffs. (43.02 KB, patch)
2010-02-11 05:04 PST, Jan Horak
karlt: review-
Details | Diff | Splinter Review
Patch 0.5, Monitor added, observers removed (43.78 KB, patch)
2010-05-17 02:27 PDT, Jan Horak
karlt: review-
Details | Diff | Splinter Review
Patch 0.6, warnings removed, Cancel removed (42.77 KB, patch)
2010-05-20 04:54 PDT, Jan Horak
karlt: review+
Details | Diff | Splinter Review
Patch 0.7, sync to mozilla-central (43.08 KB, patch)
2011-01-05 05:49 PST, Jan Horak
no flags Details | Diff | Splinter Review
Patch 0.8, fixed comments and added mount operation enum (43.61 KB, patch)
2011-01-06 04:24 PST, Jan Horak
karlt: review-
Details | Diff | Splinter Review
Patch 0.9, fixed comments and mount operation enum (43.47 KB, patch)
2011-01-10 03:38 PST, Jan Horak
karlt: review+
Details | Diff | Splinter Review
Patch 0.1, fixed license (43.42 KB, patch)
2011-01-11 02:19 PST, Jan Horak
jst: approval2.0-
Details | Diff | Splinter Review

Description Jan Horak 2009-05-21 06:24:57 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.10) Gecko/2009042708 Fedora/3.0.10-1.fc10 Firefox/3.0.10
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.10) Gecko/2009042708 Fedora/3.0.10-1.fc10 Firefox/3.0.10

As gnomeVFS is deprecated we should port gnomevfs extension to GIO/GVFS. Gnomevfs extension allows opening sftp:// or smb:// URI directly in Firefox.

Reproducible: Always
Comment 1 Jan Horak 2009-05-21 06:28:58 PDT
Created attachment 378853 [details] [diff] [review]
Initial extension patch

Use "--enable-extensions=gio" to compile with gio module.
Comment 2 Christopher Aillon (sabbatical, not receiving bugmail) 2009-08-04 14:25:57 PDT
closing in favor of the other bug

*** This bug has been marked as a duplicate of bug 402892 ***
Comment 3 Jan Horak 2009-08-10 06:25:23 PDT
Actually this bug/patch is about GnomeVFS extension which allows to open URI like sftp:// smb://. 

Bug #402892 is concerned about FF integration into Gnome (file association, MIME types, default browser settings, etc.).

Therefore the review request is still valid.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-06 14:41:42 PDT
Comment on attachment 378853 [details] [diff] [review]
Initial extension patch

I only took a cursory look at the patch --- i.e. this is really an sr, and really not an r :-)
Comment 5 Karl Tomlinson (:karlt) 2009-09-11 20:24:11 PDT
Comment on attachment 378853 [details] [diff] [review]
Initial extension patch

>+REQUIRES	= xpcom \

This can now be removed (since bug 398573).

>+# to install gnome-vfs2 in order to use the rest of mozilla ;-)

s/gnome-vfs2/gio/

>+ * This code is based on original Mozilla gnome-vfs extension. It implements
>+ * input stream provided by GVFS/GIO.

This should match the template at

http://www.mozilla.org/MPL/MPL-1.1.html#exhibit-a

so I think that means "The Original Code is the Mozilla gnome-vfs extension",
unfortunately.  Although the comment is helpful, it would be better separated
from the license block.

>+     case G_IO_ERROR_FAILED:                     return NS_ERROR_UNEXPECTED;

Looks like G_IO_ERROR_FAILED is a generic failure, so "case
G_IO_ERROR_FAILED:" should probably just fall through to the "default:" to
return NS_ERROR_FAILURE.

>+     case G_IO_ERROR_NOT_MOUNTED:
>+     case G_IO_ERROR_HOST_NOT_FOUND:             return NS_ERROR_UNKNOWN_HOST;

G_IO_ERROR_NOT_MOUNTED is here and in unhandled below.  Consider
NS_ERROR_NOT_CONNECTED here, or just remove.

>+  G_IO_ERROR_INVALID_FILENAME,

NS_ERROR_FILE_INVALID_PATH 

>+  G_IO_ERROR_WOULD_BLOCK,

NS_BASE_STREAM_WOULD_BLOCK

>+  G_IO_ERROR_TIMED_OUT,

Maybe NS_ERROR_NET_TIMEOUT.

>+  if (!gdk_win)
>+  return NULL;
>+
>+  // Get the container
>+  gpointer user_data = NULL;
>+  gdk_window_get_user_data(gdk_win, &user_data);
>+  if (!user_data)
>+  return NULL;

Indent the return statements, please.

>+  // Make sure its really a container
>+  MozContainer *parent_container = (MozContainer*) (user_data);
>+  if (!parent_container)
>+  return NULL;

This is barely pretending to "make sure".
It actually only needs to be a GtkWidget.
Either use GTK_IS_WIDGET or skip the check.

>+    nsCOMPtr<nsIBaseWindow> baseWin(do_QueryInterface(reinterpret_cast<nsISupports*> (window->GetDocShell()) ));

>+        baseWin = do_QueryInterface(reinterpret_cast<nsISupports*> (window->GetDocShell()) );

The reinterpret_casts don't look right.
Does including nsIDocShell should fix the problem?

>+  nsCOMPtr<nsIWindowWatcher> wwatch =
>+      do_GetService(NS_WINDOWWATCHER_CONTRACTID, &rv);
>+  //NS_ENSURE_SUCCESS(rv, rv);

Should check for failure here somehow.

>+
>+  nsCOMPtr<nsIDOMWindow> active;
>+  wwatch->GetActiveWindow(getter_AddRefs(active));
>+
>+  GtkWindow *parent_wnd = NULL;
>+  parent_wnd = get_gtk_window_for_nsiwidget(DOMWindowToWidget(active));

This doesn't look thread-safe.

And is there always an active window?

We actually have no idea which window is associated with the stream (AFAIK).
Is the transient parent important?

>+  GMountOperation* mount_op = gtk_mount_operation_new(parent_wnd);

gtk_mount_operation_new needs gtk+-2.14 so there needs to be a configure check
for that version.

Is GTK thread-safe?

>+  if (error) {
>+    g_warning("Error reading directory content: %s", error->message);
>+    nsresult rv = MapGIOResult(error);
>+    g_error_free(error);
>+    return rv;
>+  }
>+  g_object_unref(f_enum);

f_enum needs to be freed even if error.

>+    if (mime_type && strcmp(mime_type, APPLICATION_OCTET_STREAM) != 0) {
>+      SetContentTypeOfChannel(mime_type);
>+      g_free(mime_type);
>+    }

mime_type needs to be freed even if it is APPLICATION_OCTET_STREAM, I assume.

>+  mBytesRemaining = (PRUint64) g_file_info_get_size(info);

The cast doesn't look necessary.

>+      // location is not yet mounted, try to mount
>+      g_warning("Unable to get file info: %s", error->message);

I'd rather not have g_warnings unless something has gone wrong with the code
(or external libraries).  Here, at least the error is handled so this error
seems expected.  If you want the user to see the error, then that may be
complicated, but g_warning is not the way to present the error to the user.

>+ * @return NS_OK when read successfully, NS_BASE_STREAM_CLOSED when enf of file,

"end"

>+    *aCountRead = g_input_stream_read(G_INPUT_STREAM(mStream),
>+                                      aBuf,
>+                                      aCount,
>+                                      NULL,
>+                                      &error);

XPCOM methods usually should not set output parameters when throwing an error.
That protocol is not strictly followed, but returning PRUint32(-1) for
aCountRead could be particularly bad.

>+        if ( g_file_info_get_name(info)[0] == '.'
>+          && (g_file_info_get_name(info)[1] == '\0'
>+          || (g_file_info_get_name(info)[1] == '.'
>+          && g_file_info_get_name(info)[2] == '\0')) )

Can you align to make the grouping clearer, please?
Each line should be just inside its innermost opening parenthesis.
I would store the result of g_file_info_get_name in a variable rather than
calling the function 5 times.

>+nsGIOInputStream::Read(char     *aBuf,
>+                       PRUint32  aCount,
>+                       PRUint32 *aCountRead)
>+{
>+  *aCountRead = 0;
>+
>+  if (mStatus == NS_BASE_STREAM_CLOSED)
>+    return NS_OK;
>+  if (NS_FAILED(mStatus)) {
>+    g_warning("Cannot doOpenFIle: %d", mStatus);
>+    return mStatus;
>+  }

Can this warning be closer to the source of the failure?

>+  if (!mStream && !mDirOpen)
>+    mStatus = DoOpen();
>+  if (mStatus != NS_OK) {
>+    g_warning("Cannot DoOpen file - error code: %x", mStatus);
>+    mChannel->Cancel(mStatus);
>+    return mStatus;
>+  }

Should mChannel only be Canceled on the first error rather than each
time a Read is attempted?

Is it safe to Cancel on this thread?

>+  return mStatus;
>+
>+}

>+  return NS_OK;
>+
>+}

The method blocks can be closed on the line following the return (without the
blank line).

>+    PRInt32 colon_location = strchr(flatSpec.get(), ':') - flatSpec.get();
>+    if (colon_location <= 0)

The conversion to PRInt32 isn't necessarily going to be -ve if strchr returns
NULL (and even the ptrdiff_t won't necessarily be -ve, because NULL is not in
the same array as flatSpec).  flatSpec.FindChar(':') looks like what you want.

http://hg.mozilla.org/mozilla-central/annotate/0e157c793c5c/xpcom/glue/nsStringAPI.h#l676

>+      if (strncmp( *uri_schemes, flatSpec.get(), colon_location - 1 ) == 0) {
>+        uri_scheme_supported = PR_TRUE;
>+        break;
>+      }

If *uri_schemes is, for example, "scheme-one", then uri_scheme_supported will
be true even when flatSpec is "sx:something", for example.  I think the
following should work better:

StringHead(flatspec, colon_location).Equals(*uri_schemes)

>+    if (uri_scheme_supported == PR_FALSE) {

Just use PRBool as a boolean type:

  if (!uri_scheme_supported)

>+  // XXX Can we really assume that all gnome-vfs URIs can be parsed using
>+  //     nsStandardURL?  We probably really need to implement nsIURI/nsIURL
>+  //     in terms of the gnome_vfs_uri_XXX methods, but at least this works
>+  //     correctly for smb:// URLs ;-)
>+  //
>+  // FIXME    Also, it might not be possible to fully implement nsIURI/nsIURL in
>+  //     terms of GnomeVFSURI since some Necko methods have no GnomeVFS
>+  //     equivalent.

Does this all apply to GIO also?
Comment 6 Jan Horak 2009-09-18 06:36:18 PDT
Created attachment 401429 [details] [diff] [review]
Patch 0.2, fixed according to review comments

Thanks a lot for your comments. I've tried to fix problematic issues. I've also dropped from searching for root window (to be able to create modal dialogue). 

In fact while invoking command 'firefox sftp://etc' failed to get root window anyway. The problem is that if you close the Firefox while the login window is still showed the Firefox keeps naturally running. Right now I'm not sure how to solve this issue.

Could you look at this patch once again, please?

Also please look at GTK 2.14 checking in configure.in, I'm not very sure if this is the right way to do the version checking.
Comment 7 Karl Tomlinson (:karlt) 2009-10-01 22:49:52 PDT
(In reply to comment #6)
> The problem is that if you close the Firefox while the login window is
> still showed the Firefox keeps naturally running. Right now I'm not sure how to
> solve this issue.

What did the gnomevfs extension do here?

nsAppStartup counts XUL windows and tries to quit when the last is closed;
that may cause some problems here.

Would it make sense to implement a GMountOperation (to replace the GtkMountOperation) using nsIAuthPrompt (like the gnomevfs extension used)?

One thing we need to tell the user when asking for a password is the host to which they are connecting.
Comment 8 Reed Loden [:reed] (use needinfo?) 2010-01-05 11:59:53 PST
Jan, any updates on this?
Comment 9 Jan Horak 2010-01-12 05:25:43 PST
Created attachment 421233 [details] [diff] [review]
Patch 0.3, switched from gtk_mount_operation back to nsIAuthPrompt

Sorry for long delay. I've removed gtk_mount_operation and put nsIAuthPrompt back. I also switched to using observers for getting dialogue result. 
Any ideas for?:
  while (!mMountFinished) {
    PR_Sleep(1000); // XXX waiting loop
  }
Please could you check it?
Comment 10 Jan Horak 2010-01-12 06:52:45 PST
Created attachment 421249 [details] [diff] [review]
Patch 0.3b, switched from gtk_mount_operation back to nsIAuthPrompt

Sorry, bogus diff parts.
Comment 11 Serge Gautherie (:sgautherie) 2010-02-10 10:25:16 PST
Comment on attachment 421249 [details] [diff] [review]
Patch 0.3b, switched from gtk_mount_operation back to nsIAuthPrompt

>diff -r 1a3d81ba980a config/autoconf.mk.in
>@@ -261,16 +261,19 @@ MOZ_GCONF_CFLAGS = @MOZ_GCONF_CFLAGS@
> MOZ_GIO_CFLAGS = @MOZ_GIO_CFLAGS@
> MOZ_GIO_LIBS = @MOZ_GIO_LIBS@
> 
>+MOZ_GIO_CFLAGS = @MOZ_GIO_CFLAGS@
>+MOZ_GIO_LIBS = @MOZ_GIO_LIBS@
>+

Bad merge?
Comment 12 Jan Horak 2010-02-11 05:04:02 PST
Created attachment 426480 [details] [diff] [review]
Patch 0.4, removed configure.in and autoconf.mk.in diffs.

Sorry, you're right. Removed diffs of configure.in and autoconf.mk.in as they are obsolete now.
Comment 13 Karl Tomlinson (:karlt) 2010-05-11 21:07:40 PDT
Comment on attachment 426480 [details] [diff] [review]
Patch 0.4, removed configure.in and autoconf.mk.in diffs.

(In reply to comment #9)
> I also switched to using observers for getting dialogue result. 

These observers are retrieving the result of g_file_mount_enclosing_volume
(rather than the password dialog).  nsIObserverService is really for global
notifications.  Here IIUC there could be multiple concurrent nsGIOInputStreams
and thus multiple GMountOperations.  It seems that
mount_enclosing_volume_finished needs a reference to the nsGIOInputStream.

> Any ideas for?:
>   while (!mMountFinished) {
>     PR_Sleep(1000); // XXX waiting loop
>   }
> Please could you check it?

mozilla::Monitor should suit this case:

http://hg.mozilla.org/mozilla-central/annotate/f7a9b2f21b09/xpcom/glue/Monitor.h#l59
https://developer.mozilla.org/en/NSPR_API_Reference/Introduction_to_NSPR#NSPR_Thread_Synchronization

It would be helpful to have some comments explaining on which thread the
callbacks from g_file_mount_enclosing_volume will be invoked.
(It's a little counter-intuitive that its neither the thread that called
g_file_mount_enclosing_volume nor the thread that created the GMountOperation.

I don't know that we can be sure that nsGIOInputStream::Read is always called
off the main thread.  If it gets called on the main thread, MountVolume will
lock.  I think the right thing to do if G_IO_ERROR_NOT_MOUNTED is received
when called on the main thread is just skip the MountVolume and return an
error.  It looks like "bool NS_IsMainThread()" is the function to use to check
this.

Can you check the list of headers included please?
There have been some modifications so I think some are longer be needed.

configure.in should probably have a check that gio is available, similar to
the `echo "$MOZ_EXTENSIONS" | grep -c gnomevfs` test.

>+ * The Original Code is IBM Corporation.

"* The Original Code is the Mozilla gnome-vfs extension.
 *
 * The Initial Developer of the Original Code is IBM Corporation."

>+     case G_IO_ERROR_NOT_MOUNTED:                return NS_ERROR_NOT_CONNECTED

>+/* unhandled:
>+  G_IO_ERROR_NOT_REGULAR_FILE,
>+  G_IO_ERROR_NOT_SYMBOLIC_LINK,
>+  G_IO_ERROR_NOT_MOUNTABLE_FILE,
>+  G_IO_ERROR_TOO_MANY_LINKS,
>+  G_IO_ERROR_NOT_MOUNTED,

G_IO_ERROR_NOT_MOUNTED is handled above so remove it from this list.

>+    // Make sure authIn->uri is consistent with the channel's URI.
>+    //
>+    // XXX This check is probably not IDN safe, and it might incorrectly
>+    //     fire as a result of escaping differences.  It's unclear what
>+    //     kind of transforms GnomeVFS might have applied to the URI spec
>+    //     that we originally gave to it.  In spite of the likelihood of
>+    //     false hits, this check is probably still valuable.
>+    //
>+    nsCAutoString spec;
>+    uri->GetSpec(spec);
>+    int uriLen = strlen(default_domain);
>+    if (!StringHead(spec, uriLen).Equals(nsDependentCString(default_domain, uriLen)))
>+    {
>+      LOG(("gnomevfs: [spec=%s authIn->uri=%s]\n", spec.get(), default_domain));
>+      NS_ERROR("URI mismatch");
>+    }

I wouldn't expect default_domain to be the same as the spec.  It seems the
only reason this passes (when testing with sftp:) is that default_domain is
empty.  Might as well just remove the check I guess.  It was only run in
debug nsGnomeVFSProtocolHandler builds anyway.

If G_ASK_PASSWORD_NEED_DOMAIN is requested, should the reply be
G_MOUNT_OPERATION_UNHANDLED?

>+        const PRUnichar *strings[] = { realm.get(), dispHost.get() };
>+        bundle->FormatStringFromName(NS_LITERAL_STRING("EnterUserPasswordForRealm").get(),

"EnterUserPasswordForRealm" has been changed to "EnterLoginForRealm"
Apparently nsGnomeVFSProtocolHandler didn't get updated.
http://hg.mozilla.org/mozilla-central/annotate/5b027af3af29/dom/locales/en-US/chrome/prompts.properties
http://hg.mozilla.org/mozilla-central/rev/5949243ac286

++  GMountOperation* mount_op = g_mount_operation_new();
>+  g_signal_connect (mount_op, "ask_password",
>+                     G_CALLBACK (mount_operation_ask_password), mChannel);

Make this "ask-password" for consistency with the documentation,
and touch up indentation.

It looks like the channel could be destroyed before
mount_operation_ask_password is called.  Disconnecting the callback from the
GMountOperation when the channel is released may be possible somehow.  Or
perhaps its easier to use the nsGIOInputStream for the user data and to get
the channel from that, but then the nsGIOInputStream::mChannel would still
need to be NULLed at the right time.  Or perhaps it is possible to
synchronously cancel g_file_mount_enclosing_volume.  I haven't really worked
out the best way to handle this.

>+  if (error) {
>+    g_warning("Error reading directory content: %s", error->message);
>+    nsresult rv = MapGIOResult(error);
>+    g_error_free(error);
>+    g_object_unref(f_enum);
>+    return rv;
>+  }
>+  g_object_unref(f_enum);

Instead of having two g_object_unref calls, unref f_enum in one call before
the "if (error)" check.

>+  //if (mBytesRemaining != PR_UINT64_MAX)

I guess there's no need for this with g_file_info_get_size?

>+      //AuthCallback((gconstpointer)&mSpec, 0, NULL, 0, mChannel);

Remove this.

>+  GError *error = NULL;
>+  nsresult rv = NS_OK;
>+  PRUint32 bytes_read = 0;
>+  if (mStream) {
>+    // file read
>+    bytes_read = g_input_stream_read(G_INPUT_STREAM(mStream),
>+                                     aBuf,
>+                                     aCount,
>+                                     NULL,
>+                                     &error);

If bytes_read is only going to be used in the "if (mStream)" block then
instead of initializing bytes_read to 0, declare and initialize with the
g_input_stream_read call.

Similarly, declare |error| inside the block.

I suspect NS_OK is not the right thing to return if this function does
nothing.

>+        const char * fname = g_file_info_get_name(info);
>+        if ( fname[0] == '.'
>+          && (fname[1] == '\0' || (fname[1] == '.' && fname[2] == '\0')) )

Can we can be sure that fname is non-NULL?
Can you touch up the indentation, please?

>+          nu->EscapeString(nsDependentCString(g_file_info_get_name(info)),

Use fname (if non-NULL).

>+ * @param aCount length if aBuf

"length of aBuf"

>+  // Check if file is already opened, otherwise open it
>+  if (!mStream && !mDirOpen) {
>+    mStatus = DoOpen();

Should mStatus be checked for previous failure before trying the open again?

>+      g_warning("Unable to open location - error code: %x", mStatus);
>+      mChannel->Cancel(NS_ERROR_NOT_CONNECTED);
>+      return 0;

Probably no need for this warning as DoOpen already provides failure warnings.

nsIChannel.idl says "This interface must be used only from the XPCOM main
thread", which makes me concerned about this.

nsGnomeVFSProtocolHandler didn't bother to Cancel the channel.
Is this necessary?

The return value should be a (failure) nsresult.

>+    g_warning("Unable to read from location - error code: %x", mStatus);
>+    mChannel->Cancel(mStatus);

Similar issues here.

>+        uri_scheme_supported = PR_TRUE;
>+        //break;

It would make sense to keep this break.
Comment 14 Karl Tomlinson (:karlt) 2010-05-12 14:20:50 PDT
(In reply to comment #13)
> It looks like the channel could be destroyed before
> mount_operation_ask_password is called.

Sorry, I think I'm wrong here and I expect what you have is fine.
I don't completely grasp the relationship between the nsGIOInputStream and the nsIChannel, but I assume that, if a thread is Read()ing the nsGIOInputStream, something must be holding a reference to the nsIChannel.
Comment 15 Jan Horak 2010-05-17 02:27:13 PDT
Created attachment 445660 [details] [diff] [review]
Patch 0.5, Monitor added, observers removed

Removed observers replacing them with user pointer in mount_enclosing_volume_finished. Added comments about threads, Waiting loop replaced by monitor.

Usage of Cancel method for channels shows nice error messages to user which depends on gio failure type. Please have a look.
Comment 16 Karl Tomlinson (:karlt) 2010-05-18 21:27:45 PDT
Comment on attachment 445660 [details] [diff] [review]
Patch 0.5, Monitor added, observers removed

>+    else
>+        if test `echo "$MOZ_EXTENSIONS" | grep -c gio` -ne 0; then
>+            PKG_CHECK_MODULES(MOZ_GNOMEVFS, gio-2.0 >= $GIO_VERSION [
>+              MOZ_GIO_LIBS=`echo $MOZ_GIO_LIBS | sed 's/-llinc\>//'`
>+            ])
>+        fi

This would need a comma after GIO_VERSION, I assume, and the first argument
would need to be MOZ_GIO, but this doesn't work anyway because MOZ_EXTENSIONS
is not yet set.

That means that the same test for MOZ_GNOMEVFS doesn't work either and so
I guess no one has used that.

Therefore feel free to just leave configure.in as is and --enable-gio will be needed to build the gio extension.

>+LOCAL_INCLUDES = $(MOZ_GTK2_CFLAGS) \
>+                 $(MOZ_GIO_CFLAGS)
>+

>+EXTRA_DSO_LDOPTS = \
>+                  $(XPCOM_GLUE_LDOPTS) \
>+                  $(NSPR_LIBS) \
>+                  $(MOZ_GTK2_LIBS) \
>+                  $(MOZ_GIO_LIBS) \
>+                  $(NULL)

>+#include <gtk/gtk.h>

GTK is no longer used.  I expect gio/gio.h would be sufficient.

>+  mozilla::MonitorAutoEnter mon(mMonitorMountInProgress);
>+  /* Waiting for finish of mount operation thread */
>+  mon.Wait();
>+  g_warning("monitor done");
 +  g_object_unref(mount_op);

The state of the mount operation should be checked before waiting in case the
operation already Notify/signals before this wait starts.  Also
pthread_cond_wait() can wakeup spuriously, so the state should be checked on
return from Wait().  If G_MOUNT_OPERATION_IN_PROGRESS were a
GMountOperationResult this would normally look something like

   while (mMountRes == G_MOUNT_OPERATION_IN_PROGRESS)
     mon.Wait();

>+ * nsGIOInputStream. This function is called in main thread as async request 
>+ * from dbus.

How about "main thread as an async request typically from dbus"?
IIUC, while gvfs is implemented using dbus, GIO need not necessarily use dbus, for other modules for example.

If G_ASK_PASSWORD_NEED_DOMAIN is requested, should the reply be
G_MOUNT_OPERATION_UNHANDLED?

Please remove the debugging g_warnings.
Ideally g_warnings would only be for cases where the code is misused
(and normally we use NS_ASSERTION for that).
Comment 17 Karl Tomlinson (:karlt) 2010-05-18 21:31:12 PDT
(In reply to comment #15)
> Usage of Cancel method for channels shows nice error messages to user which
> depends on gio failure type. Please have a look.

How do you get these error messages and where do you see them?
I tried sftp://notauser@localhost/etc, cancelling and entering the wrong
password 3 times.

And how do you know that it is safe to call nsIInputStreamChannel::Cancel()
from the Read() thread?
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2010-05-18 21:51:56 PDT
> And how do you know that it is safe to call nsIInputStreamChannel::Cancel()
> from the Read() thread?

It's not, unless the Read() thread is the main thread.
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2010-05-18 21:57:49 PDT
I'd think an error rv from Read() would propagate into the channel status.  Does it not?
Comment 20 Jan Horak 2010-05-20 04:54:54 PDT
Created attachment 446488 [details] [diff] [review]
Patch 0.6, warnings removed, Cancel removed

Sorry for confusion with Cancel. Error reporting works without calling it. Sorry also for g_warning leftovers (they was just for my debug runs).

I removed Cancel call, useless g_warnings and configure.in changes.

There is still no feedback when user aborts password dialog or fill wrong login/pass.
Comment 21 Karl Tomlinson (:karlt) 2010-05-23 20:24:09 PDT
Comment on attachment 446488 [details] [diff] [review]
Patch 0.6, warnings removed, Cancel removed

Thank you, Jan.  This is looking good.

(In reply to comment #20)
> There is still no feedback when user aborts password dialog or fill wrong
> login/pass.

I don't think we need to sort that out in order to land this.  The gnomevfs extension seems to leave no feedback on failure often enough.

>+  /* Calling g_file_mount_enclosing_volume creates a new thread */

Is this actually true?  I thought typically it would start a new process and
send a dbus request.  But really it could do anything.  The key is that it is
asynchronous and the reply arrives on the main thread.

>+  /* Waiting for finish of mount operation thread */
>+  while (mMountRes == G_MOUNT_OPERATION_UNHANDLED)

I was initially worried that this might sometimes get set to
G_MOUNT_OPERATION_UNHANDLED as a GIO result of
g_file_mount_enclosing_volume().
But then I remembered that mMountRes is only set to internal values based on
the error from g_file_mount_enclosing_volume_finish.

I think it would be clearer to use an internal enum type for mMountRes with 3 values for the in-progress/failure/success states.
Comment 22 Jan Horak 2011-01-05 05:49:31 PST
Created attachment 501303 [details] [diff] [review]
Patch 0.7, sync to mozilla-central

Please have a look at version synced with latest mozilla-central. When review gets positive I'll try to set need-checkin.
Comment 23 Karl Tomlinson (:karlt) 2011-01-05 14:03:38 PST
The merge with m-c looks good, thanks, but, before checkin, can you correct the "Calling g_file_mount_enclosing_volume creates a new thread" comment and use a new distinct enum type for mMountRes, in line with comment 21, please?

We'll also need to get approval2.0, but as this is not part of the default build, I don't expect that to be a problem.
Comment 24 Karl Tomlinson (:karlt) 2011-01-05 14:08:21 PST
Comment on attachment 501303 [details] [diff] [review]
Patch 0.7, sync to mozilla-central

>+#include "nsIStandardURL.h"
>+#include "nsMimeTypes.h"
>+#include "nsNetUtil.h"
>+#include "mozilla/Monitor.h"
>+#include "nsIStandardURL.h"

One of the nsIStandardURL.h includes can be removed too :)
Comment 25 Jan Horak 2011-01-06 04:24:16 PST
Created attachment 501632 [details] [diff] [review]
Patch 0.8, fixed comments and added mount operation enum

Here it comes...
Comment 26 Karl Tomlinson (:karlt) 2011-01-09 15:33:50 PST
Comment on attachment 501632 [details] [diff] [review]
Patch 0.8, fixed comments and added mount operation enum

>+nsGIOInputStream::SetMountResult(GMountOperationResult result, gint error_code)
>+{
>+  mozilla::MonitorAutoEnter mon(mMonitorMountInProgress);
>+  mMountRes = (result == G_MOUNT_OPERATION_HANDLED) 
>+      ? MOUNT_OPERATION_SUCCESS : MOUNT_OPERATION_FAILED;

SetMountResult is only used in this file and the parameter is not really a
GMountOperationResult.  It should be the internal enum "MountOperationResult".

>+      , mMountRes(MOUNT_OPERATION_IN_PROGRESS)

mMountRes is only used in nsGIOInputStream::MountVolume() so doesn't need to
be initialized here.

>+  /* A g_file_mount_enclosing_volume is using dbus request to mount volume.

"/* g_file_mount_enclosing_volume uses a dbus request to mount the volume."

>+     Callback mount_enclosing_volume_finished is called in main thread 
>+     (not this thread from witch it this method called). */

"(not this thread on which this method is called)."
Comment 27 Jan Horak 2011-01-10 03:38:57 PST
Created attachment 502458 [details] [diff] [review]
Patch 0.9, fixed comments and mount operation enum

Ah, sorry about it, I was too hasty with enums. Thanks for fixing comments too.
Comment 28 Karl Tomlinson (:karlt) 2011-01-10 22:56:55 PST
Comment on attachment 502458 [details] [diff] [review]
Patch 0.9, fixed comments and mount operation enum

>+++ b/extensions/gio/Makefile.in	Mon Jan 10 12:32:39 2011 +0100
>+# This code is based on original Mozilla gnome-vfs extension. It implements
>+# input stream provided by GVFS/GIO.

This license header needs to be made consistent with the original gnomevfs
Makefile.in, in the same way as you fixed up for nsGIOProtocolHandler.cpp.

I think we can request approval2.0 when that's done.
Comment 29 Jan Horak 2011-01-11 02:19:31 PST
Created attachment 502759 [details] [diff] [review]
Patch 0.1, fixed license
Comment 30 Karl Tomlinson (:karlt) 2011-01-11 11:33:33 PST
(In reply to comment #23)
> ... this is not part of the default build ...

To be more precise:
This extension is not enabled by default.  The only change to the default build here is a lookup of the service NS_NETWORK_PROTOCOL_CONTRACTID_PREFIX"moz-gio" (which won't normally be present).  This is similar to what is done for the gnomevfs extension.

The benefit of the patch is that distributions no longer shipping gnomevfs can enable the gio extension for similar behaviour using modern system libraries.
Comment 31 Jan Horak 2011-01-28 06:29:49 PST
Setting checkin-needed, is it possible?
Comment 32 Mounir Lamouri (:mounir) 2011-01-28 06:37:48 PST
(In reply to comment #31)
> Setting checkin-needed, is it possible?

Unfortunately not. This patch need to be approved before landing on mozilla-central because we are too close to the release. If it's not approved, it will be able to be pushed after the release.
Comment 33 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-01-28 12:45:18 PST
I really think we should take this post-FF4.
Comment 34 Johnny Stenback (:jst, jst@mozilla.com) 2011-02-02 14:17:46 PST
Comment on attachment 502759 [details] [diff] [review]
Patch 0.1, fixed license

Agreed, this is too big of a change for Firefox 4 at this point. We'll get this in after branching!
Comment 37 Christian Persch (GNOME) (away; not receiving bug mail) 2011-03-24 16:42:59 PDT
The patch as checked in still is full of debugging g_warnings()s; I think they should be removed, as the reviewer in comment 5 and comment 16 also said.

MapGIOResult(gint) converts GIOErrorEnum codes to nsresult, but MapGIOResult(GError*) hands it any GError's ->code without first checking that ->domain == G_IO_ERROR (there's no guarantee all those gio functions only return G_IO_ERROR errors).
Comment 38 Ben Bucksch (:BenB) 2014-08-25 13:17:18 PDT
xref bug 1058177 - gvfs within firefox process using 100% disk.

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