Closed Bug 1132001 Opened 8 years ago Closed 8 years ago

TEST-UNEXPECTED-FAIL | Linux systems can fail to have a mailto handler, and this causes test_handlerService.js to fail.


(Core Graveyard :: File Handling, defect)

Not set


(firefox38 fixed)

Tracking Status
firefox38 --- fixed


(Reporter: gareth, Unassigned)



(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150115213607

Steps to reproduce:

I ran ./mach xpcshell-test uriloader/exthandler/tests/unit/test_handlerService.js 

Actual results:

 0:00.88 TEST_STATUS: Thread-1 run_test FAIL [run_test : 164] false == true
 0:00.88 LOG: Thread-1 INFO exiting test

Expected results:

This test should not have attempted to use a mailto handler on my system, since I do not have a mailto handler.

The file test_handlerService.js contains a noMailTo variable, which is true if the OS is windows and if there is no mailto: handler ; false otherwise. My system is linux, and also does not provide a mailto handler. The noMailTo variable should be set to true on my system. This means detecting the presence or absence of a mailto: handler on linux.

This is related to which noticed the same problem in windows, and presumably led to the creation of the noMailTo variable ; and to which suggests that it might be better in the long term to create a temporary handler for testing.
I'm using nixos 14.12 with the following firefox dev environment:

    firefoxEnv = pkgs.myEnvFun {
      name = "firefoxEnv";
      buildInputs = [ stdenv pkgconfig gtk glib gobjectIntrospection
                      dbus_libs dbus_glib alsaLib gcc xlibs.libXrender
                      xlibs.libX11 xlibs.libXext xlibs.libXft xlibs.libXt
                      ats pango freetype fontconfig gdk_pixbuf cairo python
                      git autoconf213 unzip zip yasm alsaLib dbus_libs which atk
                      gstreamer gst_plugins_base pulseaudio emacs mercurial 
      extraCmds = ''
       export C_INCLUDE_PATH=${dbus_libs}/include/dbus-1.0:${dbus_libs}/lib/dbus-1.0/include
       export CPLUS_INCLUDE_PATH=${dbus_libs}/include/dbus-1.0:${dbus_libs}/lib/dbus-1.0/include
       for i in $nativeBuildInputs; do
       export LD_LIBRARY_PATH
       export AUTOCONF=autoconf
Component: Untriaged → File Handling
Product: Firefox → Core
Attached patch Attempting to fix the test. (obsolete) — Splinter Review
With this patch, the test passes on my machine. However, I'm nervous. I haven't been able to test on a machine which has a mailto handler correctly set by gconf, so I can only be sure that things are ok in the absence of a mailto handler. In particular, I'm assuming that (on a box with mailto set up) the magic string "mailto:" correctly identifies the mailto URI Scheme for the gio service.

Could someone please test this patch on a machine with properly set up mailto handlers?
Tested on debian jessie with xfce, which has default mailto handler in gnome setting.

The string passed to nsIGIOService.getAppForURIScheme should be "mailto" (no trailing colon). With that fix it passes the test on my environment, cool :D

(btw, in the comment and variable name, I guess it's not gconf but gio. sorry if my explanation wasn't clear)
Attached patch Fixing the test: attempt 2 (obsolete) — Splinter Review
Thanks arai -- the misleading comments were my mistake.

This update to the patch does s/gConf/GIO/ to fix that mistake, and s/mailto:/mailto/ so that the test passes on your machine. The test still passes on my machine.

I'm requesting that this patch be reviewed. Apologies if I picked the wrong reviewer -- I looked for someone who had both committed changes to this file, and who had reviewed changes committed to this file.
Attachment #8562849 - Attachment is obsolete: true
Attachment #8563349 - Flags: review?(dolske)
Comment on attachment 8563349 [details] [diff] [review]
Fixing the test: attempt 2

Review of attachment 8563349 [details] [diff] [review]:

Thanks for the patch! Looks good to me assuming it really does throw in the case I noted.

::: uriloader/exthandler/tests/unit/test_handlerService.js
@@ +44,5 @@
>      regSvc.close();
>    }
> +  let isGIO = (";1" in Components.classes);
> +  if (isGIO) {

Optional nitpick: this should probably be "isLinux" or "hasGIO". The check above it uses "isWindows" because only Windows has the windows registry. We use similar checks in other tests (this test is a little unusual in that it then actually looks in the registry, usually it's just a hack to determine the platform for other reasons). I'd probably just use "isLinux" for consistency/clarity with that.

@@ +53,5 @@
> +    try {
> +      gIOSvc.getAppForURIScheme("mailto");
> +      noMailto = false;
> +    } catch (ex) {
> +      noMailto = true;

Huh. This API actually _throws_ when no handler exists?

I guess that makes since, since it was probably written for C++ callers, where checking a return value instead of an outval is more common.
Attachment #8563349 - Flags: review?(dolske) → review+
Oh, first patch, even. \o/ Do you need someone to commit this, or are you already familiar with the process?
Thanks Justin! I've changed the variable name as suggested in this latest revision. I don't think I have commit access, so I think I'll have to ask for your help to commit it.

The getAppForURIScheme call does indeed throw on my machine. I used the REPL to read the exception. It says:

"Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIGIOService.getAppForURIScheme]".

When you approved the patch I was sent an email suggesting I create a mozillians account. Would you mind vouching for me? Feel free to email me at the address in the patch if you need any kind of link/ID or similar from me.
Attachment #8563349 - Attachment is obsolete: true
Attachment #8564163 - Flags: review?(dolske)
Comment on attachment 8564163 [details] [diff] [review]
Changing variable names as suggested

Oh no! I totally missed your updated patch, sorry for the delay.

I just pushed this to Try, just to be extra sure it doesn't break any tests (but I wouldn't expect it to).

Results should come in over the next few hours, I'll get it landed once it's clear so that it'll be in Firefox 38.
Attachment #8564163 - Flags: review?(dolske) → review+
All green, landed!

Odds are fx-team will merge to mozilla-central sometime today, and your patch will be in tomorrow's Nightly build. You'll see some automated comments in this bug when that does happen. [Also, Monday is the next uplift date, so next week's release of Aurora / Developer Edition based on 38 will have this too.]

Did you already create a Mozillians account?
Ever confirmed: true
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Thanks both!

Justin: I did indeed create a mozillians account using the name "gds". If you are willing and able to vouch for me, I'd very much appreciate it.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.