994216 - Fix linux mochitest-other on AWS nodes

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect, P1)

Comment 1

[Marco Castelluccio [:marco]]

Could I have access to one of the machines to try and debug the test error?

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

got a loaner machine, hacked on it for a while.

http://dxr.mozilla.org/mozilla-central/source/toolkit/webapps/tests/test_hosted.xul#

desktopINI value is:
/home/cltbld/.local/share/applications/owa-samplehostedapp-2fde7c13b95438da5839bb037568d46d.desktop

ls -la /home/cltbld/.local/share/
total 28
drwxr-xr-x 6 cltbld cltbld 4096 Apr  9 12:34 .
drwxr-xr-x 3 cltbld cltbld 4096 Apr  2 12:11 ..
-rw-rw-r-- 1 cltbld cltbld    0 Apr  2 12:11 .converted-launchers
-rw-rw-r-- 1 cltbld cltbld  835 Apr  2 12:11 gsettings-data-convert
drwx------ 2 cltbld cltbld 4096 Apr  2 17:44 gvfs-metadata
drwx------ 3 cltbld cltbld 4096 Apr  2 12:11 telepathy
drwx------ 3 cltbld cltbld 4096 Apr  2 12:11 zeitgeist
[cltbld@localhost jmaher]$ 


mkdir /home/cltbld/.local/share/applications

run tests and they pass.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

If the problem is that the desktop INI file's parent directory doesn't exist, then the fix should be to ensure it exists by creating it if needed after retrieving its path in _createSystemFiles <http://hg.mozilla.org/mozilla-central/annotate/5a5ed08df529/toolkit/webapps/LinuxNativeApp.js#l279>:

  let desktopINIfile = getFile(this.desktopINI);
  if (desktopINIfile.parent && !desktopINIfile.parent.exists()) {
    desktopINIfile.parent.createUnique(Ci.nsIFile.DIRECTORY_TYPE, PERMS_DIRECTORY);
  }

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

looking in nativeapp, we create the ini file(s) in a tmp dir, then we move the directory to the path in question (~/.local/share/applications) which in this bug, doesn't exist.

In linuxNativeApp, we do this:
http://dxr.mozilla.org/mozilla-central/source/toolkit/webapps/LinuxNativeApp.js?from=linuxnativeapp.js#166

Here is move directory:
http://dxr.mozilla.org/mozilla-central/source/toolkit/webapps/NativeApp.jsm#374


I could be on the wrong path, :myk helped me out a bit in IRC.  If we can fix this in the *nativeApp.js files, I believe that would be the ideal fix.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: ensure desktop INI file parent directory exists

And here it is in patch form.  This should work, although I'm still in the process of building and testing it.

Comment 6

[Chris AtLee [:catlee]]

If we're going to dump stuff anywhere outside the test directory or profile directory, can we also make sure to clean up after?

Comment 7

[Marco Castelluccio [:marco]]

(In reply to Chris AtLee [:catlee] from comment #6)
> If we're going to dump stuff anywhere outside the test directory or profile
> directory, can we also make sure to clean up after?

In the test we have a cleanup function that cleans everything but the temp directory, that is cleaned up by the NativeApp.js code itself.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

(In reply to Joel Maher (:jmaher) from comment #4)
> looking in nativeapp, we create the ini file(s) in a tmp dir, then we move
> the directory to the path in question (~/.local/share/applications) which in
> this bug, doesn't exist.

It looks like we create the "webapp" INI file in a temporary directory but the "desktop" INI file in its final location.

webappINI is set to simply "webapp.ini":

http://hg.mozilla.org/mozilla-central/annotate/5a5ed08df529/toolkit/webapps/LinuxNativeApp.js#l19

And then it's retrieved relative to the base directory passed into _createConfigFiles:

http://hg.mozilla.org/mozilla-central/annotate/5a5ed08df529/toolkit/webapps/LinuxNativeApp.js#l260

But desktopINI is set to an absolute path:

http://hg.mozilla.org/mozilla-central/annotate/5a5ed08df529/toolkit/webapps/LinuxNativeApp.js#l36

And then it's retrieved without reference to a base directory:

http://hg.mozilla.org/mozilla-central/annotate/5a5ed08df529/toolkit/webapps/LinuxNativeApp.js#l278

After which it's used to create an nsIINIParserWriter on which we eventually call writeFile, which is the method in which the failure is reported (specifically: "nsINIProcessor.js :: INIProcessor.prototype.writeFile :: line 147," per bug 991274).  So I suspect this is the source of the failure.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

verified the patch fixes the problem on the loaner slave.

As many trees are closed, we should land this asap and work on opening trees.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

the tests should have a common cleanup function for (rm -Rf ~/.local/share/applications)

Comment 11

[Marco Castelluccio [:marco]]

Comment on attachment 8404188 [details] [diff] [review]
patch v1: ensure desktop INI file parent directory exists

Review of attachment 8404188 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/webapps/LinuxNativeApp.js
@@ +278,5 @@
>      // $XDG_DATA_HOME/applications/owa-<webappuniquename>.desktop
>      let desktopINIfile = getFile(this.desktopINI);
> +    if (desktopINIfile.parent && !desktopINIfile.parent.exists()) {
> +      desktopINIfile.parent.createUnique(Ci.nsIFile.DIRECTORY_TYPE, PERMS_DIRECTORY);
> +    }

Here you can just use |nsIFile::create|, because we need the directory to be exactly "~/.local/share/applications".

If you want, you could use OS.File.makeDir and Task.async here.

Comment 12

[Marco Castelluccio [:marco]]

(In reply to Joel Maher (:jmaher) from comment #10)
> the tests should have a common cleanup function for (rm -Rf
> ~/.local/share/applications)

I think we shouldn't delete the entire directory. It is a standard directory used by other software too.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: use create instead of createUnique

(In reply to Marco Castelluccio [:marco] from comment #11)
> Here you can just use |nsIFile::create|, because we need the directory to be
> exactly "~/.local/share/applications".

Indeed!  It shouldn't matter, since we just checked that !desktopINIfile.parent.exists(), but it's more correct.  Here's a patch with that change.


> If you want, you could use OS.File.makeDir and Task.async here.

In the interest of landing the simplest fix for the test failure, I'll leave that to a future change.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #5)
> And here it is in patch form.  This should work, although I'm still in the
> process of building and testing it.

My build completed, and I confirmed that the patch passes tests, whether or not ~/.local/share/applications exists.

Comment 15

[Marco Castelluccio [:marco]]

I think you can avoid the "desktopINIfile.parent" check, but the patch is good anyway.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

(In reply to Marco Castelluccio [:marco] from comment #15)
> I think you can avoid the "desktopINIfile.parent" check, but the patch is
> good anyway.

That's highly likely.  I did it out of an abundance of caution (nsIFile.parent can be null, although presumably it can't be null in this case).


Per RyanVM on IRC, I landed this directly on Central:

https://hg.mozilla.org/mozilla-central/rev/690c810c8e3e

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Merged around, thanks :)

https://hg.mozilla.org/releases/mozilla-aurora/rev/3770f546df84