Bug #539226 “firefox(-gnome-support) should be compiled with Gio...” : Bugs : firefox package : Ubuntu

Revision history for this message

In Mozilla Bugzilla #494163, Jhorak (jhorak) wrote on 2009-05-21:

#1

Created an attachment (id=378853)
Initial extension patch

Use "--enable-extensions=gio" to compile with gio module.

Revision history for this message

In Mozilla Bugzilla #494163, Caillon (caillon) wrote on 2009-08-04:

#2

closing in favor of the other bug

*** This bug has been marked as a duplicate of bug 402892 ***

Revision history for this message

In Mozilla Bugzilla #494163, Jhorak (jhorak) wrote on 2009-08-10:

#3

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.

Revision history for this message

In Mozilla Bugzilla #494163, Roc-ocallahan (roc-ocallahan) wrote on 2009-09-06:

#4

(From update of attachment 378853)
I only took a cursory look at the patch --- i.e. this is really an sr, and really not an r :-)

Revision history for this message

In Mozilla Bugzilla #494163, Karlt (karlt) wrote on 2009-09-11:

#5

Download full text (6.9 KiB)

(From update of attachment 378853)
>+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 b...

(From update of attachment 378853)
>+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?

Revision history for this message

In Mozilla Bugzilla #494163, Jhorak (jhorak) wrote on 2009-09-18:

#6

Created an attachment (id=401429)
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.

Revision history for this message

In Mozilla Bugzilla #494163, Karlt (karlt) wrote on 2009-10-01:

#7

