New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 804348 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug

Blocking:
issue 784331



Sign in to add a comment

DirReaderPosixUnittest.Read failing on internal.client.clank/asan-clang-phone

Project Member Reported by sheriff-...@appspot.gserviceaccount.com, Jan 22 2018

Issue description

Filed by sheriff-o-matic@appspot.gserviceaccount.com on behalf of aberent@google.com

DirReaderPosixUnittest.Read failing on internal.client.clank/asan-clang-phone

Builders failed on: 
- asan-clang-phone: 
  https://uberchromegw.corp.google.com/i/internal.client.clank/builders/asan-clang-phone


 
Components: Internals>Core
Owner: brettw@chromium.org
Status: Assigned (was: Available)
Looking at the code of the test (https://cs.chromium.org/chromium/src/base/files/dir_reader_posix_unittest.cc?rcl=85b9d0662601895ecb6b25977ce3117725d0d525&l=35) the failure is when it tries to open the current directory for read. It isn't clear to me why the test can assume that the current directory is valid, and as such this looks to me like a bug in the test.

I don't, however, know why this is only failing on this device; but it may depend on which tests have been run previously, and hence on which directory is current.

An example log is https://logs.chromium.org/v/?s=chrome%2Fbb%2Finternal.client.clank%2Fasan-clang-phone%2F3700%2F%2B%2Frecipes%2Fsteps%2Fbase_unittests%2F0%2Fstdout

The critical lines are:
I 2439.705s run_tests_on_device(HT7580202717)  [==========] Running 1 test from 1 test case.
I 2439.706s run_tests_on_device(HT7580202717)  [----------] Global test environment set-up.
I 2439.706s run_tests_on_device(HT7580202717)  [----------] 1 test from DirReaderPosixUnittest
I 2439.706s run_tests_on_device(HT7580202717)  [ RUN      ] DirReaderPosixUnittest.Read
I 2439.706s run_tests_on_device(HT7580202717)  [FATAL:dir_reader_posix_unittest.cc(36)] Check failed: prev_wd >= 0 (-1 vs. 0)
I
Assigning to brettw@ who seems to be the last person to have changed this code (even if it was 6 years ago). Please re-assign to current owner.
There is a CL to fix this awaiting review: https://chromium-review.googlesource.com/c/chromium/src/+/881042
Cc: -aberent@google.com brettw@chromium.org
Labels: OS-Android Type-Bug
Owner: aber...@chromium.org
Did something change in the bot config? Looking at the older bot logs, base_unittests used to pass. It's not just failing on one device. The test first failed on HT7580202898 and then it failed the retry on HT7580202717.

Do we want to first land a logging CL, to figure out:
- What error open() failed with, using PLOG(ERROR).
- What directory "." corresponds to, using base::ReadSymbolicLink().

Once we have more information on what's going on, maybe we can decide what's the correct course of action.

BTW, I'm throwing out my theory that some our test did a chdir() and deleted the dir afterwards. There aren't that many chdir() callers in base.
Happy to create a logging CL to find out more,however, even if we find the immediate cause, is it a good idea for the test to assume that the current directory is valid, or to try to maintain it? As shown by the try jobs on https://chromium-review.googlesource.com/c/chromium/src/+/881042, removing this doesn't break any other tests. This would seem to suggest that there is no need to reset the current directory at the end of the test.

I have no idea if the bot config changed, and yes, I know it used to work. My theory was that some failure in some other test is leaving the phone with a strange current directory. I am not sure that the test harness ever resets the current directory, so this might not even be a base test.


Project Member

Comment 6 by bugdroid1@chromium.org, Jan 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9ae1c0928e08b0ba25a640c5ace6fa2f0bff9201

commit 9ae1c0928e08b0ba25a640c5ace6fa2f0bff9201
Author: Anthony Berent <aberent@chromium.org>
Date: Wed Jan 24 18:07:31 2018

Add logging to DirReaderPosixUnittest

To understand why the test sometimes can't read the current directory.

BUG= 804348 

Change-Id: If928c4d0982db04e8bfaa2d0fb22d606da1687f8
Reviewed-on: https://chromium-review.googlesource.com/883341
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531606}
[modify] https://crrev.com/9ae1c0928e08b0ba25a640c5ace6fa2f0bff9201/base/files/dir_reader_posix_unittest.cc

