New issue
Advanced search Search tips

Issue 858899 link

Starred by 4 users

Issue metadata

Status: Verified
Owner:
Closed: Jul 11
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Media files from MTP device are not displayed

Project Member Reported by sdantul...@chromium.org, Jun 29 2018

Issue description

ChromeOS  10718.34.0, 68.0.3440.40  beta-channel  eve, kevin

MTP Device: Iphone with iOS version 11.4
 
What steps will reproduce the problem?
1. Connect iphone to chromebook
2. Accept permission dialog that pops up on iphone
3. Open Files app and click on 'Apple iphone' section
4. Open DCIM folder to view media folders
5. Click on any of the folders ('xxxAPPLE') to view media files

What is the expected result?
Media files should be displayed

What happens instead?
Media files are not displayed at all even after leaving the device connected for a long time (> 15mins). The progress bar is always in loading state. 

Feedback report from Eve: https://listnr.corp.google.com/report/85523519627

Feedback report from Kevin: https://listnr.corp.google.com/report/85523784106

Issue not observed on M67 stable build (10575.58.0, 67.0.3396.99	kevin) and M69 dev build (10825.0.0, 69.0.3475.0 eve, minnie). Media files were displayed in about 5 minutes on both the devices
 
Description: Show this description
Labels: CrOSFilesFeature-ExternalMedia
Owner: amistry@chromium.org
Status: Assigned (was: Untriaged)
I've managed to repro this on kevin and caroline on 68, but not on 69. I have an iPhone 8 running 11.4.
Cc: thestig@chromium.org
+cc thestig, who's been making changes to MTP code. I've narrowed the fix to between 69.0.3452 and 69.0.3455.1

I'm currently bisecting CLs to find the fix.
Was fixed in:
https://chromium.googlesource.com/chromium/src/+/44663e8f02729bf01d296ce86087f87168420713

I'll see if it can be merged into 68, or if the original breakage needs to be reverted instead (in the release branch).
A clean merge to 68 is possible if both crrev.com/3d7f15c742e7aff293b9b2065837bf98cb3a042b and crrev.com/44663e8f02729bf01d296ce86087f87168420713 are cherry-picked.

@thestig, WDYT?
Cc: donna...@intel.com
The cherry-picks in comment 6 sound ok, but what would be the revert option? Can you check where this broke?
Breakage was caused by crrev.com/aaa4d5aa673b0e9f9973bc38386f95f59e2fb3ec

I tried reverting it in the 68 branch, but after having to also revert 4 follow-ups, I couldn't get a build. I think the best option is to merge the two I mentioned in comment 6. There are too many changes to go backwards.
Labels: Merge-Request-68
Requesting a merge of crrev.com/3d7f15c742e7aff293b9b2065837bf98cb3a042b and crrev.com/44663e8f02729bf01d296ce86087f87168420713

Servicification of MTP has broken it under M68. There are too many follow-up changes to do a graceful revert in the 68 branch, but the two requested patches merge cleanly and fix the issue.
Project Member

Comment 10 by sheriffbot@chromium.org, Jul 9

Labels: -Merge-Request-68 Hotlist-Merge-Review Merge-Review-68
This bug requires manual review: M68 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
SGTM.
Labels: -Merge-Review-68 Merge-Approved-68
Thanks for the details on alternatives and testing. Bernie is out, I will approve this for merge in M68.
Project Member

Comment 13 by bugdroid1@chromium.org, Jul 10

Labels: -merge-approved-68 merge-merged-3440
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/99180f2481b702ae4928a92b1dd39231996ede2d

commit 99180f2481b702ae4928a92b1dd39231996ede2d
Author: Lei Zhang <thestig@chromium.org>
Date: Tue Jul 10 00:24:16 2018

[M68] Align GetFileInfo() between MtpManager and MtpDaemonClient.

TBR=thestig@chromium.org
BUG= 858899 

Align GetFileInfo() between MtpManager and MtpDaemonClient.

MediaTransferProtocolDaemonClient currently provides a low-level
GetFileInfo() method, and MtpManager does the translation to the
high-level GetFileInfo() method that MTPDeviceTaskHelper understands.

Change MtpManager's GetFileInfo() from the high-level method to the
low-level method, so it acts more like a pass-through. Make
MTPDeviceTaskHelper handle the translation.

BUG= 769630 

Change-Id: I21e20b71d333a1ee7173071f04aaf2e37dd61844
Reviewed-on: https://chromium-review.googlesource.com/1080212
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#565413}(cherry picked from commit 3d7f15c742e7aff293b9b2065837bf98cb3a042b)
Reviewed-on: https://chromium-review.googlesource.com/1130715
Reviewed-by: Anand Mistry <amistry@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#624}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/99180f2481b702ae4928a92b1dd39231996ede2d/chrome/browser/media_galleries/chromeos/mtp_device_task_helper.cc
[modify] https://crrev.com/99180f2481b702ae4928a92b1dd39231996ede2d/chrome/browser/media_galleries/chromeos/mtp_device_task_helper.h
[modify] https://crrev.com/99180f2481b702ae4928a92b1dd39231996ede2d/components/storage_monitor/test_media_transfer_protocol_manager_chromeos.cc
[modify] https://crrev.com/99180f2481b702ae4928a92b1dd39231996ede2d/components/storage_monitor/test_media_transfer_protocol_manager_chromeos.h
[modify] https://crrev.com/99180f2481b702ae4928a92b1dd39231996ede2d/services/device/media_transfer_protocol/media_transfer_protocol_manager.cc
[modify] https://crrev.com/99180f2481b702ae4928a92b1dd39231996ede2d/services/device/media_transfer_protocol/media_transfer_protocol_manager.h
[modify] https://crrev.com/99180f2481b702ae4928a92b1dd39231996ede2d/services/device/media_transfer_protocol/mtp_device_manager.cc
[modify] https://crrev.com/99180f2481b702ae4928a92b1dd39231996ede2d/services/device/media_transfer_protocol/mtp_device_manager.h
[modify] https://crrev.com/99180f2481b702ae4928a92b1dd39231996ede2d/services/device/public/mojom/mtp_manager.mojom

Project Member

Comment 14 by bugdroid1@chromium.org, Jul 10

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

commit 1530ae01b4687f98b62ee816762ac1042ea2b270
Author: Lei Zhang <thestig@chromium.org>
Date: Tue Jul 10 00:27:57 2018

[M68] Replace MtpManager's ReadDirectory() with ReadDirectoryEntryIds().

TBR=thestig@chromium.org
BUG= 858899 

Replace MtpManager's ReadDirectory() with ReadDirectoryEntryIds().

MediaTransferProtocolDaemonClient currently provides a low-level
ReadDirectoryEntryIds() method, and MtpManager does the translation to
the high-level ReadDirectory() method that MTPDeviceTaskHelper
understands.

Change MtpManager's ReadDirectory() method to ReadDirectoryEntryIds(),
so it acts more like a pass-through. Make MTPDeviceTaskHelper handle the
translation.

BUG= 769630 

Change-Id: I5154bd4beac0b6ab15579f166cdf8b8add6a7dac
Reviewed-on: https://chromium-review.googlesource.com/1080214
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#565441}(cherry picked from commit 44663e8f02729bf01d296ce86087f87168420713)
Reviewed-on: https://chromium-review.googlesource.com/1130735
Reviewed-by: Anand Mistry <amistry@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#625}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/1530ae01b4687f98b62ee816762ac1042ea2b270/chrome/browser/media_galleries/chromeos/mtp_device_task_helper.cc
[modify] https://crrev.com/1530ae01b4687f98b62ee816762ac1042ea2b270/chrome/browser/media_galleries/chromeos/mtp_device_task_helper.h
[modify] https://crrev.com/1530ae01b4687f98b62ee816762ac1042ea2b270/components/storage_monitor/test_media_transfer_protocol_manager_chromeos.cc
[modify] https://crrev.com/1530ae01b4687f98b62ee816762ac1042ea2b270/components/storage_monitor/test_media_transfer_protocol_manager_chromeos.h
[modify] https://crrev.com/1530ae01b4687f98b62ee816762ac1042ea2b270/services/device/media_transfer_protocol/media_transfer_protocol_manager.cc
[modify] https://crrev.com/1530ae01b4687f98b62ee816762ac1042ea2b270/services/device/media_transfer_protocol/media_transfer_protocol_manager.h
[modify] https://crrev.com/1530ae01b4687f98b62ee816762ac1042ea2b270/services/device/media_transfer_protocol/mtp_device_manager.cc
[modify] https://crrev.com/1530ae01b4687f98b62ee816762ac1042ea2b270/services/device/media_transfer_protocol/mtp_device_manager.h
[modify] https://crrev.com/1530ae01b4687f98b62ee816762ac1042ea2b270/services/device/public/mojom/mtp_manager.mojom

Status: Fixed (was: Assigned)
I've flashed 68.0.3440.57 and checked that I can now access the photos on my iPhone.
Status: Verified (was: Fixed)
Verified on ChromeOS 10718.50.0, 68.0.3440.59 beta-channel eve, minnie

Sign in to add a comment