New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 706169 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 623210



Sign in to add a comment

DevTools extension iframes should be rendered in separate processes

Project Member Reported by alex...@chromium.org, Mar 28 2017

Issue description

Currently, devtools extension iframes are allowed to stay in the devtools process.  This exception was added to RenderFrameHostManager::IsRendererTransferNeededForNavigation as a quick fix for  issue 564216 .  It avoids the process swap for the extension's devtools_page as well as panels and sidebar panes that it creates (all of these are iframes).

As part of the discussion on https://codereview.chromium.org/2646683002/, we discovered that it's actually possible to kick devtools extensions out of process.  The devtools APIs are all written on top of postMessage, and there are no other synchronous scripting requirements of any kind.  According to caseq@, the main issue to resolve is changing how the devtools extension APIs are injected.  Currently, that relies on DevToolsHost::setInjectedScriptForOrigin [1], which only works within a single renderer. Instead, we'll need to inject the script APIs from the browser; dgozman@ suggests watching for the matching frames in DevToolsUIBindings::FrontendWebContentsObserver::ReadyToCommitNavigation and
injecting the API script from there.

This should further strengthen the security model for --isolate-extensions and remove one of the last cases where a chrome-extension:// URL isn't rendered in an extension process.  We'll try to work on this in Q2.

A few more notes from recent discussions:

- Doing this should allow us to remove the devtools scheme checks from RFHM::IsRendererTransferNeededForNavigation and DetermineSiteInstanceForURL.  Devtools extension process swaps would be handled just like all other process swaps.  In  issue 564216 , we were crashing on the devtools extension transfer path, but that's not happening anymore, probably because we're shortcutting transfers not to go through the OpenURL path anymore.

- It should be fine, and in fact desirable, to put the devtools_page, panels, and sidebar panes into the extension process along with the rest of the extension.  Nick and I discussed whether we might need to invent a new type of process with devtools extension API access but without other extension privileges, but there seems to be no fundamental reason for separating the privileges, and putting everything into the extension process would simplify lives of developers who currently have to use message passing between the devtools and non-devtools parts of their extension.  We should be careful to only grant script API access to devtools extension iframes within the devtools window.

- For cases like devtools -> devtools_page -> panel -> web iframe -> devtools extension iframe, there's the question of whether the bottom devtools extension iframe should get devtools extension APIs.  Devtools folks seem to be fine either way, davidsac@ mentioned that APIs aren't granted in such cases today, and we might as well keep it that way to minimize the privileges.

