Comment 14 for bug 539226

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