714168 - make reftest-remote seems to fail on tablets

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

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

bz found an issue while trying to run make reftest-remote on his eee tablet.  I was able to confirm on my galaxy tablet, but not on the tegra.  This is all with ADB vs SUT.

What we see is about 4 of these:
12:25 < bz> and from logcat: E/GeckoConsole(23115): [JavaScript Error: "ERROR addons.xpi: Failed to move entry 
            /mnt/sdcard/tests/reftest/profile/extensions/staged/[e-mail]/chrome/reftest: [Exception... 
            "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.permissions]"  
            nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: 
            resource://gre/modules/XPIProvider.jsm :: <TOP_LEVEL> :: line 
12:25 -!- qheaden_away [[e-mail]] has joined #developers
12:25 < bz> E/GeckoConsole(23115): [JavaScript Error: "ERROR addons.xpi: Failure moving 
            /mnt/sdcard/tests/reftest/profile/extensions/staged/[e-mail]/chrome to 
            /mnt/sdcard/tests/reftest/profile/extensions/[e-mail]" {file: 
            "resource://gre/modules/XPIProvider.jsm" line: 262}]


So for some reason we are creating the staged extensions, but we are unable to move them over to the unstaged area.

bz found this on xul fennec and I found this on native fennec.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is blocking bug 598482 at the moment, because I need to debug the reftest fails there somehow....

Comment 2

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

in automation.py.in, I did:
#    extensionsRootDir = os.path.join(profileDir, "extensions", "staged")
    extensionsRootDir = os.path.join(profileDir, "extensions")

and it worked.  So the question is why when we do the extensions/staged directory do we fail on android.  

a hack around could be to ignore the 'staged' for android tests, but I don't think that is a solution I want to check in.  I need to learn more about what we need the 'staged' directory for.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> in automation.py.in, I did:

Works for me too.  I can actually run reftests now and get useful output!  Thanks!

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We need to make --ignore-window-size an explicit override for reftest-remote.  bz and I were chasing down a test that was failing silently because of this.  If I didn't know to look for this, we could have wasted a lot of time.  Luckily it still fails with the right window size.

I may have r+'d the patch that added that originally, in which case I've changed my mind ;).

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Another note: we need to LogInfo() the dirty rects here: http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.js#1269 .

Comment 6

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

so the --ignore-window-size is a way to run tests without an exception:
http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.js#1200

iirc, it is required to have the proper resolution otherwise we cannot run the tests without getting bogus results.  In order to solve that problem we would need to have a reftest tag to indicate resolution required for the test (i.e. resolution.x > 1000).

The tegras (in buildbot automation) run with proper resolution and do not require --ignore-window-size.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Joel Maher (:jmaher) from comment #6)
> iirc, it is required to have the proper resolution otherwise we cannot run
> the tests without getting bogus results.

That's the problem: bz had his tablet in landscape mode, so was seeing a test fail, but there's no way to tell whether it's a bogus failure or a "real" failure without significant investigation.  The error in the "else" case you linked very loudly says, "you just got a possibly bogus result".  (In this case, bz rotated to portrait and we saw the same failure.  Whew.)

I would much rather have developers say "I don't care about bogus failures" explicitly, than assume that bad results are valid and end up debugging the wind.  For the other reftest-ipc targets, developers need to explicitly pass --ignore-window-size.

Yeah, this doesn't affect tinderbox.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Notes
 - LayerManager::PrintInfo should say when it's not rendering to its default target
 - we should LogInfo() *after* the drawWindow() in DoDrawWindow() finishes, too

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

so, the root cause of the issue here is nsLocalFileUnix does not like operating on a FAT file system, which is where devicemangerADB likes to put the profile. I would imagine that we would run into similar for users who move their profile to the sdcard (which is another good reason for use to remove that feature).

Rather than make nsLocalFileUnix more sane, this simply adds try-catch statements around key sections of the addon installation code. Most of this is Mossop's work, but I'm still going to ask him to review my additions to it and let him worry about who else needs to take a look.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> We need to make --ignore-window-size an explicit override for
> reftest-remote.  bz and I were chasing down a test that was failing silently
> because of this.  If I didn't know to look for this, we could have wasted a
> lot of time.  Luckily it still fails with the right window size.
> 
> I may have r+'d the patch that added that originally, in which case I've
> changed my mind ;).