(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.

Revision history for this message

In Mozilla Bugzilla #494163, Reed Loden (reed) wrote on 2010-01-05:

#8

Jan, any updates on this?

Revision history for this message

In Mozilla Bugzilla #494163, Jhorak (jhorak) wrote on 2010-01-12:

#9

Created an attachment (id=421233)
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?

Revision history for this message

In Mozilla Bugzilla #494163, Jhorak (jhorak) wrote on 2010-01-12:

#10

Created an attachment (id=421249)
Patch 0.3b, switched from gtk_mount_operation back to nsIAuthPrompt

Sorry, bogus diff parts.

Revision history for this message

In Mozilla Bugzilla #494163, Sgautherie-bz (sgautherie-bz) wrote on 2010-02-10:

#11

(From update of attachment 421249)
>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?

Revision history for this message

In Mozilla Bugzilla #494163, Jhorak (jhorak) wrote on 2010-02-11:

#12

Created an attachment (id=426480)
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.

Revision history for this message

Javier Jardón (jjardon) wrote on 2010-03-15:

#13

Binary package hint: firefox

GnomeVFS is deprecated and won't be included in future GNOME releases

Also, GNOME support in mozilla seems quite modular and can be selected the desired libraries:
https://hg.mozilla.org/mozilla-central/file/69cb1df1cb0f/toolkit/system/gnome/nsGnomeModule.cpp

Bug Watch Updater (bug-watch-updater) on 2010-04-16

Changed in firefox:
status:	Unknown → Confirmed

Revision history for this message

In Mozilla Bugzilla #494163, Karlt (karlt) wrote on 2010-05-11:

#14

Download full text (7.0 KiB)

(From update of attachment 426480)
(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....

(From update of attachment 426480)
(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.

Revision history for this message

In Mozilla Bugzilla #494163, Karlt (karlt) wrote on 2010-05-12:

#15

(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.

Revision history for this message

In Mozilla Bugzilla #494163, Jhorak (jhorak) wrote on 2010-05-17:

#16

Created an attachment (id=445660)
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.

Revision history for this message

In Mozilla Bugzilla #494163, Karlt (karlt) wrote on 2010-05-18:

#17

(From update of attachment 445660)
>+ 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).

(From update of attachment 445660)
>+    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).

Revision history for this message

In Mozilla Bugzilla #494163, Karlt (karlt) wrote on 2010-05-18:

#18

(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?

Revision history for this message

In Mozilla Bugzilla #494163, Bzbarsky (bzbarsky) wrote on 2010-05-18:

#19

> 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.

Revision history for this message

In Mozilla Bugzilla #494163, Bzbarsky (bzbarsky) wrote on 2010-05-18:

#20

I'd think an error rv from Read() would propagate into the channel status. Does it not?

Revision history for this message

In Mozilla Bugzilla #494163, Jhorak (jhorak) wrote on 2010-05-20:

#21

Created an attachment (id=446488)
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.

Revision history for this message

In Mozilla Bugzilla #494163, Karlt (karlt) wrote on 2010-05-23:

#22

(From update of attachment 446488)
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.

Bug Watch Updater (bug-watch-updater) on 2010-09-18

Changed in firefox:
importance:	Unknown → Wishlist

Revision history for this message

In Mozilla Bugzilla #494163, Jhorak (jhorak) wrote on 2011-01-05:

#23

Created attachment 501303
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.

Revision history for this message

In Mozilla Bugzilla #494163, Karlt (karlt) wrote on 2011-01-05:

#24

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.

Revision history for this message

In Mozilla Bugzilla #494163, Karlt (karlt) wrote on 2011-01-05:

#25

Comment on attachment 501303
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 :)

Revision history for this message

In Mozilla Bugzilla #494163, Jhorak (jhorak) wrote on 2011-01-06:

#26

Created attachment 501632
Patch 0.8, fixed comments and added mount operation enum

Here it comes...

Revision history for this message

In Mozilla Bugzilla #494163, Karlt (karlt) wrote on 2011-01-09:

#27

Comment on attachment 501632
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)."

Revision history for this message

In Mozilla Bugzilla #494163, Jhorak (jhorak) wrote on 2011-01-10:

#28

Created attachment 502458
Patch 0.9, fixed comments and mount operation enum

Ah, sorry about it, I was too hasty with enums. Thanks for fixing comments too.

Revision history for this message

In Mozilla Bugzilla #494163, Karlt (karlt) wrote on 2011-01-11:

#29

Comment on attachment 502458
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.

Revision history for this message

In Mozilla Bugzilla #494163, Jhorak (jhorak) wrote on 2011-01-11:

#30

Created attachment 502759
Patch 0.1, fixed license

Revision history for this message

In Mozilla Bugzilla #494163, Karlt (karlt) wrote on 2011-01-11:

#31

(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.

Revision history for this message

In Mozilla Bugzilla #494163, Jhorak (jhorak) wrote on 2011-01-28:

#32

Setting checkin-needed, is it possible?

Revision history for this message

In Mozilla Bugzilla #494163, Mounir-lamouri (mounir-lamouri) wrote on 2011-01-28:

#33

(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.

Revision history for this message

In Mozilla Bugzilla #494163, Roc-ocallahan (roc-ocallahan) wrote on 2011-01-28:

#34

I really think we should take this post-FF4.

Bug Watch Updater (bug-watch-updater) on 2011-01-29

Changed in firefox:
status:	Confirmed → In Progress

Revision history for this message

In Mozilla Bugzilla #494163, Jst (jst) wrote on 2011-02-02:

#35

Comment on attachment 502759
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!

Revision history for this message

In Mozilla Bugzilla #494163, Ehsan-mozilla (ehsan-mozilla) wrote on 2011-03-24:

#36

http://hg.mozilla.org/projects/cedar/rev/7c42f37e0284

Revision history for this message

In Mozilla Bugzilla #494163, Ehsan-mozilla (ehsan-mozilla) wrote on 2011-03-24:

#37

http://hg.mozilla.org/mozilla-central/rev/7c42f37e0284

Revision history for this message

In Mozilla Bugzilla #494163, Chpe (chpe) wrote on 2011-03-24:

#38

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).

Bug Watch Updater (bug-watch-updater) on 2011-03-28

Changed in firefox:
status:	In Progress → Fix Released

Revision history for this message

Micah Gersten (micahg) wrote on 2011-09-14:

#39

Starting with Ubuntu 11.04, Firefox is compiled with gio. Please report any other issues you may find.

Changed in firefox (Ubuntu):
status:	New → Fix Released

Revision history for this message

Alexandr (olexandr-dmitriev) wrote on 2012-09-04:

#40

Hi!
When I go to File - Open - I don't see any network bookmars, only local folder.

Revision history for this message

In Mozilla Bugzilla #494163, Ben-bucksch (ben-bucksch) wrote on 2014-08-25:

#41

xref bug 1058177 - gvfs within firefox process using 100% disk.

Affects		Status	Importance	Assigned to	Milestone
	Mozilla Firefox	Fix Released	Wishlist	mozilla-bugs #494163
	firefox (Ubuntu)	Fix Released	Undecided	Unassigned

Ubuntu
firefox package

firefox(-gnome-support) should be compiled with Gio support instead GnomeVFS

Bug Description

Other bug subscribers

Remote bug watches

Ubuntufirefox package

firefox(-gnome-support) should be compiled with Gio support instead GnomeVFS

Bug Description

Other bug subscribers

Remote bug watches

Ubuntu
firefox package