1485855 - [mozdevice] adb.py Do not attempt to chmod external storage as it will not affect permissions and will fail on unrooted devices.

Status: RESOLVED FIXED

(Testing :: Mozbase, enhancement)

Description

[Bob Clary [:bc] (inactive)]

chmod has no effect on external storage on Android (e.g sdcards) but we still attempt to call chmod on it. It seems that when called via su, this has no affect but does not cause an error either.

However, on some unrooted devices, attempting to chmod content on the external storage will fail with a permissions error.

Since it has no effect even when the command "succeeds", we can just pretend we did it and not waste time nor suffer errors.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: do-not-chmod-external-storage.patch

Comment 2

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

Comment on attachment 9003667 [details] [diff] [review]
do-not-chmod-external-storage.patch

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

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +1544,5 @@
>          path = posixpath.normpath(path.strip())
>          self._logger.debug('chmod: path=%s, recursive=%s, mask=%s, root=%s' %
>                             (path, recursive, mask, root))
> +        if (path.startswith('/sdcard') or path.startswith('/storage/sdcard') or
> +            path.startswith('/mnt/sdcard')):

could we simplify this by saying:
if '/sdcard' in path:
   self._logger...
   return

pease add a comment here so we know why we are skipping this

Comment 3

[Bob Clary [:bc] (inactive)]

I thought about that and I'm not happy with just looking for /sdcard anywhere in the path. I can imagine cases where a path not on the external storage would contain an sdcard* subdirectory which we would then skip. As I was going to sleep I was wondering if I can figure out if the path is on external storage or not regardless of the path's literal value. I'm pretty sure the android java api has it but I'm not sure about from the shell.

Re comment: sure.

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: do-not-chmod-external-storage.patch

I couldn't figure out a means of getting the external storage locations via a command but Bogdan sent the environments for a number of different devices he has access to and I found that there were a number of possible paths for external storage. It seems to be a good approach to detect this at run time rather than hard code it. I prepopulated the list with /mnt/sdcard and /sdcard since we often see/use them but the environment variables should cover any edge cases we might see in the future. I've confirmed that mozregression continues to work with these changes.

Comment 5

[Bob Clary [:bc] (inactive)]

Comment on attachment 9003887 [details] [diff] [review]
do-not-chmod-external-storage.patch

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

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +1571,5 @@
>          # detected.
>          path = posixpath.normpath(path.strip())
>          self._logger.debug('chmod: path=%s, recursive=%s, mask=%s, root=%s' %
>                             (path, recursive, mask, root))
> +        if self.is_path_internal_storage(path):

I missed a timeout here. Will fix locally.

Comment 6

[Bob Clary [:bc] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a82794f96e5e95f7a899e866ca790f92040db472&filter-tier=1&filter-tier=2&filter-tier=3

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b70bf5d57074
[mozdevice] adb.py Do not attempt to chmod external storage as it will not affect permissions and will fail on unrooted devices, r=jmaher.

Comment 8

[Geoff Brown [:gbrown]]

Glad to see this. Sometimes this type of failed permission change log message becomes a red herring for bug investigations -- annoying.

I had a thought for making this more robust: Could we determine will-chmod-work-here? by examining the associated mount type and options? I suspect this idea is not practical (variety of mount types and applicable options, difficult to parse 'mount' output, difficult to associate mount points with chmod paths) -- just mentioning it.

Comment 9

[Bob Clary [:bc] (inactive)]

I looked into it for a very short period of time but like you mention it seemed like a can of worms. I had the insight to use the STORAGE environment variables after Bogdan sent me the output of set on a bunch of the devices he had available. I'll forware them to you.

Comment 10

[Andrei Ciure[:aciure]]

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