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

RESOLVED FIXED in Firefox 63

Status

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: bc, Assigned: bc)

Tracking

Trunk
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Attachment #9003667 - Flags: review?(jmaher)
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
Attachment #9003667 - Flags: review?(jmaher) → review-
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.
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.
Attachment #9003887 - Flags: review?(jmaher)
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.
Attachment #9003667 - Attachment is obsolete: true
Attachment #9003887 - Flags: review?(jmaher) → review+
Pushed by bclary@mozilla.com:
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.
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.
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.
https://hg.mozilla.org/mozilla-central/rev/b70bf5d57074
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Blocks: 1487130
You need to log in before you can comment on or make changes to this bug.