[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js?l=765
 

Comment 1 by nick@chromium.org, Mar 29 2017

Cc: devlin@chromium.org

Comment 2 by nasko@chromium.org, Mar 29 2017

Cc: -devlin@chromium.org rdevlin....@chromium.org
Additionally, we will need to fix the tests added in https://codereview.chromium.org/2646683002/ so that the expected process checks reflect the change.

We may also want to fix the "HttpIframeInDevToolsExtensionPanel" test to only contain a single web iframe like the other two "HttpIframeInDevToolsExtension*" tests, since those cases may be less relevant.  For the same reason, we may also want to get rid of the "NonDevToolsExtensionInDevToolsExtension","DevToolsExtensionInDevToolsExtension", and "DevToolsExtensionInItself" tests.

On the other hand, Alex makes a good point that these tests, although they cover cases of less concern for this new change, would still offer some additional test coverage and may be worth keeping.

Comment 4 by nick@chromium.org, Mar 29 2017

For the most part it seems the devtools API is implemented as polyfill, written terms of window.postMessage. But something needs to happen to run this code in the devtools pages (the arguments, like the inspected window, also need to be sent - hopefully we don't need to get them from the devtools process).

Currently the API -- injectedExtensionAPI() -- arrives via a content script in the devtools process, on all pages from the extension origin. Such an origin-based approach seems like it won't work with the oopif approach, since it would get injected into other non-devtools contexts for that extension, and since you'll potentially have multiple instances of devtools_page.html in the same extension process.

These concerns may lead to needing to make the devtools APIs work more like ordinary extension bindings.  Ideally we can find something close to that. Are there other extension APIs that are polyfill-like, or that work via script injection, analogous to how WebKit\Source\devtools\front_end\extensions\ExtensionAPI.js works? 
Hi Nick,

From what I have observed, today the API script is not injected in all devtools extension pages based on their origin.  For example, an iframe in a devtools extension to another page from the same extension does not seem to be granted devtools API priveleges.  Could I get a sanity check on this?  Please download the following extension, install it, and follow the instructions in the readme:
http://dudeguy409.github.io/Chrome_Devtools_Extension_Tests/Devtools_API_Fail_DT_Extension.tar.gz

Additionally, from our discussion the other day, it sounded like it would not really matter if all pages for a devtools extension were given devtools API access. 
Blocking: 623210
Project Member

Comment 8 by bugdroid1@chromium.org, Jun 13 2017

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

commit b1a9ccfec934b1cb7f726d12d21fdd53db18503a
Author: Pavel Feldman <pfeldman@chromium.org>
Date: Tue Jun 13 22:53:46 2017

DevTools: prepare to run devtools extensions out of process, check for devtools site in ::ShouldAllowOpenURL.
See https://chromium-review.googlesource.com/c/532314/4 for complete context.

Bug:  706169 
Change-Id: I3c51dbeac1882e0bc7b338497df8304494fdc729
Reviewed-on: https://chromium-review.googlesource.com/533936
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Pavel Feldman <pfeldman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#479190}
[modify] https://crrev.com/b1a9ccfec934b1cb7f726d12d21fdd53db18503a/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Jun 14 2017

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

commit ad5aa4eb912447773197750c96d6b7b5ad0897ba
Author: Pavel Feldman <pfeldman@chromium.org>
Date: Wed Jun 14 17:14:35 2017

DevTools: inject devtools extensions API via browser, not renderer - prepare to OOPIF extensions migration.

Bug:  706169 
Change-Id: I260a404499d31797a3e051a281b15eb5e12aa934

TBR=tsepez

Change-Id: I260a404499d31797a3e051a281b15eb5e12aa934
Reviewed-on: https://chromium-review.googlesource.com/533701
Commit-Queue: Pavel Feldman <pfeldman@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#479429}
[modify] https://crrev.com/ad5aa4eb912447773197750c96d6b7b5ad0897ba/chrome/browser/devtools/devtools_embedder_message_dispatcher.cc
[modify] https://crrev.com/ad5aa4eb912447773197750c96d6b7b5ad0897ba/chrome/browser/devtools/devtools_embedder_message_dispatcher.h
[modify] https://crrev.com/ad5aa4eb912447773197750c96d6b7b5ad0897ba/chrome/browser/devtools/devtools_ui_bindings.cc
[modify] https://crrev.com/ad5aa4eb912447773197750c96d6b7b5ad0897ba/chrome/browser/devtools/devtools_ui_bindings.h
[modify] https://crrev.com/ad5aa4eb912447773197750c96d6b7b5ad0897ba/content/browser/devtools/devtools_frontend_host_impl.cc
[modify] https://crrev.com/ad5aa4eb912447773197750c96d6b7b5ad0897ba/content/public/browser/devtools_frontend_host.h
[modify] https://crrev.com/ad5aa4eb912447773197750c96d6b7b5ad0897ba/content/renderer/devtools/devtools_agent.cc
[modify] https://crrev.com/ad5aa4eb912447773197750c96d6b7b5ad0897ba/content/renderer/devtools/devtools_agent.h
[modify] https://crrev.com/ad5aa4eb912447773197750c96d6b7b5ad0897ba/content/renderer/devtools/devtools_client.cc
[modify] https://crrev.com/ad5aa4eb912447773197750c96d6b7b5ad0897ba/content/renderer/devtools/devtools_client.h
[modify] https://crrev.com/ad5aa4eb912447773197750c96d6b7b5ad0897ba/content/shell/browser/shell_devtools_bindings.cc
[modify] https://crrev.com/ad5aa4eb912447773197750c96d6b7b5ad0897ba/content/shell/browser/shell_devtools_bindings.h

Project Member

Comment 10 by bugdroid1@chromium.org, Jun 14 2017

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

commit 43fdd031c0df9197cefcc33728474f13e02d4f3c
Author: Pavel Feldman <pfeldman@chromium.org>
Date: Wed Jun 14 21:02:52 2017

DevTools: move extensions tests into http/tests/inspector.

Bug:  706169 
Change-Id: I70073eb2607a7bab8a2299b0998e7a2255ed3b4c
Reviewed-on: https://chromium-review.googlesource.com/534965
Commit-Queue: Pavel Feldman <pfeldman@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#479503}
[modify] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/FlagExpectations/enable-network-service
[modify] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/SlowTests
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/audits-test.js
[modify] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions-network-test.js
[modify] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions-test.js
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-api-expected.txt
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-api.html
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-audits-api-expected.txt
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-audits-api.html
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-audits-content-script-expected.txt
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-audits-content-script.html
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-audits-expected.txt
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-audits-tests.js
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-audits.html
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-eval-content-script-expected.txt
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-eval-content-script.html
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-eval-expected.txt
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-eval.html
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-events-expected.txt
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-events.html
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-network-expected.txt
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-network.html
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-panel-expected.txt
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-panel.html
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-reload-expected.txt
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-reload.html
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-resources-expected.txt
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-resources.html
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-sidebar-expected.txt
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-sidebar.html
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-timeline-api-expected.txt
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/extensions-timeline-api.html
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/multiple-extensions-expected.txt
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/multiple-extensions.html
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/resources/abe.png
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/resources/audits-style1.css
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/resources/subframe.html
[rename] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/extensions/resources/test-script.js
[modify] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/http/tests/inspector/resources/extension-main.js
[modify] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/inspector/audits/audits-empty-stylesheet.html
[modify] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/inspector/audits/audits-panel-functional.html
[modify] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/inspector/audits/audits-panel-noimages-functional.html
[modify] https://crrev.com/43fdd031c0df9197cefcc33728474f13e02d4f3c/third_party/WebKit/LayoutTests/inspector/user-metrics.html

Project Member

Comment 11 by bugdroid1@chromium.org, Jun 15 2017

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

commit 28010fcf09d929d2598531c0d141bfe61e7ea8c7
Author: Pavel Feldman <pfeldman@chromium.org>
Date: Thu Jun 15 21:20:23 2017

DevTools: flip devtools extensions to load out of process.

Bug:  706169 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: I301b6b6b7d7711a44651d93cfed208e529c63dc8

TBR=for page_info_bubble_view_browsertest.cc

Change-Id: I301b6b6b7d7711a44651d93cfed208e529c63dc8
Reviewed-on: https://chromium-review.googlesource.com/535022
Commit-Queue: Pavel Feldman <pfeldman@chromium.org>
Reviewed-by: Nick Carter <nick@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#479834}
[modify] https://crrev.com/28010fcf09d929d2598531c0d141bfe61e7ea8c7/chrome/browser/devtools/devtools_sanity_browsertest.cc
[modify] https://crrev.com/28010fcf09d929d2598531c0d141bfe61e7ea8c7/chrome/browser/ui/views/page_info/page_info_bubble_view_browsertest.cc
[modify] https://crrev.com/28010fcf09d929d2598531c0d141bfe61e7ea8c7/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/28010fcf09d929d2598531c0d141bfe61e7ea8c7/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/28010fcf09d929d2598531c0d141bfe61e7ea8c7/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process
[modify] https://crrev.com/28010fcf09d929d2598531c0d141bfe61e7ea8c7/third_party/WebKit/LayoutTests/http/tests/inspector/extensions-test.js
[delete] https://crrev.com/e464732fb0179fa3514239419c804547a0f56b8d/third_party/WebKit/LayoutTests/http/tests/inspector/injected-script-for-origin-expected.txt
[delete] https://crrev.com/e464732fb0179fa3514239419c804547a0f56b8d/third_party/WebKit/LayoutTests/http/tests/inspector/injected-script-for-origin.html
[delete] https://crrev.com/e464732fb0179fa3514239419c804547a0f56b8d/third_party/WebKit/LayoutTests/http/tests/inspector/resources/injected-script-for-origin-frame.html
[modify] https://crrev.com/28010fcf09d929d2598531c0d141bfe61e7ea8c7/third_party/WebKit/Source/core/inspector/DevToolsHost.cpp
[modify] https://crrev.com/28010fcf09d929d2598531c0d141bfe61e7ea8c7/third_party/WebKit/Source/core/inspector/DevToolsHost.h
[modify] https://crrev.com/28010fcf09d929d2598531c0d141bfe61e7ea8c7/third_party/WebKit/Source/core/inspector/DevToolsHost.idl
[modify] https://crrev.com/28010fcf09d929d2598531c0d141bfe61e7ea8c7/third_party/WebKit/Source/core/inspector/InspectorFrontendClient.h
[modify] https://crrev.com/28010fcf09d929d2598531c0d141bfe61e7ea8c7/third_party/WebKit/Source/devtools/front_end/devtools_compatibility.js
[modify] https://crrev.com/28010fcf09d929d2598531c0d141bfe61e7ea8c7/third_party/WebKit/Source/devtools/front_end/externs.js
[modify] https://crrev.com/28010fcf09d929d2598531c0d141bfe61e7ea8c7/third_party/WebKit/Source/web/WebDevToolsFrontendImpl.cpp
[modify] https://crrev.com/28010fcf09d929d2598531c0d141bfe61e7ea8c7/third_party/WebKit/Source/web/WebDevToolsFrontendImpl.h

Owner: pfeldman@chromium.org
Status: Fixed (was: Available)
I think Pavel has done all the work for this (thanks, Pavel!), so I'll mark this as fixed.  Please reopen if there's any remaining work to be done.

Sign in to add a comment