As a Linux user, at the command prompt, I generally expect $PWD to exist, and running ls should succeed. IMO it's fair to extend that expectation to tests.

One of my other guesses is Android has some sandboxing that's affecting this test. The bot hasn't gotten to r531606 yet, so gotta wait...
Seems to be a permission issue, although I have no idea why the test would not have permission to read the 
root directory:

C 14739.431s Main  [ERROR:dir_reader_posix_unittest.cc(37)] Current directory: /
C 14739.431s Main  [ERROR:dir_reader_posix_unittest.cc(41)] opening . failed with : Permission denied (13)
C 14739.431s Main  [FATAL:dir_reader_posix_unittest.cc(42)] Check failed: prev_wd >= 0 (-1 vs. 0)

The bot is beyond r532274 and still failing on this test.

Log messages say that the current directory is /, and the open is failing with permission denied. Probably the right fix is to get the test harness to start in a readable directory, but I don't know about enough about the test harness to make those changes.

For now, as the problem seems to be diagnosed, I'm just disabling this test on ASAN so we can get the bot green. Below is the full failure log for future reference. The bot is asan-clang-phone and the suite is base_unittests.

I  154.178s run_tests_on_device(HT7580202717)  [ RUN      ] DirReaderPosixUnittest.Read
I  154.178s run_tests_on_device(HT7580202717)  [ERROR:dir_reader_posix_unittest.cc(37)] Current directory: /
I  154.178s run_tests_on_device(HT7580202717)  [ERROR:dir_reader_posix_unittest.cc(41)] opening . failed with : Permission denied (13)
I  154.178s run_tests_on_device(HT7580202717)  [FATAL:dir_reader_posix_unittest.cc(42)] Check failed: prev_wd >= 0 (-1 vs. 0)
I  154.178s run_tests_on_device(HT7580202717)  #00 0xb2c96575 /data/app/org.chromium.native_test-2/lib/arm/libbase.cr.so+0x0020a575
I  154.178s run_tests_on_device(HT7580202717)  #01 0xb1b23355 /data/app/org.chromium.native_test-2/lib/arm/lib_base_unittests__library.cr.so+0x00942355
I  154.178s run_tests_on_device(HT7580202717)  #02 0xb26e77b7 /data/app/org.chromium.native_test-2/lib/arm/lib_base_unittests__library.cr.so+0x015067b7
I  154.178s run_tests_on_device(HT7580202717)  #03 0xb26e8449 /data/app/org.chromium.native_test-2/lib/arm/lib_base_unittests__library.cr.so+0x01507449
I  154.178s run_tests_on_device(HT7580202717)  #04 0xb26e8fad /data/app/org.chromium.native_test-2/lib/arm/lib_base_unittests__library.cr.so+0x01507fad
I  154.178s run_tests_on_device(HT7580202717)  #05 0xb26f2307 /data/app/org.chromium.native_test-2/lib/arm/lib_base_unittests__library.cr.so+0x01511307
I  154.178s run_tests_on_device(HT7580202717)  #06 0xb26f1d13 /data/app/org.chromium.native_test-2/lib/arm/lib_base_unittests__library.cr.so+0x01510d13
I  154.178s run_tests_on_device(HT7580202717)  #07 0xb273a165 /data/app/org.chromium.native_test-2/lib/arm/lib_base_unittests__library.cr.so+0x01559165
I  154.178s run_tests_on_device(HT7580202717)  #08 0xb2709f3f /data/app/org.chromium.native_test-2/lib/arm/lib_base_unittests__library.cr.so+0x01528f3f
I  154.178s run_tests_on_device(HT7580202717)  #09 0xb2766fb3 /data/app/org.chromium.native_test-2/lib/arm/lib_base_unittests__library.cr.so+0x01585fb3
I  154.178s run_tests_on_device(HT7580202717)  #10 0xb2709d6f /data/app/org.chromium.native_test-2/lib/arm/lib_base_unittests__library.cr.so+0x01528d6f
I  154.178s run_tests_on_device(HT7580202717)  #11 0xb2708ca1 /data/app/org.chromium.native_test-2/lib/arm/lib_base_unittests__library.cr.so+0x01527ca1
I  154.178s run_tests_on_device(HT7580202717)  
I  154.178s run_tests_on_device(HT7580202717)  [ERROR:test_suite.cc(298)] Currently running: DirReaderPosixUnittest.Read
I  154.179s run_tests_on_device(HT7580202717)  [host]> /b/build/slave/asan-clang-phone/build/src/build/android/pylib/symbols/../../../../third_party/android_platform/development/scripts/stack --arch arm64 --output-directory /b/build/slave/asan-clang-phone/build/src/out/Debug --more-info /tmp/tmpzbC10v
I  154.325s run_tests_on_device(HT7580202717)  Finished running tests on this device.

