Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2646683002:
Implement OOPIFs within Devtools Extensions (Closed)
|
Created:
3 years, 11 months ago by davidsac (gone - try alexmos) Modified:
3 years, 8 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, site-isolation-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement OOPIFs within Devtools Extensions
Fix IFrames existing within Devtools to be implemented outside of the devtools processes when necessary.
Currently, some IFrames are rendered within the same process as devtools when they shouldn't be.
The discussion of which IFrames should be allowed in devtools and when ended up becoming fairly complicated. See the attached DESIGN DOC below.
This involved creating numerous chrome extensions for manually testing, debugging, and helping to understand how devtools extensions work. They can be found at http://dudeguy409.github.io/Chrome_Devtools_Extension_Tests/index.html
DESIGN_DOC=https://docs.google.com/a/chromium.org/document/d/1Yxs0DMjoWwiPB5oFh4TRifim7FsEmMdbnd7FGWpReCU/edit?usp=sharing
BUG=570483
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2646683002
Cr-Commit-Position: refs/heads/master@{#460876}
Committed: https://chromium.googlesource.com/chromium/src/+/88a1aa7a6141160af77f9829f64b17c3c6cda5ee
Patch Set 1 #Patch Set 2 : adjust test to check if oopifs and all 3 cases #Patch Set 3 : remove code from failed fix attempt #Patch Set 4 : added more tests for frames within devtools extensions #Patch Set 5 : finish rough draft of tests #
Total comments: 37
Patch Set 6 : refactor duplicate test code #Patch Set 7 : added broken method #Patch Set 8 : finish rough draft of tests without popup.html #Patch Set 9 : finish rough draft of tests without popup.html #
Total comments: 26
Patch Set 10 : add popup check for non-http iframe tests #Patch Set 11 : refactor http iframe tests to use helper extension loader #Patch Set 12 : add tests that about blank and devtools still work in devtools #Patch Set 13 : make suggested fixes #Patch Set 14 : add devtools extension oopif fix implementation #
Total comments: 16
Patch Set 15 : fix all tests for refactoring #Patch Set 16 : small cleanups #Patch Set 17 : added renavigation fix #Patch Set 18 : added test for renavigating to about:blank #
Total comments: 29
Patch Set 19 : Refactor |LoadExtensionForTest| and other fixes #
Total comments: 20
Patch Set 20 : added renavigation test and small fixes #Patch Set 21 : refactor tests to use simple navigation checks #Patch Set 22 : Rebase #Patch Set 23 : added |WaitForLoadStop| and fixed a navigation #
Total comments: 6
Patch Set 24 : fixed bug links in comments #
Total comments: 70
Patch Set 25 : Separate out sidebarpane and panel, and other small fixes #
Total comments: 8
Patch Set 26 : more small fixes #Patch Set 27 : fixed a comment #Patch Set 28 : added TODOs for future bug fix #
Total comments: 8
Patch Set 29 : small URL fixes #
Messages
Total messages: 84 (47 generated)
Description was changed from ========== Implement OOPIFs within Devtools Extensions Fix IFrames existing within Devtools extensions to be implemented outside of the extensions and devtools processes. Currently, they are rendered within the same process as the extension. BUG=570483 ========== to ========== Implement OOPIFs within Devtools Extensions Fix IFrames existing within Devtools extensions to be implemented outside of the extensions and devtools processes. Currently, they are rendered within the same process as the extension. BUG=570483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by davidsac@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
alexmos@chromium.org changed reviewers: + alexmos@chromium.org
Organizing a couple of thoughts about this so far, just to explain why I think it's not sufficient to only check the http/https schemes as in PS1. Basically, we should be creating OOPIFs only when crossing extensions/web boundary *and* --isolate-extensions is enabled, but this check can violate that. For example, if chrome-devtools:// directly embeds an HTTP/HTTPS iframe, and you're running with --isolate-extensions, we'll go ahead and create an OOPIF for that iframe with this code. This is wrong, as --isolate-extensions implies that only extensions embedding web iframes should trigger OOPIFs. I actually went and tried this, and this is indeed what happens. It's actually scary that you can just go and create web iframes inside a chrome-devtools:// page, and nothing will stop you (I did it by inspecting the devtools window itself and just adding an iframe there). But nonetheless, I don't think we should be triggering this isolation with --isolate-extensions, and the only way to learn if you are is to go through the content_browser_client, as Nick outlined on the bug description. Second, as I mentioned there's no reason to make this specific to HTTP/HTTPS. E.g., we've had lots of trouble with nested URLs (blob: or filesystem: URLs) that can have an inner origin that's HTTP/HTTPS but their scheme isn't, and we should be isolating those as well. I don't think we need to special-case HTTP/HTTPS -- if you're in a devtools extension, and you're embedding something that's not the same extension, then we should isolate it in an OOPIF. (An interesting case to think about is whether a devtools extension can embed another devtools extension, and what happens in that case.) Third, I think only extensions that have declared the devtools permission in their manifest should be allowed to be kept in the devtools SiteInstance. If devtools somehow tried to embed a non-devtools-enabled extension, I think we should kick that out to an OOPIF. This would be another thing you should check in ChromeContentBrowserClientExtensionsPart. (And it'd be good to have a test for this too :). )
Hey Andrew -- some preliminary comments below. Overall, I think these tests will give us good coverage. It'd be easier to reason about if you could enumerate the embedding hierarchies they cover (in the description?), e.g.: - devtools -> devtools extension panel -> web iframe (NameOfTestThatCoversIt) - Test1: devtools -> devtools extension sidebar pane -> web iframe (...) - devtools -> web iframe (...) - devtools -> non-devtools extension (...) etc. This way it'd be easier to see if anything's missing. It'd be nice if each test has a description of scenarios it covers as well. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:90: using content::RenderFrameHost; not needed, since you're including the .h for it above. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:934: "src='devtools.js'></script></head><body><iframe src='" + Does this load when you install the extension, without requiring to switch to any extension panel? It's probably good to double-check that it loads as well (for example, with DOMMessageQueue and waiting for "DONE", like others). https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:946: "chrome.devtools.panels.elements.createSidebarPane('iframe_pane',\n" This test is getting kind of long; it's worth thinking about splitting off the sidebar pane and the devtools.html iframe parts into separate tests. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:970: content::DOMMessageQueue message_queue; DOMMessageQueue has to be instantiated before the call that can trigger the "DONE" message, in this case SwitchToExtensionPanel (like it was in the original code), otherwise your test might be flaky due to races. Same for all other uses of DOMMessageQueue below. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:979: SwitchToPanel(window_, "elements"); What is this one for? https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:999: ASSERT_EQ(frame_count, 7); I think this kind of checking might not be sufficient -- it doesn't verify that the right frames are OOPIFs. E.g., if the extension iframes became OOPIFs but the web iframes didn't, this wouldn't catch it. It's probably better to explicitly look up a RFH for each extension iframe, and then for each web iframe, and verify that the SiteInstance for each extension RFH is the same as SiteInstance of the main devtools RFH, and that the SiteInstance of each web iframe RFH is different. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1008: // Tests a chrome.devtools extension panel that embeds an http:// iframe. Is this true? Looks like test.html is a iframe from a second, non-devtools extension, not a http:// iframe. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1010: DevToolsExtensionWithRegExtension) { What does RegExtension stand for? https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1080: RenderFrameHost* rfh = main_web_contents()->GetAllFrames()[2]; Which one is this one? You can rename the var so it's clear. General note for all tests and all uses of GetAllFrames: it's best not to rely on the order of frames in GetAllFrames, which might change - I don't think that function's API isn't guaranteeing any particular order. It'd be better to use ChildFrameAt (for finding child frames by index) or FrameMatchesName (finding frames by name) in browser_test_utils.h, which would be more reliable for what you want. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1088: rfh->ExecuteJavaScript(javascript); Why use this instead of EXPECT_TRUE(ExecuteScript())? The latter is the standard thing we use in all tests. Same for all other uses. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1094: ASSERT_EQ(rfhs.size(), (unsigned)3); Reverse order. It should be (expected, actual) everywhere. Also (unsigned)3 -> 3U. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1095: ASSERT_TRUE(rfhs[0]->GetLastCommittedURL().SchemeIs("chrome-devtools")); ASSERT->EXPECT. ASSERT should be used when the test can't continue if this isn't true, but most places where you have it don't need that and can use EXPECT instead. That way if things go wrong, your test can show everything that's wrong instead of stopping here. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1095: ASSERT_TRUE(rfhs[0]->GetLastCommittedURL().SchemeIs("chrome-devtools")); It's very difficult to know what indices correspond to which RFH. Better to instantiate the RFHs with names like |devtools_rfh|, extension_b_rfh, web_iframe_rfh, etc, which will make the tests more readable. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1105: rfhs[0]->GetSiteInstance()->GetSiteURL().SchemeIs("chrome-devtools")); Use kChromeDevToolsScheme instead of "chrome-devtools". https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1117: rfhs[1]->GetSiteInstance()->GetSiteURL().SchemeIs("chrome-devtools")); rfhs[2] (same bug copy-pasted in a couple of other places below) https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1161: "<html><body>does nothing</body></html>"); Might be better for this test to put the iframe with the second extension here in the panel, instead of in the devtools.html page; I'd imagine that might be slightly more likely to occur in practice? Not a big difference though. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1387: // something Can you investigate how the request is actually blocked? Then we will know how to check for the proper error here. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1400: DevToolsHttpIframeOutsideExtension) { Feels like you can combine this one with the previous one and just re-navigate the same iframe. It's basically about disallowing anything other than devtools extensions in devtools iframes.
Description was changed from ========== Implement OOPIFs within Devtools Extensions Fix IFrames existing within Devtools extensions to be implemented outside of the extensions and devtools processes. Currently, they are rendered within the same process as the extension. BUG=570483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Implement OOPIFs within Devtools Extensions Fix IFrames existing within Devtools extensions to be implemented outside of the extensions and devtools processes. Currently, they are rendered within the same process as the extension. It has been discussed that Iframes inside of devtools but not within devtools extensions should be forbidden, and Iframes of all types within devtools extensions should be allowed, but be rendered outside of the devtools process, unless they are from the same devtools extension of the parent frame. In order to test this new functionality, new tests have been added: 1- DevToolsExtensionWithHttpIframe: tests that http Iframes within the three "devtools pages" (the visible devtools panel for the devtools extension, the sidebar pane page for the devtools extension that is visible in the elemanets panel, and the devtools background page, which is different from the extension's background page)(https://developer.chrome.com/extensions/devtools) are rendered in their own processes and not in the devtools process or the extension's process. 2- DevToolsExtensionWithRegExtension BUG=570483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Implement OOPIFs within Devtools Extensions Fix IFrames existing within Devtools extensions to be implemented outside of the extensions and devtools processes. Currently, they are rendered within the same process as the extension. It has been discussed that Iframes inside of devtools but not within devtools extensions should be forbidden, and Iframes of all types within devtools extensions should be allowed, but be rendered outside of the devtools process, unless they are from the same devtools extension of the parent frame. In order to test this new functionality, new tests have been added: 1- DevToolsExtensionWithHttpIframe: tests that http Iframes within the three "devtools pages" (the visible devtools panel for the devtools extension, the sidebar pane page for the devtools extension that is visible in the elemanets panel, and the devtools background page, which is different from the extension's background page)(https://developer.chrome.com/extensions/devtools) are rendered in their own processes and not in the devtools process or the extension's process. 2- DevToolsExtensionWithRegExtension BUG=570483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Implement OOPIFs within Devtools Extensions Fix IFrames existing within Devtools extensions to be implemented outside of the extensions and devtools processes. Currently, they are rendered within the same process as the extension. It has been discussed that Iframes inside of devtools but not within devtools extensions should be forbidden, and Iframes of all types within devtools extensions should be allowed, but be rendered outside of the devtools process, unless they are from the same devtools extension of the parent frame. In order to test this new functionality, new tests have been added: 1- DevToolsExtensionWithHttpIframe: tests that http Iframes within the three "devtools pages" (the visible devtools panel for the devtools extension, the sidebar pane page for the devtools extension that is visible in the elemanets panel, and the devtools background page, which is different from the extension's background page)(https://developer.chrome.com/extensions/devtools) are rendered in their own processes and not in the devtools process or the extension's process. 2- DevToolsExtensionWithRegExtension: tests that a non-devtools extension can be embedded within a devtools extension, but that it will be isolated from devtools and the devtools extension. 3- DevToolsExtensionWithinDevToolsExtension: tests that if a devtools extension is embedded within a different devtools extension, it isn't accidentally rendered within devtools along with the parent devtools extension. 4- DevToolsExtensionWithinItself: tests that a devtools extension can still have subframes to itself in a "devtools page" and that they will be rendered within the devtools process as well, not in the extension process 5- RegularExtensionInDevtools: tests that an Iframe to a non-devtools extension cannot be injected into the devtools process. 6- DevToolsHttpIframeOutsideExtension: tests that a web Iframe cannot be injected into the devtools process. BUG=570483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Hi Alex, I just made another patchset as a reference for my first round of refactoring, where I have addressed most of your proposed fixes, except for using popup.html. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:90: using content::RenderFrameHost; On 2017/02/08 21:48:00, alexmos wrote: > not needed, since you're including the .h for it above. Done. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:934: "src='devtools.js'></script></head><body><iframe src='" + On 2017/02/08 21:48:00, alexmos (OOO soon) wrote: > Does this load when you install the extension, without requiring to switch to > any extension panel? It's probably good to double-check that it loads as well > (for example, with DOMMessageQueue and waiting for "DONE", like others). Done. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:946: "chrome.devtools.panels.elements.createSidebarPane('iframe_pane',\n" On 2017/02/08 21:48:00, alexmos (OOO soon) wrote: > This test is getting kind of long; it's worth thinking about splitting off the > sidebar pane and the devtools.html iframe parts into separate tests. Done. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:970: content::DOMMessageQueue message_queue; On 2017/02/08 21:48:00, alexmos wrote: > DOMMessageQueue has to be instantiated before the call that can trigger the > "DONE" message, in this case SwitchToExtensionPanel (like it was in the original > code), otherwise your test might be flaky due to races. Same for all other uses > of DOMMessageQueue below. Done. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:979: SwitchToPanel(window_, "elements"); On 2017/02/08 21:48:00, alexmos wrote: > What is this one for? added comment: "//This is a bit of a hack to switch to the sidebar pane in the elements panel that the Iframe has been added to." https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:999: ASSERT_EQ(frame_count, 7); On 2017/02/08 21:48:00, alexmos (OOO soon) wrote: > I think this kind of checking might not be sufficient -- it doesn't verify that > the right frames are OOPIFs. E.g., if the extension iframes became OOPIFs but > the web iframes didn't, this wouldn't catch it. It's probably better to > explicitly look up a RFH for each extension iframe, and then for each web > iframe, and verify that the SiteInstance for each extension RFH is the same as > SiteInstance of the main devtools RFH, and that the SiteInstance of each web > iframe RFH is different. Done. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1008: // Tests a chrome.devtools extension panel that embeds an http:// iframe. On 2017/02/08 21:48:00, alexmos wrote: > Is this true? Looks like test.html is a iframe from a second, non-devtools > extension, not a http:// iframe. Sorry, comments have been corrected. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1010: DevToolsExtensionWithRegExtension) { On 2017/02/08 21:48:00, alexmos wrote: > What does RegExtension stand for? All occurrences have been renamed to non-devtools extension. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1080: RenderFrameHost* rfh = main_web_contents()->GetAllFrames()[2]; On 2017/02/08 21:48:00, alexmos (OOO soon) wrote: > Which one is this one? You can rename the var so it's clear. > > General note for all tests and all uses of GetAllFrames: it's best not to rely > on the order of frames in GetAllFrames, which might change - I don't think that > function's API isn't guaranteeing any particular order. It'd be better to use > ChildFrameAt (for finding child frames by index) or FrameMatchesName (finding > frames by name) in browser_test_utils.h, which would be more reliable for what > you want. Done. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1094: ASSERT_EQ(rfhs.size(), (unsigned)3); On 2017/02/08 21:48:01, alexmos wrote: > Reverse order. It should be (expected, actual) everywhere. Also (unsigned)3 -> > 3U. Done. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1095: ASSERT_TRUE(rfhs[0]->GetLastCommittedURL().SchemeIs("chrome-devtools")); On 2017/02/08 21:48:00, alexmos wrote: > ASSERT->EXPECT. ASSERT should be used when the test can't continue if this > isn't true, but most places where you have it don't need that and can use EXPECT > instead. That way if things go wrong, your test can show everything that's > wrong instead of stopping here. Done. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1095: ASSERT_TRUE(rfhs[0]->GetLastCommittedURL().SchemeIs("chrome-devtools")); On 2017/02/08 21:48:00, alexmos (OOO soon) wrote: > It's very difficult to know what indices correspond to which RFH. Better to > instantiate the RFHs with names like |devtools_rfh|, extension_b_rfh, > web_iframe_rfh, etc, which will make the tests more readable. Done. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1105: rfhs[0]->GetSiteInstance()->GetSiteURL().SchemeIs("chrome-devtools")); On 2017/02/08 21:48:01, alexmos wrote: > Use kChromeDevToolsScheme instead of "chrome-devtools". Done. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1117: rfhs[1]->GetSiteInstance()->GetSiteURL().SchemeIs("chrome-devtools")); On 2017/02/08 21:48:00, alexmos wrote: > rfhs[2] (same bug copy-pasted in a couple of other places below) Done.
I didn't get through everything, but wanted to send out what I have before leaving. It'll be very nice to have this coverage -- thanks for thinking through all the possibilities! The test code does seem a bit long right now. There might be more opportunities for simplifying the code or using helpers, and perhaps we should move the tests out into a separate file/test class, but I'd suggest starting to work on the right plumbing for the fix itself, and coming back to that later. Most important comment below is probably the one about comparing SiteInstances directly whenever you want to check whether two frames are in same/different processes, which applies to all tests. https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:518: const Extension* LoadExtensionForTest(std::string name, nit: const std::string& name https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:521: bool devtools, nit: is_devtools, or is_devtools_extension https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:522: bool devtools_script) { similarly, I think this actually means should_create_panel (this is what the script actually does) https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:528: if (has_test_page) { Do you need to pass this in? What if you just always included test.html in the extension, but it would just be used in some but not all tests? Might be a bit of extra work but it'll make the code easier to understand. https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:977: dir->WriteManifest(extensions::DictionaryBuilder() Can this test (and the next two) also use your new LoadExtensionForTest helper? (I realize the scripts to run differ; perhaps you could pass a flag to toggle between using a panel or a sidebar?) https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:985: embedded_test_server()->GetURL("a.com", "/popup_iframe.html"); This can just stay at its old location in the file, next to its use -- there's no reason to move it up here, right? https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1054: EXPECT_TRUE(devtools_devtools_extension_rfh->GetSiteInstance() Instead of this, let's just expect that devtools_devtools_extension_rfh->GetSiteInstance() is equal to main_devtools_rfh->GetSiteInstance(). Same everywhere below. https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1061: http_iframe_rfh->GetSiteInstance()->GetSiteURL().host()); Similarly, for this kind of check, let's ensure that http_iframe_rfh->GetSiteInstance() is not equal to the devtools SiteInstance. https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1290: LoadExtensionForTest("Devtools Extension", dir.get(), false, true, true); To make the bools more readable (someone reading the test should know what they refer to without needing to look up the helper definition), we typically either include /* param_name */ right after the bool, or use an enum instead. https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1327: non_devtools_extension_rfh->ExecuteJavaScript(javascript); Still wondering if there are any reasons to use ExecuteJavaScript? Seems like EXPECT_TRUE(ExecuteScript(non_devtools_extension_rfh, "your script")); would work just as well, and it would also verify that the script executed properly. https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1394: GURL extension_file_url = devtools_b_extension->GetResourceURL("/test.html"); I'd name this extension_b_url, to make its uses more readable. https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1402: content::RenderFrameHost* devtools_extension_a_devtools_rfh = no need for second "devtools" in the names here https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1427: devtools_extension_b_devtools_rfh->GetLastCommittedURL()); So, technically, at this point, if we ever regressed and started putting this into a separate process, devtools_extension_b_devtools_rfh would be destroyed at this point, and a new RFH would be created for that frame. This test would then crash here, due to the use-after-free on |devtools_extension_b_devtools_rfh|. Might be nice for the test to fail in a nicer way, but this is probably fine for now.
Hi Alex, I made most of the suggested fixes, but had a few questions, so be sure to check my responses. It's funny. Some of the changes I made before reading your comments, which I guess is a good sign that the refactoring is hopefully going well. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1088: rfh->ExecuteJavaScript(javascript); On 2017/02/08 21:48:00, alexmos (OOO until 2-21) wrote: > Why use this instead of EXPECT_TRUE(ExecuteScript())? The latter is the > standard thing we use in all tests. Same for all other uses. Done. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1161: "<html><body>does nothing</body></html>"); On 2017/02/08 21:48:01, alexmos (OOO until 2-21) wrote: > Might be better for this test to put the iframe with the second extension here > in the panel, instead of in the devtools.html page; I'd imagine that might be > slightly more likely to occur in practice? Not a big difference though. Done. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1400: DevToolsHttpIframeOutsideExtension) { On 2017/02/08 21:48:00, alexmos (OOO until 2-21) wrote: > Feels like you can combine this one with the previous one and just re-navigate > the same iframe. It's basically about disallowing anything other than devtools > extensions in devtools iframes. That's definitely a possibility. In either case, it might be better to wait on this until all of the tests are finalized. I personally think it might be a bit cleaner to keep them separate end easier to make sure that the later navigations aren't effected by previous ones. Maybe we can come back to this? https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:518: const Extension* LoadExtensionForTest(std::string name, On 2017/02/15 01:23:41, alexmos (OOO until 2-21) wrote: > nit: const std::string& name Done. https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:521: bool devtools, On 2017/02/15 01:23:41, alexmos (OOO until 2-21) wrote: > nit: is_devtools, or is_devtools_extension Done. https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:522: bool devtools_script) { On 2017/02/15 01:23:41, alexmos (OOO until 2-21) wrote: > similarly, I think this actually means should_create_panel (this is what the > script actually does) Done. Kinda. Renamed to |should_create_panel_and_pane| to adjust for new functionality in the script. https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:528: if (has_test_page) { On 2017/02/15 01:23:41, alexmos (OOO until 2-21) wrote: > Do you need to pass this in? What if you just always included test.html in the > extension, but it would just be used in some but not all tests? Might be a bit > of extra work but it'll make the code easier to understand. Done. I hesitated to do this initially, because I was afraid there would be collisions between one devtools extension trying to ask another extension's test file, but I can see now that it's definitely not an issue. https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:977: dir->WriteManifest(extensions::DictionaryBuilder() On 2017/02/15 01:23:41, alexmos (OOO until 2-21) wrote: > Can this test (and the next two) also use your new LoadExtensionForTest helper? > (I realize the scripts to run differ; perhaps you could pass a flag to toggle > between using a panel or a sidebar?) The first two use this now, but not the third. Let me know how you feel about this. I think it would be fine to finaggle LoadExtensionForTest to work for the third as well, but it might make the method more distorted and confusing to the point that it's not worth it? https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:985: embedded_test_server()->GetURL("a.com", "/popup_iframe.html"); On 2017/02/15 01:23:41, alexmos (OOO until 2-21) wrote: > This can just stay at its old location in the file, next to its use -- there's > no reason to move it up here, right? The code has changed quite a bit, so I'm not sure this is a relevant comment any more. It is still at the beginning now so that the URL can be passed into the helper method. https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1054: EXPECT_TRUE(devtools_devtools_extension_rfh->GetSiteInstance() On 2017/02/15 01:23:41, alexmos (OOO until 2-21) wrote: > Instead of this, let's just expect that > devtools_devtools_extension_rfh->GetSiteInstance() is equal to > main_devtools_rfh->GetSiteInstance(). Same everywhere below. Hmm I feel like it might still be helpful to check the scheme or origin for each expected site instance to make sure that they are getting put in the right process though? Like, to make sure that they don't both have the wrong origin? Is that still reasonable? https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1290: LoadExtensionForTest("Devtools Extension", dir.get(), false, true, true); On 2017/02/15 01:23:41, alexmos (OOO until 2-21) wrote: > To make the bools more readable (someone reading the test should know what they > refer to without needing to look up the helper definition), we typically either > include /* param_name */ right after the bool, or use an enum instead. Done. Tell me if that looks good to you, though. https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1327: non_devtools_extension_rfh->ExecuteJavaScript(javascript); On 2017/02/15 01:23:41, alexmos (OOO until 2-21) wrote: > Still wondering if there are any reasons to use ExecuteJavaScript? > Seems like > EXPECT_TRUE(ExecuteScript(non_devtools_extension_rfh, "your script")); > would work just as well, and it would also verify that the script executed > properly. Done. Sounds great. It was on my todo list, it was just a temporarily low-priority fix for me. https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1394: GURL extension_file_url = devtools_b_extension->GetResourceURL("/test.html"); On 2017/02/15 01:23:41, alexmos (OOO until 2-21) wrote: > I'd name this extension_b_url, to make its uses more readable. Done. https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1402: content::RenderFrameHost* devtools_extension_a_devtools_rfh = On 2017/02/15 01:23:41, alexmos (OOO until 2-21) wrote: > no need for second "devtools" in the names here We might want some other name differentiation then. I agree that the name is confusing and kinda ridiculous-sounding, but I was kinda planning ahead for the current version, where there is both a devtools_rfh and panel_rfh for certain extensions. https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1427: devtools_extension_b_devtools_rfh->GetLastCommittedURL()); On 2017/02/15 01:23:42, alexmos (OOO until 2-21) wrote: > So, technically, at this point, if we ever regressed and started putting this > into a separate process, devtools_extension_b_devtools_rfh would be destroyed at > this point, and a new RFH would be created for that frame. This test would then > crash here, due to the use-after-free on |devtools_extension_b_devtools_rfh|. > Might be nice for the test to fail in a nicer way, but this is probably fine for > now. Done. As part of my refactor, I got rid of the calls to executeScript, so I was able to move the naming of all of the renderframehosts to the last part of the test before the expect statements.
davidsac@chromium.org changed reviewers: + creis@chromium.org, nick@chromium.org
Hi Nick and Charlie, I have added a rought draft fix for OOPIFs in Devtools extensions in patch set 14. Feel free to take a look. The biggest area that I'm still concerned about is finding a way to check which mode Chrome is running in when the code is called, but as previously mentioned, this might not be an issue.
Seems like it's on the right track with what we discussed last week (combined with the scheme -> origin change in https://codereview.chromium.org/2646683002/). Does it behave well in your tests and manual testing? If Nick's happy with the approach, we can clean it up and add some comments for why this makes sense, since it's definitely a bit subtle. :)
https://codereview.chromium.org/2646683002/diff/260001/chrome/browser/devtool... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/260001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:525: // Remove these comments. https://codereview.chromium.org/2646683002/diff/260001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1304: std::string message1; You use 'message1' in several places as a name, but there's never a 'message2'. Why rename message -> message1? https://codereview.chromium.org/2646683002/diff/260001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2646683002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1515: /*if (content::AreAllSitesIsolatedForTesting() || It's supposed to be a PRESUBMIT error if you call any function with a name like ForTest / ForTesting, outside of a unittest/browsertest .cc file. https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md https://codereview.chromium.org/2646683002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1516: extensions::IsIsolateExtensionsEnabled()) { // TODO get this working This function is not known to content; content doesn't know what extensions are. https://codereview.chromium.org/2646683002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1531: return false;*/ This commented-out code should not be committed. https://codereview.chromium.org/2646683002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1533: url::Origin origin = url::Origin(dest_url); What if the origin is unique? https://codereview.chromium.org/2646683002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1538: if (rfh->GetLastCommittedURL().SchemeIs(kChromeDevToolsScheme) || Why would the last committed URL of the frame matter here? This scheme check seems like it ought to be done against |dest_url| (or |origin|) -- i.e., the origin being navigated to -- instead of against the URL used for the previous navigation. https://codereview.chromium.org/2646683002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1539: rfh->GetLastCommittedURL() == GURL(url::kAboutBlankURL)) { Why can we assume that a frame currently with an "about:blank" URL cannot ever navigate to an http URL, and need a process transfer? Any frame can be navigated to "about:blank".
Hi Alex and/or Nick, Would you please take a second look at this changelist? https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1387: // something On 2017/02/08 21:48:00, alexmos wrote: > Can you investigate how the request is actually blocked? Then we will know how > to check for the proper error here. Done. Irrelevant. This (and later tests) have been deleted due to the change in the navigation policy. https://codereview.chromium.org/2646683002/diff/80001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:1400: DevToolsHttpIframeOutsideExtension) { On 2017/02/16 20:29:01, davidsac wrote: > On 2017/02/08 21:48:00, alexmos (OOO until 2-21) wrote: > > Feels like you can combine this one with the previous one and just re-navigate > > the same iframe. It's basically about disallowing anything other than > devtools > > extensions in devtools iframes. > > That's definitely a possibility. In either case, it might be better to wait on > this until all of the tests are finalized. I personally think it might be a bit > cleaner to keep them separate end easier to make sure that the later navigations > aren't effected by previous ones. Maybe we can come back to this? As mentioned earlier, this is no longer relevant as these tests have been deleted now. https://codereview.chromium.org/2646683002/diff/260001/chrome/browser/devtool... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/260001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:525: // On 2017/02/21 22:19:53, ncarter wrote: > Remove these comments. Done. https://codereview.chromium.org/2646683002/diff/260001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1304: std::string message1; On 2017/02/21 22:19:53, ncarter wrote: > You use 'message1' in several places as a name, but there's never a 'message2'. > Why rename message -> message1? Done.
Description was changed from ========== Implement OOPIFs within Devtools Extensions Fix IFrames existing within Devtools extensions to be implemented outside of the extensions and devtools processes. Currently, they are rendered within the same process as the extension. It has been discussed that Iframes inside of devtools but not within devtools extensions should be forbidden, and Iframes of all types within devtools extensions should be allowed, but be rendered outside of the devtools process, unless they are from the same devtools extension of the parent frame. In order to test this new functionality, new tests have been added: 1- DevToolsExtensionWithHttpIframe: tests that http Iframes within the three "devtools pages" (the visible devtools panel for the devtools extension, the sidebar pane page for the devtools extension that is visible in the elemanets panel, and the devtools background page, which is different from the extension's background page)(https://developer.chrome.com/extensions/devtools) are rendered in their own processes and not in the devtools process or the extension's process. 2- DevToolsExtensionWithRegExtension: tests that a non-devtools extension can be embedded within a devtools extension, but that it will be isolated from devtools and the devtools extension. 3- DevToolsExtensionWithinDevToolsExtension: tests that if a devtools extension is embedded within a different devtools extension, it isn't accidentally rendered within devtools along with the parent devtools extension. 4- DevToolsExtensionWithinItself: tests that a devtools extension can still have subframes to itself in a "devtools page" and that they will be rendered within the devtools process as well, not in the extension process 5- RegularExtensionInDevtools: tests that an Iframe to a non-devtools extension cannot be injected into the devtools process. 6- DevToolsHttpIframeOutsideExtension: tests that a web Iframe cannot be injected into the devtools process. BUG=570483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Implement OOPIFs within Devtools Extensions Fix IFrames existing within Devtools to be implemented outside of the devtools processes when necessary. Currently, some IFrames are rendered within the same process as devtools when they shouldn't be. The discussion of which IFrames should be allowed in devtools and when ended up becoming fairly complicated. See the attached DESIGN DOC below. In order to test the new functionality, new tests have been added: 1- DevToolsExtensionWithHttpIframe: tests that http Iframes within the three "devtools pages" (the visible devtools panel for the devtools extension, the sidebar pane page for the devtools extension that is visible in the elemanets panel, and the devtools background page, which is different from the extension's background page)(https://developer.chrome.com/extensions/devtools) are rendered in their own processes and not in the devtools process or the extension's process. 2- DevToolsExtensionWithRegExtension: tests that a non-devtools extension can be embedded within a devtools extension, but that it will be isolated from devtools and the devtools extension. 3- DevToolsExtensionWithinDevToolsExtension: tests that if a devtools extension is embedded within a different devtools extension, it isn't accidentally rendered within devtools along with the parent devtools extension. 4- DevToolsExtensionWithinItself: tests that a devtools extension can still have subframes to itself in a "devtools page" and that they will be rendered within the devtools process as well, not in the extension process 5- RegularExtensionInDevtools: tests that an Iframe to a non-devtools extension cannot be injected into the devtools process. 6- DevToolsHttpIframeOutsideExtension: tests that a web Iframe cannot be injected into the devtools process. DESIGN_DOC=https://docs.google.com/a/chromium.org/document/d/1Yxs0DMjoWwiPB5oFh4TRifim7FsEmMdbnd7FGWpReCU/edit?usp=sharing BUG=570483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Implement OOPIFs within Devtools Extensions Fix IFrames existing within Devtools to be implemented outside of the devtools processes when necessary. Currently, some IFrames are rendered within the same process as devtools when they shouldn't be. The discussion of which IFrames should be allowed in devtools and when ended up becoming fairly complicated. See the attached DESIGN DOC below. In order to test the new functionality, new tests have been added: 1- DevToolsExtensionWithHttpIframe: tests that http Iframes within the three "devtools pages" (the visible devtools panel for the devtools extension, the sidebar pane page for the devtools extension that is visible in the elemanets panel, and the devtools background page, which is different from the extension's background page)(https://developer.chrome.com/extensions/devtools) are rendered in their own processes and not in the devtools process or the extension's process. 2- DevToolsExtensionWithRegExtension: tests that a non-devtools extension can be embedded within a devtools extension, but that it will be isolated from devtools and the devtools extension. 3- DevToolsExtensionWithinDevToolsExtension: tests that if a devtools extension is embedded within a different devtools extension, it isn't accidentally rendered within devtools along with the parent devtools extension. 4- DevToolsExtensionWithinItself: tests that a devtools extension can still have subframes to itself in a "devtools page" and that they will be rendered within the devtools process as well, not in the extension process 5- RegularExtensionInDevtools: tests that an Iframe to a non-devtools extension cannot be injected into the devtools process. 6- DevToolsHttpIframeOutsideExtension: tests that a web Iframe cannot be injected into the devtools process. DESIGN_DOC=https://docs.google.com/a/chromium.org/document/d/1Yxs0DMjoWwiPB5oFh4TRifim7FsEmMdbnd7FGWpReCU/edit?usp=sharing BUG=570483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Implement OOPIFs within Devtools Extensions Fix IFrames existing within Devtools to be implemented outside of the devtools processes when necessary. Currently, some IFrames are rendered within the same process as devtools when they shouldn't be. The discussion of which IFrames should be allowed in devtools and when ended up becoming fairly complicated. See the attached DESIGN DOC below. DESIGN_DOC=https://docs.google.com/a/chromium.org/document/d/1Yxs0DMjoWwiPB5oFh4TRifim7FsEmMdbnd7FGWpReCU/edit?usp=sharing BUG=570483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Replied to some previously unanswered comments, and updated the CL description (cleaned it up and added a link to a design doc). https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/160001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1061: http_iframe_rfh->GetSiteInstance()->GetSiteURL().host()); On 2017/02/15 01:23:41, alexmos wrote: > Similarly, for this kind of check, let's ensure that > http_iframe_rfh->GetSiteInstance() is not equal to the devtools SiteInstance. Are you sure? I can change it, but I felt like it might not be better to have a more specific check. Are we thinking the result for this might change for things like TDI mode? https://codereview.chromium.org/2646683002/diff/260001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2646683002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1515: /*if (content::AreAllSitesIsolatedForTesting() || On 2017/02/21 22:19:53, ncarter wrote: > It's supposed to be a PRESUBMIT error if you call any function with a name like > ForTest / ForTesting, outside of a unittest/browsertest .cc file. > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md Done. https://codereview.chromium.org/2646683002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1516: extensions::IsIsolateExtensionsEnabled()) { // TODO get this working On 2017/02/21 22:19:53, ncarter wrote: > This function is not known to content; content doesn't know what extensions are. Done. https://codereview.chromium.org/2646683002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1531: return false;*/ On 2017/02/21 22:19:53, ncarter wrote: > This commented-out code should not be committed. Done. https://codereview.chromium.org/2646683002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1533: url::Origin origin = url::Origin(dest_url); On 2017/02/21 22:19:53, ncarter wrote: > What if the origin is unique? I'm still not entirely sure what you meant by that or if it is an issue, but I think it maybe have been fixed. If not, could you please clarify? https://codereview.chromium.org/2646683002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1538: if (rfh->GetLastCommittedURL().SchemeIs(kChromeDevToolsScheme) || On 2017/02/21 22:19:53, ncarter wrote: > Why would the last committed URL of the frame matter here? > > This scheme check seems like it ought to be done against |dest_url| (or > |origin|) -- i.e., the origin being navigated to -- instead of against the URL > used for the previous navigation. Done. This was using a weird strategy to only allow OOPIFs in devtools if they were in a devtools extension, but that design approach has been abandoned. https://codereview.chromium.org/2646683002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1539: rfh->GetLastCommittedURL() == GURL(url::kAboutBlankURL)) { On 2017/02/21 22:19:53, ncarter wrote: > Why can we assume that a frame currently with an "about:blank" URL cannot ever > navigate to an http URL, and need a process transfer? > > Any frame can be navigated to "about:blank". Done. This was removed after the change in design.
https://codereview.chromium.org/2646683002/diff/340001/chrome/browser/devtool... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/340001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:520: // call them individually in each test instead of these if/elses? I might split this into two functions (for the is_devtools_extension=True and is_devtools_extension=False cases), and then try to simplify or eliminate the remaining parameters -- see below for some ideas. For the case of the non-devtools extension, you might just load something off disk (i.e. add a new directory to chrome/test/data/devtools/extensions, either forking or extending the simple_content_script extension). All you need is a web_accessible_resource right? You only really need to dynamically generate the extension if it's embedding content that can't be known at compile time. https://codereview.chromium.org/2646683002/diff/340001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:523: extensions::TestExtensionDir* dir, Having to pass in a TestExtensionDir seems like a burden. In other places, what we've done instead is to add a vector<unique_ptr<TestExtensionDir>> test_extension_directories_ to the test fixture (here that would be class DevToolsExtensionTest), and have the LoadExtensionForTest function create a dir, and then push it into the vector, for automatic cleanup later: https://cs.chromium.org/search/?q=vector.*TestExtensionDir+package:%5Echromiu... https://codereview.chromium.org/2646683002/diff/340001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:527: const std::string& test_page_url = "test.html") { Could you combine should_create_panel_and_pane and test_page_url? I.e. make the empty string correspond to the should_create_panel_and_pane=False case? https://codereview.chromium.org/2646683002/diff/340001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:573: if (has_popup_test_page) { Rather than parameterizing this with a boolean |has_popup_test_page|, it might be clearer to just write out multiple html files unconditionally: - send_done_on_load.html - simple.html Looks like renavigation_test.html could even be done the same way, since you can call embedded_testserver() from here. https://codereview.chromium.org/2646683002/diff/340001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1104: /*LOG(INFO) << "about blank last committed" There's some debugging cruft in this file that you should revisit. https://codereview.chromium.org/2646683002/diff/340001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1238: // TODO(davidsac): is this worth refactoring to use |LoadExtensionForTest|? Yes. I think you just pass in http_iframe_url and you're done, right? https://codereview.chromium.org/2646683002/diff/340001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1470: // flaky. How unfortunate. Looks like the source of this nondeterminism is the ExtensionSet itself. Easiest way to unflake this might be https://cs.chromium.org/search/?q=FrameHasSourceUrl+package:%5Echromium$&type=cs RenderFrameHost* devtools_extension_a_devtools_rfh = content::FrameMatchingPredicate( main_web_contents, base::Bind(&content::FrameHasSourceUrl, devtools_a_extension->GetResourceURL("/devtools.html"))); EXPECT_TRUE(devtools_extension_a_devtools_rfh); https://codereview.chromium.org/2646683002/diff/340001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2646683002/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1341: if (parent_site_instance->GetSiteURL().SchemeIs(kChromeDevToolsScheme)) { I think using |frame_tree_node_->parent()| is probably correct here. The other candidate we might consider, though is the SiteInstance of |frame_tree_node_->root()|, and I'm curious what you and Alex think about that. The only difference might be in exotic cases, say if you had an http iframe embed an extension iframe: devtools(extension(a.com(extension))) ? One hopes that this doesn't happen in practice. https://codereview.chromium.org/2646683002/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1343: auto* policy = content::ChildProcessSecurityPolicy::GetInstance(); content:: is unnecessary here; we're already in that namespace. https://codereview.chromium.org/2646683002/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1344: bool is_devtools_extension = Technically using 'extension' in this variable name is a layering violation -- //content doesn't know what extensions are. But, the clarity gain here is worthwhile. I suggest the compromise of a name like: "bool origin_allowed_in_devtools_process" But mention the 'extension' case in a comment like: // Some non-devtools origins (e.g., devtools extensions) have special permission to // stay in the devtools process. https://codereview.chromium.org/2646683002/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1347: ->GetProcess() Instead of calling GetProcess on current_frame_host, you can just call GetProcess on the SiteInstance, |parent_site_instance|. These are provably equivalent: RenderFrameHostImpl::GetProcess returns process_, which is initialized to "process_(site_instance->GetProcess())" in the RenderFrameHostImpl constructor. I think using |parent_site_instance| here reads better, since this block of code is a decision about whether or not we can reuse |parent_site_instance|. https://codereview.chromium.org/2646683002/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1350: if (dest_url.SchemeIs(kChromeDevToolsScheme) || is_devtools_extension) { Since you've already computed |origin|, it's probably better here to check the scheme of |origin| instead of |dest_url|. The only cases it'll differ are blob and filesystem URLs, which as far as I know, devtools doesn't use, but we might as well handle them since it's free. https://codereview.chromium.org/2646683002/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1528: auto* policy = content::ChildProcessSecurityPolicy::GetInstance(); content:: is unnecessary here, we're already inside "namespace content {}". https://codereview.chromium.org/2646683002/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1531: return !(dest_url.SchemeIs(kChromeDevToolsScheme) || is_devtools_extension); As above, checking the scheme on |origin| instead of |dest_url| will produce better results, for the [probably irrelevant] corner case of "blob:chrome-devtools://" URLs.
https://codereview.chromium.org/2646683002/diff/340001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2646683002/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1341: if (parent_site_instance->GetSiteURL().SchemeIs(kChromeDevToolsScheme)) { On 2017/03/09 23:28:49, ncarter wrote: > I think using |frame_tree_node_->parent()| is probably correct here. > > The other candidate we might consider, though is the SiteInstance of > |frame_tree_node_->root()|, and I'm curious what you and Alex think about that. > > The only difference might be in exotic cases, say if you had an http iframe > embed an extension iframe: devtools(extension(a.com(extension))) ? One hopes > that this doesn't happen in practice. Yes, I think we chatted about using the root here and decided to go with the parent. I agree this only matters for cases like devtools(extension(a.com(extension))), so the question is where we want to put the innermost extension frame. I think it's safer to put it in a regular extension process rather than the devtools process, since otherwise we'd be letting web content have some control over what gets embedded in the devtools process, which seems dangerous. Plus, the web content will presumably only be able to embed the devtools extension using an <iframe>, which isn't one of the cases that should get devtools API access according to the docs (those cases are the devtools_page and the panels/sidebars created there via chrome.devtools.panels.create and chrome.devtools.panels.elements.createSidebarPane). Andrew - I forget, does this case actually work without your changes today? Does the innermost extension frame load?
The CQ bit was checked by davidsac@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I have made another round of changes. Nick, thank you for all of the brilliant suggestions on how to refactor |LoadExtensionForTest|. I think things turned out well, but let me know. Nick, I will be out almost all day Monday and Tuesday, so please feel free to take another look at the changes and responses, and give additional feedback and/or sync up with Alex for him to review it. Thanks. https://codereview.chromium.org/2646683002/diff/340001/chrome/browser/devtool... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/340001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:520: // call them individually in each test instead of these if/elses? On 2017/03/09 23:28:49, ncarter wrote: > I might split this into two functions (for the is_devtools_extension=True and > is_devtools_extension=False cases), and then try to simplify or eliminate the > remaining parameters -- see below for some ideas. > > For the case of the non-devtools extension, you might just load something off > disk (i.e. add a new directory to chrome/test/data/devtools/extensions, either > forking or extending the simple_content_script extension). All you need is a > web_accessible_resource right? > > You only really need to dynamically generate the extension if it's embedding > content that can't be known at compile time. Done. Kinda. I had talked to Alex about this earlier, and the general concensus was that even though it wasn't necessary, enough of the extensions reused enough of the same files with only slight variations that dynamically generating them was preferable. That being said, I took in your suggestions and put a bit of my own spin on it by passing in a string of the name of the devtools page to use, or an empty string if it's a regular extension. Let me know what you think, but I think this new fix should be preferable to two separate methods. https://codereview.chromium.org/2646683002/diff/340001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:523: extensions::TestExtensionDir* dir, On 2017/03/09 23:28:49, ncarter wrote: > Having to pass in a TestExtensionDir seems like a burden. > > In other places, what we've done instead is to add a > vector<unique_ptr<TestExtensionDir>> test_extension_directories_ to the test > fixture (here that would be class DevToolsExtensionTest), and have the > LoadExtensionForTest function create a dir, and then push it into the vector, > for automatic cleanup later: > > https://cs.chromium.org/search/?q=vector.*TestExtensionDir+package:%5Echromiu... Done. https://codereview.chromium.org/2646683002/diff/340001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:527: const std::string& test_page_url = "test.html") { On 2017/03/09 23:28:48, ncarter wrote: > Could you combine should_create_panel_and_pane and test_page_url? I.e. make the > empty string correspond to the should_create_panel_and_pane=False case? Done. https://codereview.chromium.org/2646683002/diff/340001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:573: if (has_popup_test_page) { On 2017/03/09 23:28:49, ncarter wrote: > Rather than parameterizing this with a boolean |has_popup_test_page|, it might > be clearer to just write out multiple html files unconditionally: > - send_done_on_load.html > - simple.html > > Looks like renavigation_test.html could even be done the same way, since you can > call embedded_testserver() from here. Done. https://codereview.chromium.org/2646683002/diff/340001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1104: /*LOG(INFO) << "about blank last committed" On 2017/03/09 23:28:48, ncarter wrote: > There's some debugging cruft in this file that you should revisit. Done. https://codereview.chromium.org/2646683002/diff/340001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1238: // TODO(davidsac): is this worth refactoring to use |LoadExtensionForTest|? On 2017/03/09 23:28:48, ncarter wrote: > Yes. I think you just pass in http_iframe_url and you're done, right? Done. It was actually a bit more complicated than that, because this test embeds the popup iframe in the devtools hextension's devtools.html page, rather than on the panel. Basically, refactoring it from my old approach would have meant adding in a third possible devtools.html page. Fortunately, this complexity was handled nicely in the refactoring that you hinted at/suggested. Thanks a million! https://codereview.chromium.org/2646683002/diff/340001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1470: // flaky. On 2017/03/09 23:28:49, ncarter wrote: > How unfortunate. Looks like the source of this nondeterminism is the > ExtensionSet itself. > > Easiest way to unflake this might be > https://cs.chromium.org/search/?q=FrameHasSourceUrl+package:%5Echromium$&type=cs > > RenderFrameHost* devtools_extension_a_devtools_rfh = > content::FrameMatchingPredicate( > main_web_contents, > base::Bind(&content::FrameHasSourceUrl, > devtools_a_extension->GetResourceURL("/devtools.html"))); > EXPECT_TRUE(devtools_extension_a_devtools_rfh); Done. Thanks a bunch! That was very useful. https://codereview.chromium.org/2646683002/diff/340001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2646683002/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1341: if (parent_site_instance->GetSiteURL().SchemeIs(kChromeDevToolsScheme)) { On 2017/03/10 21:18:26, alexmos wrote: > On 2017/03/09 23:28:49, ncarter wrote: > > I think using |frame_tree_node_->parent()| is probably correct here. > > > > The other candidate we might consider, though is the SiteInstance of > > |frame_tree_node_->root()|, and I'm curious what you and Alex think about > that. > > > > The only difference might be in exotic cases, say if you had an http iframe > > embed an extension iframe: devtools(extension(a.com(extension))) ? One hopes > > that this doesn't happen in practice. > > Yes, I think we chatted about using the root here and decided to go with the > parent. I agree this only matters for cases like > devtools(extension(a.com(extension))), so the question is where we want to put > the innermost extension frame. I think it's safer to put it in a regular > extension process rather than the devtools process, since otherwise we'd be > letting web content have some control over what gets embedded in the devtools > process, which seems dangerous. Plus, the web content will presumably only be > able to embed the devtools extension using an <iframe>, which isn't one of the > cases that should get devtools API access according to the docs (those cases are > the devtools_page and the panels/sidebars created there via > chrome.devtools.panels.create and > chrome.devtools.panels.elements.createSidebarPane). > > Andrew - I forget, does this case actually work without your changes today? > Does the innermost extension frame load? Yeah, Alex pretty much covered it all. Even if a web Iframe embedded a devtools extension page and it were in the devtools process, it would not have access to the devtools APIs for extensions. Those permissions are very specific. As Alex mentioned, only the extension's devtools page initially has devtools api access, and it can only grant access to the panel and sidebarpane page. simply putting it into the devtools process doesn't really do anything for it, except perhaps require it to use message passing to communicate with pages in the regular extension process. To answer your question, Alex, without my fix at all, whatever is embedded in a devtools extension page in devtools stays in the devtools process, including other devtools extension pages. In sort, I think Alex and I came to a consensus that it would be preferable to keep devtools extensions pages within web pages in the extension's process at all times. Does that sound good? https://codereview.chromium.org/2646683002/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1343: auto* policy = content::ChildProcessSecurityPolicy::GetInstance(); On 2017/03/09 23:28:49, ncarter wrote: > content:: is unnecessary here; we're already in that namespace. Done. https://codereview.chromium.org/2646683002/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1344: bool is_devtools_extension = On 2017/03/09 23:28:49, ncarter wrote: > Technically using 'extension' in this variable name is a layering violation -- > //content doesn't know what extensions are. But, the clarity gain here is > worthwhile. > > I suggest the compromise of a name like: > "bool origin_allowed_in_devtools_process" > But mention the 'extension' case in a comment like: > // Some non-devtools origins (e.g., devtools extensions) have special > permission to > // stay in the devtools process. Done. https://codereview.chromium.org/2646683002/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1347: ->GetProcess() On 2017/03/09 23:28:49, ncarter wrote: > Instead of calling GetProcess on current_frame_host, you can just call > GetProcess on the SiteInstance, |parent_site_instance|. > > These are provably equivalent: RenderFrameHostImpl::GetProcess returns process_, > which is initialized to "process_(site_instance->GetProcess())" in the > RenderFrameHostImpl constructor. > > I think using |parent_site_instance| here reads better, since this block of code > is a decision about whether or not we can reuse |parent_site_instance|. Done. https://codereview.chromium.org/2646683002/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1350: if (dest_url.SchemeIs(kChromeDevToolsScheme) || is_devtools_extension) { On 2017/03/09 23:28:49, ncarter wrote: > Since you've already computed |origin|, it's probably better here to check the > scheme of |origin| instead of |dest_url|. The only cases it'll differ are blob > and filesystem URLs, which as far as I know, devtools doesn't use, but we might > as well handle them since it's free. Done, I think? I used |origin.scheme()==kChromeDevToolsScheme|. https://codereview.chromium.org/2646683002/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1528: auto* policy = content::ChildProcessSecurityPolicy::GetInstance(); On 2017/03/09 23:28:49, ncarter wrote: > content:: is unnecessary here, we're already inside "namespace content {}". Done. https://codereview.chromium.org/2646683002/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1531: return !(dest_url.SchemeIs(kChromeDevToolsScheme) || is_devtools_extension); On 2017/03/09 23:28:49, ncarter wrote: > As above, checking the scheme on |origin| instead of |dest_url| will produce > better results, for the [probably irrelevant] corner case of > "blob:chrome-devtools://" URLs. Done, I think? I used |origin.scheme()==kChromeDevToolsScheme|.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
this is very close https://codereview.chromium.org/2646683002/diff/360001/chrome/browser/devtool... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/360001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:7: #include <memory> #include <vector> https://codereview.chromium.org/2646683002/diff/360001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:518: // Loads a dynamically generated extension populated with a bunch of test // pages. |name| is the extension name to use in the manifest. // |devtools_page|, if non-empty, indicates which test page should be be // listed as a devtools_page in the manifest. |panel_iframe_src| controls the // src= attribute of the <iframe> element in the 'panel.html' test page. https://codereview.chromium.org/2646683002/diff/360001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:520: const std::string& devtools_page_url, this actually isn't an URL, it's a file, right? Calling this 'devtools_page' might be a better name. https://codereview.chromium.org/2646683002/diff/360001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:521: const std::string& test_page_url) { Rename "test_page_url -> panel_iframe_src" https://codereview.chromium.org/2646683002/diff/360001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:997: // The following three tests check that http Iframes within the three "devtools A comment that applies to 'the next three tests' is likely to fall out of date, because it's unlikely that it'll be updated if the tests are rearranged or a new one is inserted. I think it's better to just give each test a standalone comment. The contents of this summary paragraph can go either into those standalone comments, or into the CL description. https://codereview.chromium.org/2646683002/diff/360001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1108: // extension that is visible in the elemanets panel are rendered in their own elemanets is a typo (occurs a couple times in this file). https://codereview.chromium.org/2646683002/diff/360001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1460: // Tests that a devtools (not a devtools extension) Iframe can still be injected 'can still be' -> 'can be' https://codereview.chromium.org/2646683002/diff/360001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1494: devtools_iframe_rfh->GetSiteInstance()); Might be worthwhile to interact with the loaded page too. E.g.: EXPECT_TRUE(ExecuteScriptAndExtractString(devtools_iframe_rfh, 'domAutomationController.send(document.origin)', &message)); EXPECT_EQ('chrome-devtools://', messge); https://codereview.chromium.org/2646683002/diff/360001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2646683002/diff/360001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1525: // for external navigations in iframes. See https://crbug.com/564216. Maybe we could phrase this comment more clearly: // Devtools pages and devtools extensions must stay in the devtools process. https://codereview.chromium.org/2646683002/diff/360001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1529: bool is_devtools_extension = policy->HasSpecificPermissionForOrigin( Rename is_devtools_extension -> is_origin_allowed_in_devtools_process, to be consistent with the other spot.
The CQ bit was checked by davidsac@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by davidsac@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by davidsac@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by davidsac@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Nick, I have done my best to address your feedback. Please Take another look. https://codereview.chromium.org/2646683002/diff/360001/chrome/browser/devtool... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/360001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:7: #include <memory> On 2017/03/15 20:01:02, ncarter wrote: > #include <vector> Done. https://codereview.chromium.org/2646683002/diff/360001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:518: On 2017/03/15 20:01:02, ncarter wrote: > // Loads a dynamically generated extension populated with a bunch of test > // pages. |name| is the extension name to use in the manifest. > // |devtools_page|, if non-empty, indicates which test page should be be > // listed as a devtools_page in the manifest. |panel_iframe_src| controls the > // src= attribute of the <iframe> element in the 'panel.html' test page. Done. Good call on a comment here. I also mentioned that a blank |devtools_page| will create a non-devtools extension. https://codereview.chromium.org/2646683002/diff/360001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:520: const std::string& devtools_page_url, On 2017/03/15 20:01:02, ncarter wrote: > this actually isn't an URL, it's a file, right? Calling this 'devtools_page' > might be a better name. Done. https://codereview.chromium.org/2646683002/diff/360001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:521: const std::string& test_page_url) { On 2017/03/15 20:01:02, ncarter wrote: > Rename "test_page_url -> panel_iframe_src" Done. https://codereview.chromium.org/2646683002/diff/360001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:997: // The following three tests check that http Iframes within the three "devtools On 2017/03/15 20:01:02, ncarter wrote: > A comment that applies to 'the next three tests' is likely to fall out of date, > because it's unlikely that it'll be updated if the tests are rearranged or a new > one is inserted. > > I think it's better to just give each test a standalone comment. The contents of > this summary paragraph can go either into those standalone comments, or into the > CL description. Done. https://codereview.chromium.org/2646683002/diff/360001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1108: // extension that is visible in the elemanets panel are rendered in their own On 2017/03/15 20:01:02, ncarter wrote: > elemanets is a typo (occurs a couple times in this file). Done. https://codereview.chromium.org/2646683002/diff/360001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1460: // Tests that a devtools (not a devtools extension) Iframe can still be injected On 2017/03/15 20:01:02, ncarter wrote: > 'can still be' -> 'can be' Done. https://codereview.chromium.org/2646683002/diff/360001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1494: devtools_iframe_rfh->GetSiteInstance()); On 2017/03/15 20:01:02, ncarter wrote: > Might be worthwhile to interact with the loaded page too. E.g.: > > EXPECT_TRUE(ExecuteScriptAndExtractString(devtools_iframe_rfh, > 'domAutomationController.send(document.origin)', &message)); > EXPECT_EQ('chrome-devtools://', messge); Done. https://codereview.chromium.org/2646683002/diff/360001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2646683002/diff/360001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1525: // for external navigations in iframes. See https://crbug.com/564216. On 2017/03/15 20:01:02, ncarter wrote: > Maybe we could phrase this comment more clearly: > > // Devtools pages and devtools extensions must stay in the devtools process. Done. https://codereview.chromium.org/2646683002/diff/360001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1529: bool is_devtools_extension = policy->HasSpecificPermissionForOrigin( On 2017/03/15 20:01:02, ncarter wrote: > Rename is_devtools_extension -> is_origin_allowed_in_devtools_process, to be > consistent with the other spot. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2646683002/diff/430001/chrome/browser/devtool... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/430001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1031: // BUG=crbug.com/570483 BUG= -> https:// https://codereview.chromium.org/2646683002/diff/430001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1152: // (https://developer.chrome.com/extensions/devtools). BUG=crbug.com/570483 "BUG=crbug.com/570383" -> "https://crbug.com/570383" (just make it a link) https://codereview.chromium.org/2646683002/diff/430001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1265: // BUG=crbug.com/570483 BUG= -> https:// as above
https://codereview.chromium.org/2646683002/diff/430001/chrome/browser/devtool... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/430001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1031: // BUG=crbug.com/570483 On 2017/03/20 17:22:12, ncarter wrote: > BUG= -> https:// Done. https://codereview.chromium.org/2646683002/diff/430001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1152: // (https://developer.chrome.com/extensions/devtools). BUG=crbug.com/570483 On 2017/03/20 17:22:12, ncarter wrote: > "BUG=crbug.com/570383" -> "https://crbug.com/570383" (just make it a link) Done. https://codereview.chromium.org/2646683002/diff/430001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1265: // BUG=crbug.com/570483 On 2017/03/20 17:22:12, ncarter wrote: > BUG= -> https:// as above Done.
ps23 vs ps24 lgtm You'll need a devtools reviewer.
Just a bunch of minor nits from my side, but otherwise this looks ready to go. Thanks for being so thorough with the tests! I'm glad we'll have all that coverage. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:529: if (!devtools_page.empty()) { nit: no {} https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:535: GURL http_popup_url = Since you're using it from an <iframe>, this isn't really a popup. Perhaps rename to http_frame_url? https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:538: dir->WriteFile(FILE_PATH_LITERAL("web_devtools_page.html"), Maybe add a comment before this saying that only one of these devtools pages will ends up being used, and different tests use |devtools_page| to specify which one. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:539: "<html><head></head><body><iframe src='" + nit: no need for <head></head>, here and similarly below https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:557: "chrome.devtools.inspectedWindow.eval('console.log(" nit: fix the indent here and remove the preceding blank line. Same for indent in createSidebarPane https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:578: dir->WriteFile(FILE_PATH_LITERAL("renavigation_test_page.html"), In the tests, this seems to be used for a lot more than renavigation. Perhaps use a more generic name, e.g., page_with_three_frames? https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1026: // (https://developer.chrome.com/extensions/devtools). Also tests that data nit: insert "iframes with " before data. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1027: // URLs and about blank URLs are rendered in the devtools process, unless an nit: add ":" in about:blank https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1028: // OOPIF is navigated to about:blank, in which case it does not end up back in nit: "an OOPIF is navigated" -> "a web OOPIF navigates itself" (OOPIF does not imply being a web iframe; also, we don't want ambiguity about who's navigating the frame and inheriting the initiator's origin, which this test isn't concerned with) https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1052: std::vector<content::RenderFrameHost*> rfhs = Given the complexity of this test, it might help with readability if you include a short comment that outlines the structure of the whole page? It could include something like this: - DevTools - devtools_page from DevTools extension - Panel (DevTools extension) - iframe (DevTools extension) - about:blank - data: - web URL https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1054: EXPECT_EQ(7U, rfhs.size()); Sanity check: it seems for this test, we'll also install a sidebar pane that points to renavigation_test_page.html, so it will also have the three iframes. Are we relying on the fact that the sidebar pane isn't shown until you switch to the elements panel? https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1056: content::RenderFrameHost* main_devtools_rfh = nit: given that it's used so often, you could just put a using content::RenderFrameHost on top (this file already does this for RenderViewHost and a bunch of others, so it'll fit right in), and then drop "content::" from RenderFrameHost* everywhere. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1058: content::RenderFrameHost* devtools_extension_devtools_rfh = This is for the "devtools_page" from the manifest, so perhaps the name should have devtools_page? https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1062: content::RenderFrameHost* test_frame_rfh = I'd name this panel_frame_rfh https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1067: content::RenderFrameHost* about_blank_http_iframe_rfh = Why not just web_frame_rfh? https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1086: EXPECT_TRUE(main_devtools_rfh->GetSiteInstance()->GetSiteURL().SchemeIs( nit: you could declare SiteInstance* devtools_instance = main_devtools_rfh->GetSiteInstance(); and then use that everywhere below for a bit of extra clarify and to save a bunch of space. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1100: about_blank_http_iframe_rfh->GetSiteInstance()->GetSiteURL().host()); Too bad that DepictFrameTree is internal to content/. It would've been so nice to use it here instead of all the GetSiteInstance checks. Perhaps we should consider exposing it to tests outside content/ (certainly not for this CL). https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1100: about_blank_http_iframe_rfh->GetSiteInstance()->GetSiteURL().host()); I'd add an explicit EXPECT_NE(devtools_instance, about_blank_http_iframe_rfh->GetSiteInstance()) (and similarly in all other tests) https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1102: // Check that if an OOPIF navigates itself to about:blank, it stays an OOPIF. nit: Check that if the web iframe navigates itself to about:blank, it stays in the web SiteInstance. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1116: about_blank_http_iframe_rfh->GetLastCommittedURL()); Just a note that if we were to screw up the SiteInstance determination logic and end up swapping processes, we'd have a new RFH and this one would be pending deletion or already gone, so you might crash here. I guess it's ok to fail like this (crash if the RFH is gone, and if it isn't, the last committed URL will catch the error). https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1170: // Now that we know the panel is loaded, switch to it. nit: "it" should be "the sidebar pane in the elements panel", and "panel" should be "extension", right? I.e., the previous function waits for the wrong panel, and the only reason to still call it is to wait for the devtools extension itself to load. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1319: // popup_test_page.html's frame should be in extension b's Wrong URL? popup_test_page.html isn't used anywhere https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1320: // process, not in devtools or extension a's process. nit: "extension a" -> |devtools_extension|, "extension b" -> |non_devtools_extension| https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1347: // Install the dynamically-generated extension. I'd add a comment that this extension's panel iframe will point to an extension b URL, just to help quickly grasp how this test works. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1387: content::RenderFrameHost* devtools_extension_b_test_rfh = nit: I'd replace "test" with "frame", as "test" is too ambiguous. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1449: content::RenderFrameHost* devtools_extension_test_rfh = nit: test -> panel_frame or something similar https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1452: // all frames should be in the devtools process, including test.html's frame Did you mean panel instead of test.html? https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1476: GURL devtools_url = GURL("chrome-devtools://devtools/bundled/inspector.html"); Can this use kChromeUIDevToolsURL? https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1483: "devtoolsFrame.setAttribute('src', '" + nit: can shorten to just devtoolsFrame.src = ... https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1490: nit: I'd group the ExecuteScript in one block with manager declaration and manager.Wait...(), and put a blank line after that instead of here, so it's clear that this is all for this one navigation. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1512: EXPECT_EQ("chrome-devtools://devtools", message); If you end up using kChromeUIDevToolsURL above, you could get the origin from that instead of hardcoding it here. https://codereview.chromium.org/2646683002/diff/450001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2646683002/diff/450001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1299: url::Origin origin = url::Origin(dest_url); nit: you can just use url::Origin origin(dest_url) and avoid the extra copy https://codereview.chromium.org/2646683002/diff/450001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1492: url::Origin origin = url::Origin(dest_url); Ditto
The CQ bit was checked by davidsac@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Alex, I have made changes or given feedback for all of your comments. Please take another look. In the mean time, I will see what dgozman has to say. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:529: if (!devtools_page.empty()) { On 2017/03/20 22:52:51, alexmos wrote: > nit: no {} Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:535: GURL http_popup_url = On 2017/03/20 22:52:50, alexmos wrote: > Since you're using it from an <iframe>, this isn't really a popup. Perhaps > rename to http_frame_url? Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:538: dir->WriteFile(FILE_PATH_LITERAL("web_devtools_page.html"), On 2017/03/20 22:52:51, alexmos wrote: > Maybe add a comment before this saying that only one of these devtools pages > will ends up being used, and different tests use |devtools_page| to specify > which one. Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:539: "<html><head></head><body><iframe src='" + On 2017/03/20 22:52:50, alexmos wrote: > nit: no need for <head></head>, here and similarly below Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:557: "chrome.devtools.inspectedWindow.eval('console.log(" On 2017/03/20 22:52:51, alexmos wrote: > nit: fix the indent here and remove the preceding blank line. Same for indent > in createSidebarPane Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:578: dir->WriteFile(FILE_PATH_LITERAL("renavigation_test_page.html"), On 2017/03/20 22:52:50, alexmos wrote: > In the tests, this seems to be used for a lot more than renavigation. Perhaps > use a more generic name, e.g., page_with_three_frames? Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1026: // (https://developer.chrome.com/extensions/devtools). Also tests that data On 2017/03/20 22:52:50, alexmos wrote: > nit: insert "iframes with " before data. Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1027: // URLs and about blank URLs are rendered in the devtools process, unless an On 2017/03/20 22:52:51, alexmos wrote: > nit: add ":" in about:blank Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1028: // OOPIF is navigated to about:blank, in which case it does not end up back in On 2017/03/20 22:52:50, alexmos wrote: > nit: "an OOPIF is navigated" -> "a web OOPIF navigates itself" > (OOPIF does not imply being a web iframe; also, we don't want ambiguity about > who's navigating the frame and inheriting the initiator's origin, which this > test isn't concerned with) Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1052: std::vector<content::RenderFrameHost*> rfhs = On 2017/03/20 22:52:50, alexmos wrote: > Given the complexity of this test, it might help with readability if you include > a short comment that outlines the structure of the whole page? It could include > something like this: > - DevTools > - devtools_page from DevTools extension > - Panel (DevTools extension) > - iframe (DevTools extension) > - about:blank > - data: > - web URL Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1054: EXPECT_EQ(7U, rfhs.size()); On 2017/03/20 22:52:50, alexmos wrote: > Sanity check: it seems for this test, we'll also install a sidebar pane that > points to renavigation_test_page.html, so it will also have the three iframes. > Are we relying on the fact that the sidebar pane isn't shown until you switch to > the elements panel? Correct. They're there, but they don't load. Should I add a comment about it? I feel like it's mentioned in a couple other places already, but redundancy might not hurt. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1056: content::RenderFrameHost* main_devtools_rfh = On 2017/03/20 22:52:50, alexmos wrote: > nit: given that it's used so often, you could just put a using > content::RenderFrameHost on top (this file already does this for RenderViewHost > and a bunch of others, so it'll fit right in), and then drop "content::" from > RenderFrameHost* everywhere. Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1058: content::RenderFrameHost* devtools_extension_devtools_rfh = On 2017/03/20 22:52:50, alexmos wrote: > This is for the "devtools_page" from the manifest, so perhaps the name should > have devtools_page? Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1060: content::RenderFrameHost* devtools_panel_extension_rfh = renamed to devtool_extension_panel_rfh https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1062: content::RenderFrameHost* test_frame_rfh = On 2017/03/20 22:52:50, alexmos wrote: > I'd name this panel_frame_rfh Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1067: content::RenderFrameHost* about_blank_http_iframe_rfh = On 2017/03/20 22:52:51, alexmos wrote: > Why not just web_frame_rfh? Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1086: EXPECT_TRUE(main_devtools_rfh->GetSiteInstance()->GetSiteURL().SchemeIs( On 2017/03/20 22:52:50, alexmos wrote: > nit: you could declare > SiteInstance* devtools_instance = main_devtools_rfh->GetSiteInstance(); > and then use that everywhere below for a bit of extra clarify and to save a > bunch of space. Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1100: about_blank_http_iframe_rfh->GetSiteInstance()->GetSiteURL().host()); On 2017/03/20 22:52:50, alexmos wrote: > I'd add an explicit EXPECT_NE(devtools_instance, > about_blank_http_iframe_rfh->GetSiteInstance()) > (and similarly in all other tests) Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1100: about_blank_http_iframe_rfh->GetSiteInstance()->GetSiteURL().host()); On 2017/03/20 22:52:51, alexmos wrote: > Too bad that DepictFrameTree is internal to content/. It would've been so nice > to use it here instead of all the GetSiteInstance checks. Perhaps we should > consider exposing it to tests outside content/ (certainly not for this CL). Acknowledged. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1102: // Check that if an OOPIF navigates itself to about:blank, it stays an OOPIF. On 2017/03/20 22:52:50, alexmos wrote: > nit: Check that if the web iframe navigates itself to about:blank, it stays in > the web SiteInstance. Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1116: about_blank_http_iframe_rfh->GetLastCommittedURL()); On 2017/03/20 22:52:51, alexmos wrote: > Just a note that if we were to screw up the SiteInstance determination logic and > end up swapping processes, we'd have a new RFH and this one would be pending > deletion or already gone, so you might crash here. I guess it's ok to fail like > this (crash if the RFH is gone, and if it isn't, the last committed URL will > catch the error). Good thing to consider! That is a good point though, I should be more conscious that if code changes and breaks, that my tests actually CATCH the breaking change and fail when necessary. I'm glad we dodged the bullet on it this time, though. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1170: // Now that we know the panel is loaded, switch to it. On 2017/03/20 22:52:51, alexmos wrote: > nit: "it" should be "the sidebar pane in the elements panel", and "panel" should > be "extension", right? I.e., the previous function waits for the wrong panel, > and the only reason to still call it is to wait for the devtools extension > itself to load. Ok, I tried to fix this the best that I could. The purpose of "RunTestFunction(window_, "waitForTestResultsInConsole");" is to make sure that the javascript from the devtools page finishes running and that the placeholders for the panel or sidebarpane exist, so that they can be successfully switched to. In this patch set, they were always loaded together and gave the same signal when they were finished loading. Because I thought that this was a potential race condition, that has been fixed. They are now loaded separately always, and I have adjusted the comments accordingly. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1319: // popup_test_page.html's frame should be in extension b's On 2017/03/20 22:52:51, alexmos wrote: > Wrong URL? popup_test_page.html isn't used anywhere Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1320: // process, not in devtools or extension a's process. On 2017/03/20 22:52:51, alexmos wrote: > nit: "extension a" -> |devtools_extension|, > "extension b" -> |non_devtools_extension| Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1347: // Install the dynamically-generated extension. On 2017/03/20 22:52:50, alexmos wrote: > I'd add a comment that this extension's panel iframe will point to an extension > b URL, just to help quickly grasp how this test works. Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1387: content::RenderFrameHost* devtools_extension_b_test_rfh = On 2017/03/20 22:52:51, alexmos wrote: > nit: I'd replace "test" with "frame", as "test" is too ambiguous. Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1449: content::RenderFrameHost* devtools_extension_test_rfh = On 2017/03/20 22:52:51, alexmos wrote: > nit: test -> panel_frame or something similar Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1452: // all frames should be in the devtools process, including test.html's frame On 2017/03/20 22:52:50, alexmos wrote: > Did you mean panel instead of test.html? I actually meant |simple_test_page.html|. Does that sound ok? https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1476: GURL devtools_url = GURL("chrome-devtools://devtools/bundled/inspector.html"); On 2017/03/20 22:52:51, alexmos wrote: > Can this use kChromeUIDevToolsURL? Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1483: "devtoolsFrame.setAttribute('src', '" + On 2017/03/20 22:52:51, alexmos wrote: > nit: can shorten to just devtoolsFrame.src = ... Done. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1490: On 2017/03/20 22:52:50, alexmos wrote: > nit: I'd group the ExecuteScript in one block with manager declaration and > manager.Wait...(), and put a blank line after that instead of here, so it's > clear that this is all for this one navigation. Done. Good call, I did that in other tests. I don't know why I spaced out on doing it for this one. https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1512: EXPECT_EQ("chrome-devtools://devtools", message); On 2017/03/20 22:52:51, alexmos wrote: > If you end up using kChromeUIDevToolsURL above, you could get the origin from > that instead of hardcoding it here. Done. I had to add a slash, though. https://codereview.chromium.org/2646683002/diff/450001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2646683002/diff/450001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1299: url::Origin origin = url::Origin(dest_url); On 2017/03/20 22:52:51, alexmos wrote: > nit: you can just use url::Origin origin(dest_url) and avoid the extra copy Done. https://codereview.chromium.org/2646683002/diff/450001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1492: url::Origin origin = url::Origin(dest_url); On 2017/03/20 22:52:51, alexmos wrote: > Ditto Done.
davidsac@chromium.org changed reviewers: + dgozman@chromium.org
Hi dgozman, Could you please review the changelist for OWNER approval for Devtools?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nits. Thanks for all the work on this! https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1170: // Now that we know the panel is loaded, switch to it. On 2017/03/23 01:22:35, davidsac wrote: > On 2017/03/20 22:52:51, alexmos wrote: > > nit: "it" should be "the sidebar pane in the elements panel", and "panel" > should > > be "extension", right? I.e., the previous function waits for the wrong panel, > > and the only reason to still call it is to wait for the devtools extension > > itself to load. > > Ok, I tried to fix this the best that I could. The purpose of > "RunTestFunction(window_, "waitForTestResultsInConsole");" is to make sure that > the javascript from the devtools page finishes running and that the placeholders > for the panel or sidebarpane exist, so that they can be successfully switched > to. In this patch set, they were always loaded together and gave the same > signal when they were finished loading. Because I thought that this was a > potential race condition, that has been fixed. They are now loaded separately > always, and I have adjusted the comments accordingly. Yes, I agree with your race condition concerns, and separating them out into two devtools_pages seems much better. Thanks for addressing this! https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1452: // all frames should be in the devtools process, including test.html's frame On 2017/03/23 01:22:35, davidsac wrote: > On 2017/03/20 22:52:50, alexmos wrote: > > Did you mean panel instead of test.html? > > I actually meant |simple_test_page.html|. Does that sound ok? simple_test_page.html doesn't have any frames, so I don't think you want to say "simple_test_page.html's frame". Probably just simple_test_page.html instead? https://codereview.chromium.org/2646683002/diff/470001/chrome/browser/devtool... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/470001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1186: // This is a bit of a hack to switch to the sidebar pane in the elements panel nit: You could probably just remove "This is a bit of a hack" - what you're doing doesn't seem like a hack, it's just switching to the elements panel to so that the sidebar pane that's on there can load. https://codereview.chromium.org/2646683002/diff/470001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1466: // all frames should be in the devtools process, including nit: capitalize All and end with period. https://codereview.chromium.org/2646683002/diff/470001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1500: "devtoolsFrame.src='" + nit: spaces around = https://codereview.chromium.org/2646683002/diff/470001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1522: nit: no blank line
dgozman@chromium.org changed reviewers: + caseq@chromium.org
Over to caseq@ who has more insights into devtools extensions.
Hey Alex, For whatever reason, I thought that I already mailed out these comments. Also, I failed to realize that you had commented on my comments from a previous patchset, so I addressed the one relevant comment there (and accompanying fix) as well. I think there's one comment that still needs your attention, so maybe you could take a look? https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/450001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1452: // all frames should be in the devtools process, including test.html's frame On 2017/03/23 17:15:07, alexmos wrote: > On 2017/03/23 01:22:35, davidsac wrote: > > On 2017/03/20 22:52:50, alexmos wrote: > > > Did you mean panel instead of test.html? > > > > I actually meant |simple_test_page.html|. Does that sound ok? > > simple_test_page.html doesn't have any frames, so I don't think you want to say > "simple_test_page.html's frame". Probably just simple_test_page.html instead? Done. https://codereview.chromium.org/2646683002/diff/470001/chrome/browser/devtool... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/470001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1186: // This is a bit of a hack to switch to the sidebar pane in the elements panel On 2017/03/23 17:15:07, alexmos wrote: > nit: You could probably just remove "This is a bit of a hack" - what you're > doing doesn't seem like a hack, it's just switching to the elements panel to so > that the sidebar pane that's on there can load. Not exactly. I'm actually using it to switch from the default sidebarpane to the extension's sidebarpane, which actually wouldn't load otherwise. The reason it is a bit of a hack is because I looked into the method |SwitchToPanel| and found out that the actual implementation just switches to the name of whatever view you give it, regardless of whether it is a panel. It worked out pretty well in this case! What are your thoughts? keep the wording? https://codereview.chromium.org/2646683002/diff/470001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1466: // all frames should be in the devtools process, including On 2017/03/23 17:15:07, alexmos wrote: > nit: capitalize All and end with period. Done. https://codereview.chromium.org/2646683002/diff/470001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1500: "devtoolsFrame.src='" + On 2017/03/23 17:15:07, alexmos wrote: > nit: spaces around = Done. https://codereview.chromium.org/2646683002/diff/470001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1522: On 2017/03/23 17:15:07, alexmos wrote: > nit: no blank line Done.
This is fine if you look at it out of context, but I think we're going in the wrong direction with this patch. The code that you're fixing is a work-around for crbug.com/564216. I think we rather should fix the root cause, the extension iframes _should_ rather be loaded in different processes. There's nothing in the extension API that requires extension iframes to share process with DevTools, the API is actually implemented on top of (window|port).postMessage(). This should not be fundamentally difficult, perhaps the only thing that prevents this is the current implementation of DevToolsHost::setInjectedScriptForOrigin() that we use to inject the DevTools extension API into the extension frame, which is entirely in the renderer and hence only works within the boundaries of the renderer. This should rather plumbed to the browser (Dmitry suggests watching for the matching URL in DevToolsUIBindings::FrontendWebContentsObserver::ReadyToCommitNavigation and injecting the API script from there).
On 2017/03/24 23:18:29, caseq wrote: > This is fine if you look at it out of context, but I think we're going in the > wrong direction with this patch. The code that you're fixing is a work-around > for crbug.com/564216. I think we rather should fix the root cause, the extension > iframes _should_ rather be loaded in different processes. There's nothing in the > extension API that requires extension iframes to share process with DevTools, > the API is actually implemented on top of (window|port).postMessage(). > > This should not be fundamentally difficult, perhaps the only thing that prevents > this is the current implementation of DevToolsHost::setInjectedScriptForOrigin() > that we use to inject the DevTools extension API into the extension frame, which > is entirely in the renderer and hence only works within the boundaries of the > renderer. This should rather plumbed to the browser (Dmitry suggests watching > for the matching URL in > DevToolsUIBindings::FrontendWebContentsObserver::ReadyToCommitNavigation and > injecting the API script from there). Tantalizing! AFAIK the site-isolation team had been operating under the assumption that that devtools extensions needed to run in the devtools process due to some API. It's exciting to learn that we can do even better. If we assume we'll do that next, I think it might still make sense to adopt this patch anyway as a stepping stone. This is a small tweak to the process policy, and includes good tests that will be helpful when oopififying the extension iframes too; and it moves us in a good incremental direction security-wise. It's also ready now. WDYT? Let's defintiely follow up with a VC to discuss your idea and scope it out. This is one of the very few exceptions to the 'extensions frames are loaded in extension processes' rule, so I see huge benefits to making this happen.
DevTools should not be special in the context of running extensions out of process. We seem to be adding more "devtools" mentions into the content's render_frame_host_manager.cc as a part of this patch, which sounds like layering problem and introducing technical dept. The fact that devtools window is webcontents itself should only be exposed on the embedder level. What's the core of the issue? Is it that OpenURLFromTab in the delegate that devtools intercepts?
On 2017/03/27 20:03:18, pfeldman wrote: > DevTools should not be special in the context of running extensions out of > process. We seem to be adding more "devtools" mentions into the content's > render_frame_host_manager.cc as a part of this patch, which sounds like layering > problem and introducing technical dept. The fact that devtools window is > webcontents itself should only be exposed on the embedder level. > > What's the core of the issue? Is it that OpenURLFromTab in the delegate that > devtools intercepts? The goal of this patch is to boot HTTP iframes out of the devtools process (leaving only the extension frames). It's not really adding any new category of "devtools" mentions, it's just tweaking an existing one so that it applies to fewer frames. I see this as fixing technical debt, not introducing it. We (on the site isolation team) don't really have a handle on what it would take to run devtools extensions out of process. Until earlier today, we were operating under the assumption that it simply wasn't possible due to the API, which required extension code to run in the devtools process. Maybe that was a misreading/misrecollection of https://bugs.chromium.org/p/chromium/issues/detail?id=564216#c2 ? We need help from the devtools experts here to scope out the issues.
On 2017/03/27 20:24:44, ncarter wrote: > On 2017/03/27 20:03:18, pfeldman wrote: > > DevTools should not be special in the context of running extensions out of > > process. We seem to be adding more "devtools" mentions into the content's > > render_frame_host_manager.cc as a part of this patch, which sounds like > layering > > problem and introducing technical dept. The fact that devtools window is > > webcontents itself should only be exposed on the embedder level. > > > > What's the core of the issue? Is it that OpenURLFromTab in the delegate that > > devtools intercepts? > > The goal of this patch is to boot HTTP iframes out of the devtools process > (leaving only the extension frames). It's not really adding any new > category of "devtools" mentions, it's just tweaking an existing one so that > it applies to fewer frames. I see this as fixing technical debt, not > introducing it. > > We (on the site isolation team) don't really have a handle on what > it would take to run devtools extensions out of process. Until earlier today, > we were operating under the assumption that it simply wasn't possible due > to the API, which required extension code to run in the devtools process. > Maybe that was a misreading/misrecollection of > https://bugs.chromium.org/p/chromium/issues/detail?id=564216#c2 ? We need help > from the devtools experts here to scope out the issues. I'll also note that --isolate-extensions has shipped to 100% of Stable, and the http iframes in DevTools extensions staying in-process is the last large hole in our security model. This CL is fixing that security problem. I totally agree that we shouldn't be introducing new dependencies on devtools in RFHM, but we are really taking a hole that's already there for devtools and just making it smaller, not adding a new hole. I'm actually very excited if we can go one step further and get devtools extensions also running out-of-process, and we should definitely pursue that. We didn't consider that as a possibility (https://crbug.com/564216#c2) before caseq@ mentioned that the API is done on top of postMessage, which should indeed make this possible. I agree with Nick that we need to chat to understand more about what it'll take though. I don't know any of the devtools bindings code that caseq@ pointed to, and the devtools team might be in a better position to fix that than the site isolation team. I gave the whole approach a quick try with one of the DevTools extensions tests, and it doesn't look like we hit the same problem in OpenURLFromTab, but instead we time out somewhere on the TestSuite.switchToPanel path. Most likely, caseq@ is right that we need to grant bindings to the devtools extension process. Also, just for context, this is Andrew's last week with our team, so he won't be able to work out the OOP devtools extensions approach, and it might be a while before the rest of us have cycles for that. I think we're better off with this CL in the short term than nothing at all. For the new approach, the tests here will be easy to tweak, and the devtools check in RFHM will be easy to remove.
The CQ bit was checked by davidsac@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I added TODOs to remove the logic allowing devtools extension pages to remain inside the devtools process to reference the new bug, https://crbug.com/706169. We were a little unsure after the meeting though. Are these TODOs acceptable, or is extra plumbing with chrome_content_browser_client necessary? Alex is currently investigating this and it doesn't seem difficult so far, but at the same time, it is just a temporary fix.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm https://codereview.chromium.org/2646683002/diff/530001/chrome/browser/devtool... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/530001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:587: GURL about_blank_url = GURL(url::kAboutBlankURL); just inline about:blank in the string? https://codereview.chromium.org/2646683002/diff/530001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:589: GURL data_url = GURL("data:text/html,foo"); ditto. https://codereview.chromium.org/2646683002/diff/530001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:598: const Extension* extension = LoadExtensionFromPath(dir->UnpackedPath()); nit: just "return LoadExtensionFromPath(...)" https://codereview.chromium.org/2646683002/diff/530001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1122: "location.href='" + about_blank_url.spec() + "';"; hardcode about:blank
https://codereview.chromium.org/2646683002/diff/530001/chrome/browser/devtool... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2646683002/diff/530001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:587: GURL about_blank_url = GURL(url::kAboutBlankURL); On 2017/03/30 00:03:37, caseq wrote: > just inline about:blank in the string? Done. https://codereview.chromium.org/2646683002/diff/530001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:589: GURL data_url = GURL("data:text/html,foo"); On 2017/03/30 00:03:37, caseq wrote: > ditto. Done. https://codereview.chromium.org/2646683002/diff/530001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:598: const Extension* extension = LoadExtensionFromPath(dir->UnpackedPath()); On 2017/03/30 00:03:37, caseq wrote: > nit: just "return LoadExtensionFromPath(...)" Done. https://codereview.chromium.org/2646683002/diff/530001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1122: "location.href='" + about_blank_url.spec() + "';"; On 2017/03/30 00:03:37, caseq wrote: > hardcode about:blank Done.
The CQ bit was checked by davidsac@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org, alexmos@chromium.org, caseq@chromium.org Link to the patchset: https://codereview.chromium.org/2646683002/#ps550001 (title: "small URL fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm
The CQ bit was checked by davidsac@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 550001, "attempt_start_ts": 1490906100022370,
"parent_rev": "2b6c8430a8b66ce84ec2b4f6196c5332f407ea2a", "commit_rev":
"88a1aa7a6141160af77f9829f64b17c3c6cda5ee"}
Message was sent while issue was closed.
Description was changed from ========== Implement OOPIFs within Devtools Extensions Fix IFrames existing within Devtools to be implemented outside of the devtools processes when necessary. Currently, some IFrames are rendered within the same process as devtools when they shouldn't be. The discussion of which IFrames should be allowed in devtools and when ended up becoming fairly complicated. See the attached DESIGN DOC below. DESIGN_DOC=https://docs.google.com/a/chromium.org/document/d/1Yxs0DMjoWwiPB5oFh4TRifim7FsEmMdbnd7FGWpReCU/edit?usp=sharing BUG=570483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Implement OOPIFs within Devtools Extensions Fix IFrames existing within Devtools to be implemented outside of the devtools processes when necessary. Currently, some IFrames are rendered within the same process as devtools when they shouldn't be. The discussion of which IFrames should be allowed in devtools and when ended up becoming fairly complicated. See the attached DESIGN DOC below. DESIGN_DOC=https://docs.google.com/a/chromium.org/document/d/1Yxs0DMjoWwiPB5oFh4TRifim7FsEmMdbnd7FGWpReCU/edit?usp=sharing BUG=570483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2646683002 Cr-Commit-Position: refs/heads/master@{#460876} Committed: https://chromium.googlesource.com/chromium/src/+/88a1aa7a6141160af77f9829f64b... ==========
Message was sent while issue was closed.
Committed patchset #29 (id:550001) as https://chromium.googlesource.com/chromium/src/+/88a1aa7a6141160af77f9829f64b...
Message was sent while issue was closed.
Description was changed from ========== Implement OOPIFs within Devtools Extensions Fix IFrames existing within Devtools to be implemented outside of the devtools processes when necessary. Currently, some IFrames are rendered within the same process as devtools when they shouldn't be. The discussion of which IFrames should be allowed in devtools and when ended up becoming fairly complicated. See the attached DESIGN DOC below. DESIGN_DOC=https://docs.google.com/a/chromium.org/document/d/1Yxs0DMjoWwiPB5oFh4TRifim7FsEmMdbnd7FGWpReCU/edit?usp=sharing BUG=570483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2646683002 Cr-Commit-Position: refs/heads/master@{#460876} Committed: https://chromium.googlesource.com/chromium/src/+/88a1aa7a6141160af77f9829f64b... ========== to ========== Implement OOPIFs within Devtools Extensions Fix IFrames existing within Devtools to be implemented outside of the devtools processes when necessary. Currently, some IFrames are rendered within the same process as devtools when they shouldn't be. The discussion of which IFrames should be allowed in devtools and when ended up becoming fairly complicated. See the attached DESIGN DOC below. This involved creating numerous chrome extensions for manually testing, debugging, and helping to understand how devtools extensions work. They can be found at http://dudeguy409.github.io/Chrome%20Devtools%20Extension%20Tests/index.html DESIGN_DOC=https://docs.google.com/a/chromium.org/document/d/1Yxs0DMjoWwiPB5oFh4TRifim7FsEmMdbnd7FGWpReCU/edit?usp=sharing BUG=570483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2646683002 Cr-Commit-Position: refs/heads/master@{#460876} Committed: https://chromium.googlesource.com/chromium/src/+/88a1aa7a6141160af77f9829f64b... ==========
Message was sent while issue was closed.
Description was changed from ========== Implement OOPIFs within Devtools Extensions Fix IFrames existing within Devtools to be implemented outside of the devtools processes when necessary. Currently, some IFrames are rendered within the same process as devtools when they shouldn't be. The discussion of which IFrames should be allowed in devtools and when ended up becoming fairly complicated. See the attached DESIGN DOC below. This involved creating numerous chrome extensions for manually testing, debugging, and helping to understand how devtools extensions work. They can be found at http://dudeguy409.github.io/Chrome%20Devtools%20Extension%20Tests/index.html DESIGN_DOC=https://docs.google.com/a/chromium.org/document/d/1Yxs0DMjoWwiPB5oFh4TRifim7FsEmMdbnd7FGWpReCU/edit?usp=sharing BUG=570483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2646683002 Cr-Commit-Position: refs/heads/master@{#460876} Committed: https://chromium.googlesource.com/chromium/src/+/88a1aa7a6141160af77f9829f64b... ========== to ========== Implement OOPIFs within Devtools Extensions Fix IFrames existing within Devtools to be implemented outside of the devtools processes when necessary. Currently, some IFrames are rendered within the same process as devtools when they shouldn't be. The discussion of which IFrames should be allowed in devtools and when ended up becoming fairly complicated. See the attached DESIGN DOC below. This involved creating numerous chrome extensions for manually testing, debugging, and helping to understand how devtools extensions work. They can be found at http://dudeguy409.github.io/Chrome_Devtools_Extension_Tests/index.html DESIGN_DOC=https://docs.google.com/a/chromium.org/document/d/1Yxs0DMjoWwiPB5oFh4TRifim7FsEmMdbnd7FGWpReCU/edit?usp=sharing BUG=570483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2646683002 Cr-Commit-Position: refs/heads/master@{#460876} Committed: https://chromium.googlesource.com/chromium/src/+/88a1aa7a6141160af77f9829f64b... ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