on that subject I think the better approach would be to manifest tests that require a large window separate from tests that don't

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 591383 [details] [diff] [review]
patch

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

Should spin this through Blair to check I'm not being crazy but I think this is good. Generally we only set permissions in the hope that it allows a later operation (delete/move/etc.) to succeed. If setting the permissions fails then it isn't a guarantee that the operation will fail too so might as well carry on.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +201,4 @@
>    })
>  }, this);
>  
> +function setPermissions(aFile, aPermissions) {

Should add a javadoc comment describing this function and its arguments.

@@ +1095,5 @@
>          continue;
>  
>        zipReader.extract(entryName, target);
> +      try {
> +          target.permissions |= FileUtils.PERMS_FILE;

Only indent 2 spaces

Comment 12

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

(In reply to Brad Lassey [:blassey] from comment #10)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> > We need to make --ignore-window-size an explicit override for
> > reftest-remote.  bz and I were chasing down a test that was failing silently
> > because of this.  If I didn't know to look for this, we could have wasted a
> > lot of time.  Luckily it still fails with the right window size.
> > 
> > I may have r+'d the patch that added that originally, in which case I've
> > changed my mind ;).
> 
> on that subject I think the better approach would be to manifest tests that
> require a large window separate from tests that don't

This general topic should be resolved in: bug 692278.  It would be nice to get some definitions in the reftest manifest for window size.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Brad Lassey [:blassey] from comment #10)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> > We need to make --ignore-window-size an explicit override for
> > reftest-remote.  bz and I were chasing down a test that was failing silently
> > because of this.  If I didn't know to look for this, we could have wasted a
> > lot of time.  Luckily it still fails with the right window size.
> > 
> > I may have r+'d the patch that added that originally, in which case I've
> > changed my mind ;).
> 
> on that subject I think the better approach would be to manifest tests that
> require a large window separate from tests that don't

As has been discussed several times previously, that works OK for not displaying errors for tests that fail on small screens.  It does *not* avoid bogus test results in local runs.  If the screen is too small, false negatives are possible.  To avoid those, you would need to audit every single test and mark the ones that rely on comparisons outside of (what screen area?) as needed a large screen.  I don't have the time for that, and I don't think you do either.

So the target should still be named differently, say |make approx-reftest|, since devs aren't getting meaningful results in general.

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 591383 [details] [diff] [review]
patch

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

Bug summary scared me for a second there :P

r+ with these and Dave's comments met.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +201,4 @@
>    })
>  }, this);
>  
> +function setPermissions(aFile, aPermissions) {

Lets name this setFilePermissions, to avoid any potential confusion.

@@ +1099,5 @@
> +          target.permissions |= FileUtils.PERMS_FILE;
> +      }
> +      catch (e) {
> +        WARN("Failed to set permissions " + aPermissions.toString(8) + " on " +
> +             aFile.path, e);

There's no aFile in this function... your warning will throw :)

@@ +8094,5 @@
>      newFile.append(aSource.leafName);
> +    try {
> +      newFile.lastModifiedTime = Date.now();
> +    } catch (e)  {
> +      WARN("failed to set lastModifiedTime on " + newFile, e);

Probably want to log newFile.path here.

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

I fixed up according to review comments and pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/f583132674d5 and that turned everything orange, so I backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/2b74b62701e2

I'll investigate what when wrong tomorrow

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

here is a green test run with the original patch https://tbpl.mozilla.org/?tree=Try&rev=385d51501f06

So it looks like I screwed up in addressing review comments as opposed to there being something fundamentally wrong with the patch.

Comment 17

[Dave Townsend [:mossop]]

(In reply to Brad Lassey [:blassey] from comment #16)
> here is a green test run with the original patch
> https://tbpl.mozilla.org/?tree=Try&rev=385d51501f06
> 
> So it looks like I screwed up in addressing review comments as opposed to
> there being something fundamentally wrong with the patch.

You renamed the function to setFilePermissions but all the function calls are still setPermissions.

Please also correct the indentation of the function comment and put a newline before, not after.

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Mossop, thanks for doing my debugging for me :-)

re-landed https://hg.mozilla.org/integration/mozilla-inbound/rev/c30b587ed067

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c30b587ed067