Project Member

Comment 10 by bugdroid1@chromium.org, Jan 29 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d94a230684004dcad20d2892c68ecaf4f00a69bf

commit d94a230684004dcad20d2892c68ecaf4f00a69bf
Author: Matthew Cary <mattcary@chromium.org>
Date: Mon Jan 29 11:21:30 2018

Disable base::DirReaderPosixUnittest::Read on clang ASAN.

The test harness has working directory /, which is not readable in
this test.

Bug:  804348 
TBR: aberent@chromium.org,brettw@chromium.org,thestig@chromium.org
Change-Id: I31f02bef8aa61f62e2e07c94279e875d8b6d9570
Reviewed-on: https://chromium-review.googlesource.com/891158
Reviewed-by: Matthew Cary <mattcary@chromium.org>
Commit-Queue: Matthew Cary <mattcary@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532391}
[modify] https://crrev.com/d94a230684004dcad20d2892c68ecaf4f00a69bf/base/files/dir_reader_posix_unittest.cc

Yes, newer Android OS versions have / locked down for some reason. e.g. bug 787149.

I'm wondering why the current working dir is /.
Why would of it be anything else? There are no explicit home directories on
android, so everything starts with / as its current directory.
Cc: jbudorick@chromium.org
+jbudorick for suggestions.
It seems to me that if the intention of the this part of the test to test that the current directory is always readable, then as we have discovered, this may not be true even for the default current directory, at least on Android. 

If the intention is to simply restore the current directory at the end of the test, then this could be done with getcwd() and chdir() (in place of fchdir()).
Project Member

Comment 15 by bugdroid1@chromium.org, Jan 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a6ca7975ea4dab8057535cb54a42d7c53d17069d

commit a6ca7975ea4dab8057535cb54a42d7c53d17069d
Author: Matthew Cary <mattcary@chromium.org>
Date: Tue Jan 30 09:59:23 2018

Only disable base::DirReaderPosixUnittest::Read on clang ASAN

The previous change which disabled it for all ASAN was overeager,
the bug only appears on clang.

Bug:  804348 
TBR: thestig@chromium.org,brettw@chromium.org,aberent@chromium.org
Change-Id: I8ae959bce00feb1f77b23dda979ed129bd88b003
Reviewed-on: https://chromium-review.googlesource.com/892440
Commit-Queue: Matthew Cary <mattcary@chromium.org>
Reviewed-by: Matthew Cary <mattcary@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532826}
[modify] https://crrev.com/a6ca7975ea4dab8057535cb54a42d7c53d17069d/base/files/dir_reader_posix_unittest.cc

aberent: Do you have a phone you can use to repro this test failure? Will the chdir() actually succeed?
Yes and yes. I have just tested this on a Nexus 6 running Android O. The original test fails, but chdir works fine. CL follows.
Blocking: 784331
Project Member

Comment 19 by bugdroid1@chromium.org, Feb 5 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f7622c74d2c03964892cea6e85ebaae0ebc3f15f

commit f7622c74d2c03964892cea6e85ebaae0ebc3f15f
Author: Anthony Berent <aberent@chromium.org>
Date: Mon Feb 05 11:06:53 2018

Avoid trying to read the current directory

Get the current directory with getcwd() and restore it with
chdir() (rather than opening it and using fchdir()). This
avoids trying to open the current directory, which,
on Android, may be the protected root directory.

BUG= 804348 

Change-Id: Ie9b5a5b0458de72c102cc9a5ab096eca807b55a2
Reviewed-on: https://chromium-review.googlesource.com/895642
Commit-Queue: Anthony Berent <aberent@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534362}
[modify] https://crrev.com/f7622c74d2c03964892cea6e85ebaae0ebc3f15f/base/files/dir_reader_posix_unittest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment