New issue
Advanced search Search tips

Issue 844654 link

Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

File manager from extensions and apps use unpredictable web contents

Project Member Reported by rdevlin....@chromium.org, May 18 2018

Issue description

The SelectFileDialogExtension class is used to select a file from an extension or app.  The dialog is associated with a unique id (SelectFileDialogExtension::RoutingID) [1], which is based on a WebContents [2].  This routing id is used to programmatically interact with the dialog through the fileManagerPrivate API (e.g., fileManagerPrivate.cancelDialog) [3].

The dialog itself is triggered through the SelectFileDialog::SelectFile flow from the fileBrowserHandlerInternal.selectFile extension API function. [4]  The routing ID that is associated with this dialog is determined by calling FindRuntimeContext() for the gfx::NativeWindow passed into SelectFileDialogExtension::SelectFileImpl() [5].  FindRuntimeContext() looks at the gfx::NativeWindow either the associated browser or the associated app window.  If it can't find either, it finds some other web contents to use (such as the last active browser's active tab).  The gfx::NativeWindow passed into this is from the browser retrieved by ChromeExtensionFunctionDetails::GetCurrentBrowser() for the fileBrowserHandlerInternal.selectFile API function [6].  This, in turn, searches a number of ambiguous web contents and browsers, also occasionally defaulting to the active browser (or even an inactive one. [7]

In order to retrieve this same web contents, the code in fileManagerPrivate uses the deprecated GetAssociatedWebContents() function. [8]  This function, too, is fairly non-deterministic in the web contents it returns.

So, we end up having a flow like the following:
1. fileBrowserHandlerInternal.selectFile calls SelectFileDialog::SelectFile with a window of a browser retrieved using ChromeExtensionFunctionDetails::GetCurrentBrowser().
2. The SelectFileDialogExtension::SelectFileImpl method associates this dialog with a WebContents retrieved from FindRuntimeContext().
3. fileManagerPrivate.cancelDialog retrieves the ID for a WebContents with ChromeExtensionFunctionDetails::GetAssociatedWebContents().

All three of these stages - using GetCurrentBrowser(), FindRuntimeContext(), and GetAssociatedWebContents() return unpredictable values, but it's required that they match.  This makes it fragile, and difficult to understand.  Instead, these should find a way of using a deterministic value associated with the caller.

I attempted to transition at least the third step to using GetSenderWebContents() (which is deterministic and predictable) in revision 1c397d3ddc70d0a4ffacb9bec37d447d8dd8ec9f, but this had to be reverted in eaffdd8c5fb9f81bc7956de38389aad8048bcfc8 because of crashes (https://crbug.com/842581).  At this point, I think this will take someone looking more holistically into this flow.

Since the fileManagerPrivate API is the last caller of GetAssociatedWebContentsDeprecated(), I'm planning on just inlining the method there to prevent any further use.

[1] https://chromium.googlesource.com/chromium/src/+/82a5f004e0e52897a6e8bac10490a14fc2845625/chrome/browser/ui/views/select_file_dialog_extension.h#38
[2] https://chromium.googlesource.com/chromium/src/+/82a5f004e0e52897a6e8bac10490a14fc2845625/chrome/browser/ui/views/select_file_dialog_extension.cc#373
[3] https://chromium.googlesource.com/chromium/src/+/82a5f004e0e52897a6e8bac10490a14fc2845625/chrome/browser/chromeos/extensions/file_manager/private_api_dialog.cc#32
[4] https://chromium.googlesource.com/chromium/src/+/82a5f004e0e52897a6e8bac10490a14fc2845625/chrome/browser/chromeos/extensions/file_manager/file_browser_handler_api.cc#218
[5] https://chromium.googlesource.com/chromium/src/+/82a5f004e0e52897a6e8bac10490a14fc2845625/chrome/browser/ui/views/select_file_dialog_extension.cc#120
[6] https://chromium.googlesource.com/chromium/src/+/01ce72edb648ed9295f00c97afbe5eda1619c422/chrome/browser/chromeos/extensions/file_manager/file_browser_handler_api.cc#311
[7] https://chromium.googlesource.com/chromium/src/+/01ce72edb648ed9295f00c97afbe5eda1619c422/chrome/browser/extensions/chrome_extension_function_details.cc#35
[8] https://chromium.googlesource.com/chromium/src/+/82a5f004e0e52897a6e8bac10490a14fc2845625/chrome/browser/chromeos/extensions/file_manager/private_api_dialog.cc#25
 
Project Member

Comment 1 by bugdroid1@chromium.org, May 19 2018

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

commit 9c82aae1cd3fe30cb3daf3d3f3badbe1b2a04cc9
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Sat May 19 21:46:00 2018

[Extensions] (Mostly) remove GetAssociatedWebContentsDeprecated()

All callers except the fileManagerPrivate API have been updated to not
rely on GetAssociatedWebContentsDeprecated(). The fileManagerPrivate
issue is being tracked separately. Remove the function
GetAssociatedWebContentsDeprecated() and move it to be directly in the
fileManagerPrivate API to avoid having usage spread.

Bug:  461394 , 844654
Change-Id: I69adc7f3534d4908497bf73d3fcab0f7443450e7
Reviewed-on: https://chromium-review.googlesource.com/1066733
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560171}
[modify] https://crrev.com/9c82aae1cd3fe30cb3daf3d3f3badbe1b2a04cc9/chrome/browser/chromeos/extensions/file_manager/private_api_dialog.cc
[modify] https://crrev.com/9c82aae1cd3fe30cb3daf3d3f3badbe1b2a04cc9/chrome/browser/extensions/chrome_extension_function.cc
[modify] https://crrev.com/9c82aae1cd3fe30cb3daf3d3f3badbe1b2a04cc9/chrome/browser/extensions/chrome_extension_function.h
[modify] https://crrev.com/9c82aae1cd3fe30cb3daf3d3f3badbe1b2a04cc9/chrome/browser/extensions/chrome_extension_function_details.cc
[modify] https://crrev.com/9c82aae1cd3fe30cb3daf3d3f3badbe1b2a04cc9/chrome/browser/extensions/chrome_extension_function_details.h
[modify] https://crrev.com/9c82aae1cd3fe30cb3daf3d3f3badbe1b2a04cc9/extensions/browser/extension_function.cc
[modify] https://crrev.com/9c82aae1cd3fe30cb3daf3d3f3badbe1b2a04cc9/extensions/browser/extension_function.h

Comment 2 by sashab@google.com, May 24 2018

Labels: -Pri-2 CrOSFilesCategory-CodeHealth Pri-3
Status: Available (was: Untriaged)
Is there a timeline for this? What kind of issues does this cause?
The biggest issue is around understanding what the code is currently doing, since it makes it very hard to know without knowing the *exact* state of the browser at that time.  This also inherently means it's very fragile, and could break without much warning.  However, it seems to have worked without much incident for the past couple years, so it's mostly just a code health problem.

Certainly no timeline on my end - I don't have enough knowledge of the file manager to pursue it.

Sign in to add a comment