Running reftests on an Android device may use stale cached test/reference files

RESOLVED FIXED in Firefox 57

Status

()

Firefox for Android
Testing
P1
normal
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: jfkthame, Assigned: gbrown)

Tracking

unspecified
Firefox 57
Unspecified
Android
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

9 months ago
STR (for an Android device, using locally-built Fennec from a mozilla-central checkout on macOS):

- Do "./mach reftest path/to/test/directory" to run a bunch of tests

- Edit one of the test or reference files, so as to fix a failure (or introduce a new one, for testing purposes)

- Do "./mach reftest path/to/test/directory" again

Expected:
The second test run should use the modified files and hence give different results than the first.

Actual:
The second test run uses the old, unmodified files.

Rebuilding/repackaging/reinstalling Fennec from the desktop system does not help; the stale test files are still used on the device.

To avoid the problem, it is necessary to "Clear Cache" on the Android device. This is a pain when attempting to develop/fix tests -- both because it adds a cumbersome step to the edit/test cycle, and because it's easy to forget to do it and thus waste time testing something other than what is intended -- and potentially much more time trying to understand results that don't match the current test files.

In local testing, I found that hacking netwerk/test/httpserver/httpd.js to forcibly add a "Cache-Control: must-revalidate" header to all its responses appears to work around the issue, though I don't know what additional implications this might have for other parts of our testing.
Wow, this is interesting! It sounds like the cached copy is coming from the necko cache. That cache was, at least in the distant past, in the firefox profile, which is deleted between test runs. Has the necko cache moved to the Android cache now? Does it survive there even after Fennec is re-installed? I think that would explain everything described here. I will try to reproduce and verify.
Assignee: nobody → gbrown
Blocks: 1364381
Component: Reftest → Testing
Priority: -- → P1
Product: Testing → Firefox for Android
Version: Version 3 → unspecified
See Also: → bug 742558, bug 961024
Thanks to the STR, this was easy to verify. In my experience - running 'mach reftest layout/reftests/inline' with/without modification - a cached file was used most of the time, but not always. (I suppose the modified test file might be evicted, or expire from the necko cache.) If I cleared the Android cache for Fennec before a run - from Android Settings, or with 'adb shell pm clear org.mozilla.fennec_...' - test results were always consistent with the test file on the host.

On the Android 4.3 emulator, after a test run, I see the necko cache at /data/data/org.mozilla.fennec_gbrown/cache/profile/cache2. That seems consistent with intended design - see See Also bugs. 

AFAIK, our test harnesses do nothing to manipulate this cache. (Test harnesses set up a fresh profile for each run, but the cache is independent of the profile, on Android.)

The test harnesses could easily run 'adb shell pm clear <app>' before starting tests, and that would work in some cases, but I believe it would fail without root privileges.

I think we should not modify httpd.js or otherwise modify Cache-Control or related headers. That might interfere with some test results and performance.

:honza -- Do you have any ideas here? Are there prefs or environment variables that could be used to signal necko to discard any existing cache found on startup, or to change the location of the cache?
Flags: needinfo?(odvarko)
Flags: needinfo?(odvarko) → needinfo?(honzab.moz)
(In reply to Geoff Brown [:gbrown] from comment #2)
 or to change the location of the cache?

browser.cache.disk.parent_directory
Flags: needinfo?(honzab.moz)

Updated

9 months ago
See Also: → bug 1389249
(In reply to Honza Bambas (:mayhemer) from comment #3)
> (In reply to Geoff Brown [:gbrown] from comment #2)
>  or to change the location of the cache?
> 
> browser.cache.disk.parent_directory

That works great - thanks :mayhemer (sorry for the confusion :Honza!).
Created attachment 8896533 [details] [diff] [review]
move necko cache location and ensure it is empty

In each of the Android mochitest and reftest harnesses, I've used a pref to change the location of the necko cache from its default to a new location within the "test root". I explicitly delete that directory at harness cleanup and again at initialization time (in case the previous run was interrupted).

Testing locally ('mach reftest ...' with emulator on Ubuntu 16), I now find the test results are always consistent with my changes to the test files on the host.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b0bc2ea6d30c652d5bbddc316ae3942b422eb1f confirms I've done no obvious harm to emulator unit tests.

I had been concerned that deleting the cache after a long test run might make a test job run for too long, but spot checks on the try push suggest there's no obvious difference to job run time.


Jonathan - Does this patch resolve the issue for you?
Attachment #8896533 - Flags: review?(bob)
Attachment #8896533 - Flags: feedback?(jfkthame)
(Reporter)

Comment 7

9 months ago
Comment on attachment 8896533 [details] [diff] [review]
move necko cache location and ensure it is empty

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

Yes, this appears to work as expected for me. Thanks!
Attachment #8896533 - Flags: feedback?(jfkthame) → feedback+

Comment 8

9 months ago
Comment on attachment 8896533 [details] [diff] [review]
move necko cache location and ensure it is empty

Looks good. Sorry for the delay.
Attachment #8896533 - Flags: review?(bob) → review+

Comment 9

9 months ago
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40095d291eb4
Ensure a fresh necko cache for each mochitest and reftest run; r=bc
Duplicate of this bug: 1364381

Comment 11

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/40095d291eb4
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.