New issue
Advanced search Search tips

Issue 809194 link

Starred by 1 user
Status: Assigned
Owner:
rdevlin....@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Remove Non-Deterministic ExtensionFunction WebContents, Browser, and WindowController accessors

Project Member Reported by rdevlin....@chromium.org, Feb 5 2018 
There are a number of different methods on ExtensionFunction and friends (ChromeAsyncExtensionFunction, ChromeExtensionFunctionDetails) that return a WebContents, Browser, WindowController, or similar object. Unfortunately, most of these (with the exception of render_frame_host() and GetSenderWebContents()) are very non-deterministic, returning any number of possible values (including some that might only be vaguely related to the extension).

We should remove these, and update callers to use GetSenderWebContents() where possible, or to otherwise explicitly get a different value (e.g., BrowserList::GetLastActive() to get the last active browser).

I'll own this for now, but patches welcome.
Project Member

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

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

commit 556f592eb9e1e47f4a54148d1e6ce6caa7ebbf21
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Mon Feb 05 23:35:07 2018

[Extension Functions] Remove GetExtensionWindowController

Remove ChromeAsyncExtensionFunction::GetExtensionWindowController() and
ChromeExtensionFunctionDetails::GetExtensionWindowController(). Each of
these were never used. Additionally, the behavior is very non-
deterministic, and could return any one of a number of different
windows. Instead, ExtensionFunctions should fetch the appropriate window
for the call (typically, through GetSenderWebContents()), rather than by
relying on getting any window the extension can access.

Bug: 809194

Change-Id: I00fc417cfbe4c9161f2475f50ab77fe843677faa
Reviewed-on: https://chromium-review.googlesource.com/902198
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534535}
[modify] https://crrev.com/556f592eb9e1e47f4a54148d1e6ce6caa7ebbf21/chrome/browser/extensions/chrome_extension_function.cc
[modify] https://crrev.com/556f592eb9e1e47f4a54148d1e6ce6caa7ebbf21/chrome/browser/extensions/chrome_extension_function.h
[modify] https://crrev.com/556f592eb9e1e47f4a54148d1e6ce6caa7ebbf21/chrome/browser/extensions/chrome_extension_function_details.cc
[modify] https://crrev.com/556f592eb9e1e47f4a54148d1e6ce6caa7ebbf21/chrome/browser/extensions/chrome_extension_function_details.h
Project Member

Comment 2 by bugdroid1@chromium.org, Feb 8 2018 

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

commit 4ef78a87a98e35103f8f8be2f4501acc485174e9
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Thu Feb 08 00:34:42 2018

[Extension Functions] Move GetCurrentBrowser to details only

Remove ChromeAsyncExtensionFunction::GetCurrentBrowser() and update
callers to always use
ChromeExtensionFunctionDetails::GetCurrentBrowser(). This is
functionally the same as what was happening before, but now we don't
have to duplicate the function (and very long comment) in both
chrome_extension_function and chrome_extension_function_details.

No behavior change.

Bug: 809194
Bug: 634140

Change-Id: I4379fc0c2d22800ceed7c6355828719ec81cde1b
Reviewed-on: https://chromium-review.googlesource.com/902086
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535214}
[modify] https://crrev.com/4ef78a87a98e35103f8f8be2f4501acc485174e9/chrome/browser/chromeos/extensions/file_manager/file_browser_handler_api.cc
[modify] https://crrev.com/4ef78a87a98e35103f8f8be2f4501acc485174e9/chrome/browser/extensions/api/cookies/cookies_api.cc
[modify] https://crrev.com/4ef78a87a98e35103f8f8be2f4501acc485174e9/chrome/browser/extensions/api/tabs/tabs_api.cc
[modify] https://crrev.com/4ef78a87a98e35103f8f8be2f4501acc485174e9/chrome/browser/extensions/chrome_extension_function.cc
[modify] https://crrev.com/4ef78a87a98e35103f8f8be2f4501acc485174e9/chrome/browser/extensions/chrome_extension_function.h
Project Member

Comment 3 by bugdroid1@chromium.org, May 8 2018 

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

commit eb0fa56ceccbc4d9b483d4d9045ae8c619a62d49
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Tue May 08 14:02:46 2018

[Extensions] Remove GetCurrentBrowser() usage from cookies API

GetCurrentBrowser() is pretty non-deterministic (returning various
different browser objects depending on different circumstances). It
looks like the cookies API only needs it to get at the profile
associated with the extension function. Update this to just use
GetProfile() instead.

Bug: 809194
Change-Id: I745d17313ca77da43f164f06d55eb3ddd1840901
Reviewed-on: https://chromium-review.googlesource.com/1048392
Reviewed-by: Mike West <mkwst@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556785}
[modify] https://crrev.com/eb0fa56ceccbc4d9b483d4d9045ae8c619a62d49/chrome/browser/extensions/api/cookies/cookies_api.cc

Comment 4 by benhenry@chromium.org, Aug 2

Status: Assigned (was: Available)

Comment 5 by woxxom@gmail.com, Aug 9 

r556785 apparently caused  bug 872156 , PTAL.

Comment 6 by rdevlin....@chromium.org, Aug 13 

@5 thanks, woxxom@!  Replied on that bug.
►

Sign in to add a comment