Closed Bug 994216 Opened 6 years ago Closed 6 years ago

Fix linux mochitest-other on AWS nodes

Categories

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

x86_64
Linux
defect

Tracking

(firefox29 unaffected, firefox30 fixed, firefox31 fixed, firefox-esr24 unaffected)

RESOLVED FIXED
mozilla31
Tracking Status
firefox29 --- unaffected
firefox30 --- fixed
firefox31 --- fixed
firefox-esr24 --- unaffected

People

(Reporter: catlee, Assigned: myk)

References

Details

(Keywords: ateam-unittests-task, Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

No description provided.
Could I have access to one of the machines to try and debug the test error?
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.
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);
  }
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.
And here it is in patch form.  This should work, although I'm still in the process of building and testing it.
Assignee: nobody → myk
Status: NEW → ASSIGNED
Attachment #8404188 - Flags: review?(mar.castelluccio)
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 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.
(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.
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.
the tests should have a common cleanup function for (rm -Rf ~/.local/share/applications)
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.
Attachment #8404188 - Flags: review?(mar.castelluccio) → feedback+
(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.
Priority: -- → P1
(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.
Attachment #8404188 - Attachment is obsolete: true
Attachment #8404223 - Flags: review?(mar.castelluccio)
Attachment #8404223 - Flags: review?(mar.castelluccio) → review+
(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.
I think you can avoid the "desktopINIfile.parent" check, but the patch is good anyway.
(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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → B2G C4 (2jan on)
Component: General Automation → DOM: Apps
Product: Release Engineering → Core
QA Contact: catlee
Target Milestone: B2G C4 (2jan on) → mozilla31
Version: unspecified → Trunk
Whiteboard: [qa-]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.