Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2538353002:
fix external protocol handling for OOPIFs (Closed)
|
Created:
4 years ago by davidsac (gone - try alexmos) Modified:
4 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org, tburkard+watch_chromium.org, Randy Smith (Not in Mondays), gavinp+prer_chromium.org, loading-reviews_chromium.org, mmenke, site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionfix external protocol handling for OOPIFs
Replace using the wrong render_process_id for launching external protocol
requests in chrome_resource_dispatcher_host_delegate.cc.
Previously, handlers for external protocols like 'mailto:' weren't working for
OOPIFs because the render_process_id used to retrieve the content::WebContents
object was incorrect.
BUG=668289
TEST=See bug for repro steps
Committed: https://crrev.com/1fe2ac646c67872405598ad551dc4b2b430cfce4
Cr-Commit-Position: refs/heads/master@{#439654}
Patch Set 1 : add fix, test and refactor for testing #Patch Set 2 : rebase #
Total comments: 44
Patch Set 3 : minor implementation fixes, clean up comments #Patch Set 4 : more minor fixes #Patch Set 5 : fix memory leak #Patch Set 6 : memory leak actual fix #Patch Set 7 : add log statements to investigate seg faults #Patch Set 8 : Rebase #Patch Set 9 : add nullcheck for runner #Patch Set 10 : add check if runner is necessary #
Total comments: 34
Patch Set 11 : add log statements for ChromeOS test #Patch Set 12 : minor code cleanup #Patch Set 13 : added all the logs statements #Patch Set 14 : remove log statements #Patch Set 15 : disable test in ChromeOS #
Total comments: 10
Patch Set 16 : more small cleanups #
Total comments: 6
Patch Set 17 : add comment #Patch Set 18 : add bug reference for TODO #
Total comments: 13
Patch Set 19 : remove unnecessary include #Messages
Total messages: 94 (65 generated)
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...
davidsac@chromium.org changed reviewers: + alexmos@chromium.org
Hi Alex, Would you mind taking a look at this first draft for fixing the external protocol handler?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
I am considering fixing tab_contents_id with a separate CL.
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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
[+site-isolation-reviews] Thanks Andrew, this is a good start! Some initial comments below, and also looks like you need to deal with a Mac compile failure on the trybots. Also, a couple of nits in the description: - This CL is really about fixing external protocol handling for OOPIFs, so it'd be good if the CL title reflects that. - No need to mention Delegate; this is an implementation detail of your test. (Also, if you did need to mention it, you'd want to use ExternalProtocolHandler::Delegate, which refers to the specific delegate class that you needed to override.) - Would be nice to include a short summary of what was wrong before (Handlers for external protocols such as mailto: were broken when invoked from OOPIFs, because we tried to resolve the active RenderViewHost with a wrong process ID (OOPIF's process ID and not the main frame's), and how this is handled after your CL. - Capitalize all sentences. - It's assumed that all CLs with fixes like these will have a test, so no need to call it out explicitly in the title/description. Instead, if you wanted, you could add a separate line under BUG, TEST=NameOfYourTest, which is the convention to describe the tests or testing strategy. - "This should not alter the functionality of existing code." - this isn't really true, right? Existing code used to be broken for OOPIFs, and you fixed it. :) https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... File chrome/browser/chrome_site_per_process_browsertest.cc (right): https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:12: #include "chrome/browser/external_protocol/external_protocol_handler_unittest.cc" Why do you need this? Is this for FakeExternalProtocolHandlerWorker? You can only include .h files, so this won't work, but see my note below about avoiding this. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:369: class RunsAtSomePointExternalProtocolHandlerDelegate Maybe name this MailtoExternalProtocolHandlerDelegate, which is a bit less vague about what it's for? Also, let's put a quick comment before the declaration about what this class is for. I.e., it's a helper class used to wait for a mailto: external protocol request and verify that it succeeded. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:372: bool has_triggered_external_protocol = false; Usually, this would go under private:, and the name would have an underscore after it (has_triggered_external_protocol_). Then, you'd expose this to callers by a new public function has_triggered_external_protocol(), which just returns the private field. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:378: scoped_refptr<shell_integration::DefaultProtocolClientWorker> Let's separate out individual functions with blank lines to make this easier to read. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:382: return new FakeExternalProtocolHandlerWorker( Instead of this, can you just return "new shell_integration::DefaultProtocolClientWorker(callback, protocol)", which is what CreateShellWorker in external_protocol_handler.cc does when there's no delegate? https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:395: has_triggered_external_protocol = true; Perhaps we should also save the |url| and |web_contents| similarly to this flag, and have the test verify that they have expected values? https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:400: IN_PROC_BROWSER_TEST_F(ChromeSitePerProcessTest, It's a good idea to describe the test in a comment before the declaration, and include a bug reference so it's easy to get more context. See other tests here or site_per_process_browsertest.cc for examples. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:401: ExternalProtocolCrossSiteNavigation) { I think the test name should convey that an external protocol is invoked from a cross-process subframe (cross-site navigation doesn't imply OOPIFs). So, something like LaunchExternalProtocolFromSubframe. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:402: RunsAtSomePointExternalProtocolHandlerDelegate* delegate = No need to use new/delete; this can just be a stack variable. I.e., "RunsAtSomePointExternalProtocolHandlerDelegate delegate;" https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:408: // host to be created for the WebContents. I don't think these details are important to the test. It's fine to just say "Start at a simple a.com page" or omit this. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:410: "a.com", "//cross_site_iframe_factory.html?a()")); No need to use cross_site_iframe_factory here, since you don't need to construct any hierarchy for the first navigation. In cases like this we often use simpler/shorter files, such as /title1.html. Also, no need for double /. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:413: // works??? Yes, this is a known issue that I've been hoping to fix eventually as part of https://crbug.com/425335. No need to include this TODO. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:417: // external protocol request. I'd make this comment more about why you couldn't just start the test with this navigation. I.e., let's explain that we didn't start at this URL because we want to navigate the main frame to do a cross-process transfer, which will avoid a situation where the OOPIF's swapped-out RenderViewHost and the main frame's active RenderViewHost get the same routing IDs, because that masks the bug. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:419: embedded_test_server()->GetURL("c.com", "/auto_mailto_main_frame.html")); You could avoid adding this new file by using an existing test file with an iframe, such as /chrome/test/data/iframe.html. I.e., you can navigate to GetURL("b.com", "/iframe.html"), and then navigate the iframe to your c.com mailto URL with NavigateIframeToURL (see OriginReplicationAllowsAccessToStorage in this file for an example). https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:422: // works??? Same comment as for NavigateToURL above. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:427: } Since SetExternalProtocolHandlerDelegateForTesting sets a global flag, we want to ensure other tests that run after this won't be affected by it, so how about clearing it at the end the test with SetExternalProtocolHandlerDelegateForTesting(nullptr)? https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/external... File chrome/browser/external_protocol/external_protocol_handler.cc (left): https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/external... chrome/browser/external_protocol/external_protocol_handler.cc:109: No need to remove the blank line, here and in a few places below. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/external... File chrome/browser/external_protocol/external_protocol_handler.h (right): https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/external... chrome/browser/external_protocol/external_protocol_handler.h:15: nit: no blank line https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/external... chrome/browser/external_protocol/external_protocol_handler.h:77: // "Launch Application" in an ExternalProtocolDialog.) It is assumed that the Side note: it would probably be good to (manually) check that the external protocol confirmation dialog that this comment talks about also works when invoked from inside OOPIFs. This is presumably the dialog that that asks something like "are you sure you want to launch this external application?" We'd need to find a scheme that triggers this. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/loader/c... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/loader/c... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:220: // TODO(davidsac): get rid of useless_render_process_id parameter??? Yes, let's remove this argument. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/loader/c... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:221: // TODO(davidsac): replace occurences of tab_contents_id with This comment is for a function that doesn't actually use tab_contents_id, so I'd remove it from here. And by the way, doing this rename is indeed something we should do, and it's fine to do it in a separate CL. https://codereview.chromium.org/2538353002/diff/20001/chrome/test/data/auto_m... File chrome/test/data/auto_mailto_subframe.html (right): https://codereview.chromium.org/2538353002/diff/20001/chrome/test/data/auto_m... chrome/test/data/auto_mailto_subframe.html:4: D - This subframe automatically opens a mailto: link. What does the D refer to? The site from which this is opened? It's probably better to leave this generic, as future tests might load this file from other sites or reuse it differently. Similarly, the file name doesn't need to be so specific - just page_with_mailto.html would work.
Description was changed from ========== add test and fix for external protocol handling. replace using the wrong routing_id for launching external protocol requests in chrome_resource_dispatcher_host_delegate.cc. refactor external_protocol_handler.cc to allow for testing with a subclass of Delegate. add a subclass of Delegate to use in tests. add a test for external protocols using the subclass of delegate. This should not alter the functionality of existing code. BUG=668289 ========== to ========== replace using the wrong routing_id for launching external protocol requests in chrome_resource_dispatcher_host_delegate.cc. BUG=668289 ==========
Description was changed from ========== replace using the wrong routing_id for launching external protocol requests in chrome_resource_dispatcher_host_delegate.cc. BUG=668289 ========== to ========== Replace using the wrong routing_id for launching external protocol requests in chrome_resource_dispatcher_host_delegate.cc. BUG=668289 ==========
Description was changed from ========== Replace using the wrong routing_id for launching external protocol requests in chrome_resource_dispatcher_host_delegate.cc. BUG=668289 ========== to ========== Replace using the wrong routing_id for launching external protocol requests in chrome_resource_dispatcher_host_delegate.cc. Previously, handlers for external protocols like 'mailto:' weren't working for OOPIFs because the render_process_id used to retrieve the content::WebContents object was incorrect. BUG=668289 ==========
Description was changed from ========== Replace using the wrong routing_id for launching external protocol requests in chrome_resource_dispatcher_host_delegate.cc. Previously, handlers for external protocols like 'mailto:' weren't working for OOPIFs because the render_process_id used to retrieve the content::WebContents object was incorrect. BUG=668289 ========== to ========== Replace using the wrong render_process_id for launching external protocol requests in chrome_resource_dispatcher_host_delegate.cc. Previously, handlers for external protocols like 'mailto:' weren't working for OOPIFs because the render_process_id used to retrieve the content::WebContents object was incorrect. BUG=668289 ==========
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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The requested fixes have been added.
The requested fixes have been added. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... File chrome/browser/chrome_site_per_process_browsertest.cc (right): https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:12: #include "chrome/browser/external_protocol/external_protocol_handler_unittest.cc" On 2016/12/01 19:21:13, alexmos wrote: > Why do you need this? Is this for FakeExternalProtocolHandlerWorker? You can > only include .h files, so this won't work, but see my note below about avoiding > this. Done. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:369: class RunsAtSomePointExternalProtocolHandlerDelegate On 2016/12/01 19:21:13, alexmos wrote: > Maybe name this MailtoExternalProtocolHandlerDelegate, which is a bit less vague > about what it's for? Also, let's put a quick comment before the declaration > about what this class is for. I.e., it's a helper class used to wait for a > mailto: external protocol request and verify that it succeeded. Done. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:372: bool has_triggered_external_protocol = false; On 2016/12/01 19:21:13, alexmos wrote: > Usually, this would go under private:, and the name would have an underscore > after it (has_triggered_external_protocol_). Then, you'd expose this to callers > by a new public function has_triggered_external_protocol(), which just returns > the private field. Done. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:378: scoped_refptr<shell_integration::DefaultProtocolClientWorker> On 2016/12/01 19:21:13, alexmos wrote: > Let's separate out individual functions with blank lines to make this easier to > read. Done. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:382: return new FakeExternalProtocolHandlerWorker( On 2016/12/01 19:21:13, alexmos wrote: > Instead of this, can you just return "new > shell_integration::DefaultProtocolClientWorker(callback, protocol)", which is > what CreateShellWorker in external_protocol_handler.cc does when there's no > delegate? Done. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:395: has_triggered_external_protocol = true; On 2016/12/01 19:21:14, alexmos wrote: > Perhaps we should also save the |url| and |web_contents| similarly to this flag, > and have the test verify that they have expected values? Done. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:400: IN_PROC_BROWSER_TEST_F(ChromeSitePerProcessTest, On 2016/12/01 19:21:14, alexmos wrote: > It's a good idea to describe the test in a comment before the declaration, and > include a bug reference so it's easy to get more context. See other tests here > or site_per_process_browsertest.cc for examples. Done. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:401: ExternalProtocolCrossSiteNavigation) { On 2016/12/01 19:21:13, alexmos wrote: > I think the test name should convey that an external protocol is invoked from a > cross-process subframe (cross-site navigation doesn't imply OOPIFs). So, > something like LaunchExternalProtocolFromSubframe. Done. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:402: RunsAtSomePointExternalProtocolHandlerDelegate* delegate = On 2016/12/01 19:21:13, alexmos wrote: > No need to use new/delete; this can just be a stack variable. I.e., > "RunsAtSomePointExternalProtocolHandlerDelegate delegate;" Done. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:408: // host to be created for the WebContents. On 2016/12/01 19:21:13, alexmos wrote: > I don't think these details are important to the test. It's fine to just say > "Start at a simple a.com page" or omit this. Done. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:410: "a.com", "//cross_site_iframe_factory.html?a()")); On 2016/12/01 19:21:13, alexmos wrote: > No need to use cross_site_iframe_factory here, since you don't need to construct > any hierarchy for the first navigation. In cases like this we often use > simpler/shorter files, such as /title1.html. > > Also, no need for double /. Done. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:413: // works??? On 2016/12/01 19:21:14, alexmos wrote: > Yes, this is a known issue that I've been hoping to fix eventually as part of > https://crbug.com/425335. No need to include this TODO. Done. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:417: // external protocol request. On 2016/12/01 19:21:13, alexmos wrote: > I'd make this comment more about why you couldn't just start the test with this > navigation. I.e., let's explain that we didn't start at this URL because we > want to navigate the main frame to do a cross-process transfer, which will avoid > a situation where the OOPIF's swapped-out RenderViewHost and the main frame's > active RenderViewHost get the same routing IDs, because that masks the bug. Done. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:419: embedded_test_server()->GetURL("c.com", "/auto_mailto_main_frame.html")); On 2016/12/01 19:21:13, alexmos wrote: > You could avoid adding this new file by using an existing test file with an > iframe, such as /chrome/test/data/iframe.html. I.e., you can navigate to > GetURL("b.com", "/iframe.html"), and then navigate the iframe to your c.com > mailto URL with NavigateIframeToURL (see OriginReplicationAllowsAccessToStorage > in this file for an example). Done. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:422: // works??? On 2016/12/01 19:21:13, alexmos wrote: > Same comment as for NavigateToURL above. Done. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:427: } On 2016/12/01 19:21:13, alexmos wrote: > Since SetExternalProtocolHandlerDelegateForTesting sets a global flag, we want > to ensure other tests that run after this won't be affected by it, so how about > clearing it at the end the test with > SetExternalProtocolHandlerDelegateForTesting(nullptr)? Done. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/external... File chrome/browser/external_protocol/external_protocol_handler.cc (left): https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/external... chrome/browser/external_protocol/external_protocol_handler.cc:109: On 2016/12/01 19:21:14, alexmos wrote: > No need to remove the blank line, here and in a few places below. Done. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/external... File chrome/browser/external_protocol/external_protocol_handler.h (right): https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/external... chrome/browser/external_protocol/external_protocol_handler.h:15: On 2016/12/01 19:21:14, alexmos wrote: > nit: no blank line Done. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/external... chrome/browser/external_protocol/external_protocol_handler.h:77: // "Launch Application" in an ExternalProtocolDialog.) It is assumed that the On 2016/12/01 19:21:14, alexmos wrote: > Side note: it would probably be good to (manually) check that the external > protocol confirmation dialog that this comment talks about also works when > invoked from inside OOPIFs. This is presumably the dialog that that asks > something like "are you sure you want to launch this external application?" > We'd need to find a scheme that triggers this. Done. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/loader/c... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/loader/c... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:220: // TODO(davidsac): get rid of useless_render_process_id parameter??? On 2016/12/01 19:21:14, alexmos wrote: > Yes, let's remove this argument. Done. https://codereview.chromium.org/2538353002/diff/20001/chrome/browser/loader/c... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:221: // TODO(davidsac): replace occurences of tab_contents_id with On 2016/12/01 19:21:14, alexmos wrote: > This comment is for a function that doesn't actually use tab_contents_id, so I'd > remove it from here. > > And by the way, doing this rename is indeed something we should do, and it's > fine to do it in a separate CL. Done. https://codereview.chromium.org/2538353002/diff/20001/chrome/test/data/auto_m... File chrome/test/data/auto_mailto_subframe.html (right): https://codereview.chromium.org/2538353002/diff/20001/chrome/test/data/auto_m... chrome/test/data/auto_mailto_subframe.html:4: D - This subframe automatically opens a mailto: link. On 2016/12/01 19:21:14, alexmos wrote: > What does the D refer to? The site from which this is opened? It's probably > better to leave this generic, as future tests might load this file from other > sites or reuse it differently. Similarly, the file name doesn't need to be so > specific - just page_with_mailto.html would work. Done.
Thanks, looks much better! Second round of comments, which are mostly minor, and let's investigate the remaining ChromeOS failure. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... File chrome/browser/chrome_site_per_process_browsertest.cc (right): https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:6: #include "base/debug/stack_trace.h" Don't forget to remove. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:369: // a helper class to verify that a "mailto:" external protocol nit: capitalize A and end sentence with period. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:370: // request succeeds during unit tests This is not a unit test but rather a browser test, so let's just drop "during unit tests". https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:378: std::string getURL() { return url_path_; } For simple accessors like this, we typically don't use the "get" prefix and use all lowercase and underscores. See https://google.github.io/styleguide/cppguide.html#Function_Names (For more complex functions that are not simple getters/setters, you'd use Camel Case) Also, I'd clarify in the name which url this is. I.e., name this external_protocol_url(). https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:378: std::string getURL() { return url_path_; } I'd just store the full GURL and return this from here, since this is what your method name implies. Then your call site could validate the full URL, which is more useful as it also validates the scheme. (See also my comment there.) https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:380: content::WebContents* getWebContents() { return web_contents_; } Similarly, you can rename this to web_contents(). https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:407: LOG(INFO) << "message_loop_runner_ is not null!"; Don't forget to remove the LOGs once you're done debugging. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:413: if (web_contents) { nit: no need for { here and below. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:413: if (web_contents) { I don't think you need to check if (web_contents) anymore. If the web_contents is null, your test will catch it when comparing the WebContents. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:435: std::string url_path_ = ""; no need for this assignment, std::string is going to be empty by default. But also see my earlier comment; I think you can just make this "GURL url_;" https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:455: nit: no blank line. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:467: nit: no blank line (this whole block is about navigating the subframe) https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:476: EXPECT_EQ(delegate.getURL(), "mail@example.org"); Let's also validate that the scheme of the URL is in fact "mailto". You could do that just by checking the whole URL against GURL("mailto:mail@example.org"). https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/externa... File chrome/browser/external_protocol/external_protocol_handler.cc (left): https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/externa... chrome/browser/external_protocol/external_protocol_handler.cc:8: Did you remove the blank line here by accident? https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/externa... File chrome/browser/external_protocol/external_protocol_handler.h (right): https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/externa... chrome/browser/external_protocol/external_protocol_handler.h:112: int render_process_host_id, I wonder if we could likewise refactor this to take in a WebContents directly. But we can save that for a followup CL, to avoid this one getting too big. (Note that we can't refactor ExternalProtocolHandler::LaunchUrlWithDelegate to take in a WebContents directly, as it posts a task to the FILE thread and then gets back a callback, and the WebContents could be gone by the time of the callback.) https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/loader/... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:230: int render_process_id = The else {} below is the only place left where it's used, and the corresponding routing ID is retrieved directly. Perhaps it'll be cleaner to not bother with this variable and replace the render_process_id with the direct call to web_contents->GetRenderViewHost()->GetProcess()->GetID(), similarly to GetRoutingID. https://codereview.chromium.org/2538353002/diff/180001/chrome/test/data/page_... File chrome/test/data/page_with_mailto.html (right): https://codereview.chromium.org/2538353002/diff/180001/chrome/test/data/page_... chrome/test/data/page_with_mailto.html:3: <body onload="javascript: window.location.href='mailto:mail@example.org';"> no need for "javascript:", I think.
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...
All requested fixes have been made. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... File chrome/browser/chrome_site_per_process_browsertest.cc (right): https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:6: #include "base/debug/stack_trace.h" On 2016/12/12 21:11:39, alexmos wrote: > Don't forget to remove. Done. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:369: // a helper class to verify that a "mailto:" external protocol On 2016/12/12 21:11:39, alexmos wrote: > nit: capitalize A and end sentence with period. Done. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:370: // request succeeds during unit tests On 2016/12/12 21:11:40, alexmos wrote: > This is not a unit test but rather a browser test, so let's just drop "during > unit tests". Done. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:378: std::string getURL() { return url_path_; } On 2016/12/12 21:11:40, alexmos wrote: > For simple accessors like this, we typically don't use the "get" prefix and use > all lowercase and underscores. See > https://google.github.io/styleguide/cppguide.html#Function_Names > (For more complex functions that are not simple getters/setters, you'd use Camel > Case) > > Also, I'd clarify in the name which url this is. I.e., name this > external_protocol_url(). Done. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:378: std::string getURL() { return url_path_; } On 2016/12/12 21:11:40, alexmos wrote: > I'd just store the full GURL and return this from here, since this is what your > method name implies. Then your call site could validate the full URL, which is > more useful as it also validates the scheme. (See also my comment there.) Done. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:380: content::WebContents* getWebContents() { return web_contents_; } On 2016/12/12 21:11:40, alexmos wrote: > Similarly, you can rename this to web_contents(). Done. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:407: LOG(INFO) << "message_loop_runner_ is not null!"; On 2016/12/12 21:11:39, alexmos wrote: > Don't forget to remove the LOGs once you're done debugging. Done. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:413: if (web_contents) { On 2016/12/12 21:11:39, alexmos wrote: > nit: no need for { here and below. Done. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:413: if (web_contents) { On 2016/12/12 21:11:40, alexmos wrote: > I don't think you need to check if (web_contents) anymore. If the web_contents > is null, your test will catch it when comparing the WebContents. Done. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:435: std::string url_path_ = ""; On 2016/12/12 21:11:40, alexmos wrote: > no need for this assignment, std::string is going to be empty by default. But > also see my earlier comment; I think you can just make this "GURL url_;" Done. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:455: On 2016/12/12 21:11:39, alexmos wrote: > nit: no blank line. Done. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:467: On 2016/12/12 21:11:39, alexmos wrote: > nit: no blank line (this whole block is about navigating the subframe) Done. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:476: EXPECT_EQ(delegate.getURL(), "mail@example.org"); On 2016/12/12 21:11:39, alexmos wrote: > Let's also validate that the scheme of the URL is in fact "mailto". You could > do that just by checking the whole URL against GURL("mailto:mail@example.org"). Done. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/externa... File chrome/browser/external_protocol/external_protocol_handler.cc (left): https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/externa... chrome/browser/external_protocol/external_protocol_handler.cc:8: On 2016/12/12 21:11:40, alexmos wrote: > Did you remove the blank line here by accident? Done. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/externa... File chrome/browser/external_protocol/external_protocol_handler.h (right): https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/externa... chrome/browser/external_protocol/external_protocol_handler.h:112: int render_process_host_id, On 2016/12/12 21:11:40, alexmos wrote: > I wonder if we could likewise refactor this to take in a WebContents directly. > But we can save that for a followup CL, to avoid this one getting too big. > (Note that we can't refactor ExternalProtocolHandler::LaunchUrlWithDelegate to > take in a WebContents directly, as it posts a task to the FILE thread and then > gets back a callback, and the WebContents could be gone by the time of the > callback.) Acknowledged. https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/loader/... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2538353002/diff/180001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:230: int render_process_id = On 2016/12/12 21:11:40, alexmos wrote: > The else {} below is the only place left where it's used, and the corresponding > routing ID is retrieved directly. Perhaps it'll be cleaner to not bother with > this variable and replace the render_process_id with the direct call to > web_contents->GetRenderViewHost()->GetProcess()->GetID(), similarly to > GetRoutingID. Done. https://codereview.chromium.org/2538353002/diff/180001/chrome/test/data/page_... File chrome/test/data/page_with_mailto.html (right): https://codereview.chromium.org/2538353002/diff/180001/chrome/test/data/page_... chrome/test/data/page_with_mailto.html:3: <body onload="javascript: window.location.href='mailto:mail@example.org';"> On 2016/12/12 21:11:40, alexmos wrote: > no need for "javascript:", I think. Done.
Thanks, LGTM with nits. https://codereview.chromium.org/2538353002/diff/280001/chrome/browser/chrome_... File chrome/browser/chrome_site_per_process_browsertest.cc (right): https://codereview.chromium.org/2538353002/diff/280001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:369: // request succeeds. nit: looks like this will fit on previous line. https://codereview.chromium.org/2538353002/diff/280001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:377: std::string external_protocol_url() { You can return this as a const GURL&, and just return external_protocol_url_. Then your test can just compare that to GURL("mailto:mail@example.org"). (btw, if you did need the string from a GURL, the way to get that would be url.spec()). https://codereview.chromium.org/2538353002/diff/280001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:400: return ExternalProtocolHandler::DONT_BLOCK; Good call, I agree this is safer than relying on the default from ExternalProtocolHandler. https://codereview.chromium.org/2538353002/diff/280001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:433: // navigations before getting to external protocol code nit: end with period. https://codereview.chromium.org/2538353002/diff/280001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:441: nit: no blank line
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...
All requested fixes have been made. https://codereview.chromium.org/2538353002/diff/280001/chrome/browser/chrome_... File chrome/browser/chrome_site_per_process_browsertest.cc (right): https://codereview.chromium.org/2538353002/diff/280001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:369: // request succeeds. On 2016/12/13 23:27:28, alexmos wrote: > nit: looks like this will fit on previous line. Done. https://codereview.chromium.org/2538353002/diff/280001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:377: std::string external_protocol_url() { On 2016/12/13 23:27:28, alexmos wrote: > You can return this as a const GURL&, and just return external_protocol_url_. > Then your test can just compare that to GURL("mailto:mail@example.org"). (btw, > if you did need the string from a GURL, the way to get that would be > url.spec()). Done. https://codereview.chromium.org/2538353002/diff/280001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:400: return ExternalProtocolHandler::DONT_BLOCK; On 2016/12/13 23:27:28, alexmos wrote: > Good call, I agree this is safer than relying on the default from > ExternalProtocolHandler. Acknowledged. https://codereview.chromium.org/2538353002/diff/280001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:433: // navigations before getting to external protocol code On 2016/12/13 23:27:28, alexmos wrote: > nit: end with period. Done. https://codereview.chromium.org/2538353002/diff/280001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:441: On 2016/12/13 23:27:28, alexmos wrote: > nit: no blank line Done.
davidsac@chromium.org changed reviewers: + creis@chromium.org, meacer@chromium.org
Hi, meacer@, could you please review this changelist for OWNER approval of chrome/browser/external_protocol/ ? cresi@, could you please review the overall approach of this changelist?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Replace using the wrong render_process_id for launching external protocol requests in chrome_resource_dispatcher_host_delegate.cc. Previously, handlers for external protocols like 'mailto:' weren't working for OOPIFs because the render_process_id used to retrieve the content::WebContents object was incorrect. BUG=668289 ========== to ========== fix external protocol handling for OOPIFs Replace using the wrong render_process_id for launching external protocol requests in chrome_resource_dispatcher_host_delegate.cc. Previously, handlers for external protocols like 'mailto:' weren't working for OOPIFs because the render_process_id used to retrieve the content::WebContents object was incorrect. BUG=668289 TEST=See bug for repro steps ==========
Thanks! LGTM with one rename. https://codereview.chromium.org/2538353002/diff/300001/chrome/browser/externa... File chrome/browser/external_protocol/external_protocol_handler.cc (right): https://codereview.chromium.org/2538353002/diff/300001/chrome/browser/externa... chrome/browser/external_protocol/external_protocol_handler.cc:83: int tab_contents_id, Let's rename to render_view_routing_id. https://codereview.chromium.org/2538353002/diff/300001/chrome/browser/externa... chrome/browser/external_protocol/external_protocol_handler.cc:232: Profile::FromBrowserContext(web_contents->GetBrowserContext()), url); Interesting. We only care about getting the BrowserContext from the tab? Ok, sounds like we may not need to switch from view to frame routing IDs, since all frames in a tab are necessarily in the same BrowserContext.
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_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
All issues have been handled. https://codereview.chromium.org/2538353002/diff/300001/chrome/browser/externa... File chrome/browser/external_protocol/external_protocol_handler.cc (right): https://codereview.chromium.org/2538353002/diff/300001/chrome/browser/externa... chrome/browser/external_protocol/external_protocol_handler.cc:83: int tab_contents_id, This will be completed in a follow-up CL https://codereview.chromium.org/2538353002/diff/300001/chrome/browser/externa... chrome/browser/external_protocol/external_protocol_handler.cc:232: Profile::FromBrowserContext(web_contents->GetBrowserContext()), url); On 2016/12/15 00:10:04, Charlie Reis wrote: > Interesting. We only care about getting the BrowserContext from the tab? Ok, > sounds like we may not need to switch from view to frame routing IDs, since all > frames in a tab are necessarily in the same BrowserContext. Done.
chrome/browser/external_protocol/ lgtm https://codereview.chromium.org/2538353002/diff/300001/chrome/browser/externa... File chrome/browser/external_protocol/external_protocol_handler.h (right): https://codereview.chromium.org/2538353002/diff/300001/chrome/browser/externa... chrome/browser/external_protocol/external_protocol_handler.h:111: // TODO(davidsac): Consider refactoring this to take a WebContents directly. nit: Point to the bug instead? TODO(crbug.com/12345)
The bug number has been added to the TODO https://codereview.chromium.org/2538353002/diff/300001/chrome/browser/externa... File chrome/browser/external_protocol/external_protocol_handler.h (right): https://codereview.chromium.org/2538353002/diff/300001/chrome/browser/externa... chrome/browser/external_protocol/external_protocol_handler.h:111: // TODO(davidsac): Consider refactoring this to take a WebContents directly. On 2016/12/15 01:54:16, Mustafa Emre Acer wrote: > nit: Point to the bug instead? > > TODO(crbug.com/12345) Done.
davidsac@chromium.org changed reviewers: + sky@chromium.org
sky, Could you please review this changelist for OWNER approval for the rest of the chrome/browser/ ?
sky@chromium.org changed reviewers: + mmenke@chromium.org
+mmenke for c/b/loader https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/external_protocol_dialog.mm (right): https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/external_protocol_dialog.mm:121: tab_util::GetWebContentsByID(render_process_host_id_, routing_id_); How do you know this returns non-null by the time you get here? https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/ui/exte... File chrome/browser/ui/external_protocol_dialog_delegate.cc (right): https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:69: content::WebContents* web_contents = Same comment.
https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/loader/... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:249: url, web_contents->GetRenderViewHost()->GetProcess()->GetID(), How can we be sure the process associated with the WebContents hasn't changed in the meantime?
https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/loader/... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:249: url, web_contents->GetRenderViewHost()->GetProcess()->GetID(), On 2016/12/15 20:54:54, mmenke (Out Dec 17 to Jan 2) wrote: > How can we be sure the process associated with the WebContents hasn't changed in > the meantime? Good point. Maybe we should be passing both the process ID and the routing ID in the PostTask?
https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/loader/... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:249: url, web_contents->GetRenderViewHost()->GetProcess()->GetID(), Yes, we could keep passing the process_id to LaunchURL and cancel the external protocol launch here if it doesn't match the WebContents' current process there. But wouldn't it still be ok to launch the external app in this case, as once we launch the external app, there's really no guarantee that it will come up while the page that requested it is still the current one in WebContents? Hence it might not matter if the WebContents changed processes. The |web_contents| here is used for two things: getting the BrowserContext when launching the external app, and for putting up a confirmation dialog to ask the user whether to launch unknown protocols. Those happen after the call here, and the dialog is preceded by a hop to the FILE thread and back, so seems like it could already end up being shown for the wrong page. https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/external_protocol_dialog.mm (right): https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/external_protocol_dialog.mm:121: tab_util::GetWebContentsByID(render_process_host_id_, routing_id_); We don't. The first action in ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck is to make sure that the web_contents is not null, and return if it is. Does this seem like it would work? Or do you think it's better to do the null checks at its call sites? https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/ui/exte... File chrome/browser/ui/external_protocol_dialog_delegate.cc (right): https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:69: content::WebContents* web_contents = Same reply.
https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/loader/... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:249: url, web_contents->GetRenderViewHost()->GetProcess()->GetID(), On 2016/12/15 22:40:23, davidsac wrote: > Yes, we could keep passing the process_id to LaunchURL and cancel the external > protocol launch here if it doesn't match the WebContents' current process there. > > But wouldn't it still be ok to launch the external app in this case, as once we > launch the external app, there's really no guarantee that it will come up while > the page that requested it is still the current one in WebContents? Hence it > might not matter if the WebContents changed processes. > > The |web_contents| here is used for two things: getting the BrowserContext when > launching the external app, and for putting up a confirmation dialog to ask the > user whether to launch unknown protocols. Those happen after the call here, and > the dialog is preceded by a hop to the FILE thread and back, so seems like it > could already end up being shown for the wrong page. Why do we need the process ID for either of those things? Surely we aren't running the confirmation dialog in the untrusted renderer process?
https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/loader/... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:249: url, web_contents->GetRenderViewHost()->GetProcess()->GetID(), On 2016/12/16 16:24:17, mmenke (Out Dec 17 to Jan 2) wrote: > On 2016/12/15 22:40:23, davidsac wrote: > > Yes, we could keep passing the process_id to LaunchURL and cancel the external > > protocol launch here if it doesn't match the WebContents' current process > there. > > > > But wouldn't it still be ok to launch the external app in this case, as once > we > > launch the external app, there's really no guarantee that it will come up > while > > the page that requested it is still the current one in WebContents? Hence it > > might not matter if the WebContents changed processes. > > > > The |web_contents| here is used for two things: getting the BrowserContext > when > > launching the external app, and for putting up a confirmation dialog to ask > the > > user whether to launch unknown protocols. Those happen after the call here, > and > > the dialog is preceded by a hop to the FILE thread and back, so seems like it > > could already end up being shown for the wrong page. > > Why do we need the process ID for either of those things? Surely we aren't > running the confirmation dialog in the untrusted renderer process? The process ID and routing ID are just used to get the WebContents which is then needed for both of these. ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck() uses that WebContents to get the BrowserContext, which is used to launch the external app (if we end up launching gmail for mailto, profile is needed to navigate in the right one). LaunchUrlWithDelegate needs the WebContents to put up the unknown protocol dialog (depending on platform, this is needed to parent the ui window properly, etc.), and then calls LaunchUrlWithoutSecurityCheck. Here's my understanding of the possible races here. There are two flows: (a) No dialog, i.e. the |is_whitelisted| flow above: (1) request(IO) -> (2) LaunchURL(UI) -> (3) LaunchUrlWithoutSecurityCheck(UI) which launches the external app Previously, the WebContents was resolved with a RVH process_id/routing_id pair in (3). If WebContents went away between (1) and (2), we exited early with a null check on |web_contents| above in (2). If the main frame process changed during the thread hop, we would either fail to find the WebContents (because the old RFH/RVH went away) and hit the same null check, or we would still find it and use it in (3) because the old RVH is around in swapped-out state (e.g., when the RFH is pending deletion), which the old code did not check. With this CL, the WebContents is now resolved in (2) via |web_contents_getter|, which internally uses RFH process_id/routing_id (using that instead of RVH IDs fixes this for OOPIFs). If the WC went away between (1) and (2), we return early on the same null check, so no behavior change. If the main frame process changed, we still end up either returning early on the same null check, or proceeding if the old RFH is still around and pending deletion, so again no real behavior change. (There is a slight difference in that the swapped-out RVH might stay around after RFH goes away, which I think makes the new code a bit saner.) I'm guessing part of your question is whether we want to detect that the main frame process changed and stop the external app/dialog if so. That might be reasonable, but given that it was already possible in the old code, I don't know if we should be fixing it here. Note that this only makes sense for the dialog -- I don't think we can guarantee anything about when the external handler app is launched. b) With dialog: (1) request(IO) -> (2) LaunchURL(UI) -> (3) CreateShellWorker(FILE) -> (4) OnDefaultProtocolClientWorkerFinished(UI) -> (5) optional dialog(UI) -> (6) LaunchUrlWithoutSecurityCheck(UI) (if confirmed) This is actually quite similar. With the old code, we resolved and null-checked the WebContents in (5) using RVH IDs, and then again in (6). Same kinds of races are possible, except that they can also happen during the hop to the FILE thread and back. With the new code, we resolve the WebContents in (2) and pass down its current RVH IDs through to (5) (this is the line you're asking about). Then we resolve it again in (5) and (6), and those all have null checks because WC could go away between (3) and (5). Again, I don't think we're changing behavior for any of the races.
Thanks for the explanation! chrome/browser/loader LGTM. Does seem a bit weird to be using PIDs/frame IDs to identify WebContents, since they can change, but as you say, it's the status quo. Would it make more sense to make ExternalProtocolHandler::LaunchUrlWithDelegate take the WebContents instead? That's what it really wants. The fact that it uses process IDs / frame IDs is more an implementation detail, and no reason for The RDHDelegate to know about it.
davidsac@chromium.org changed reviewers: + msw@chromium.org
Hi msw, Unfortunately, sky is out of office and I'm not sure when he will be back. Would you mind reviewing the two files in chrome/browser/ui for OWNER approval?
c/b/ui lgtm; hopefully Scott would have said the same. https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/external_protocol_dialog.mm (right): https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/external_protocol_dialog.mm:15: #include "content/public/browser/web_contents.h" optional nit: remove this include, it's not explicitly needed (type fwd declared by tab_util.h and not dereferenced here) https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/external_protocol_dialog.mm:121: tab_util::GetWebContentsByID(render_process_host_id_, routing_id_); On 2016/12/15 22:40:23, davidsac wrote: > We don't. The first action in > ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck is to make sure that the > web_contents is not null, and return if it is. Does this seem like it would > work? Or do you think it's better to do the null checks at its call sites? I can't claim to speak for Scott, but that seems okay to me.
mmenke, We will make this change in a followup CL. Thank you for this suggestion!
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...
https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/external_protocol_dialog.mm (right): https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/external_protocol_dialog.mm:15: #include "content/public/browser/web_contents.h" On 2016/12/19 20:26:55, msw wrote: > optional nit: remove this include, it's not explicitly needed (type fwd declared > by tab_util.h and not dereferenced here) Done. https://codereview.chromium.org/2538353002/diff/340001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/external_protocol_dialog.mm:121: tab_util::GetWebContentsByID(render_process_host_id_, routing_id_); On 2016/12/19 20:26:55, msw wrote: > On 2016/12/15 22:40:23, davidsac wrote: > > We don't. The first action in > > ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck is to make sure that > the > > web_contents is not null, and return if it is. Does this seem like it would > > work? Or do you think it's better to do the null checks at its call sites? > > I can't claim to speak for Scott, but that seems okay to me. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by davidsac@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org, creis@chromium.org, meacer@chromium.org, msw@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2538353002/#ps360001 (title: "remove unnecessary include")
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": 360001, "attempt_start_ts": 1482196896657300,
"parent_rev": "ee252813b0082dfd7ad6cff21bcf33bdc8203736", "commit_rev":
"5485a2fab0517e365a30cab0eea038f2dfc27902"}
Message was sent while issue was closed.
Description was changed from ========== fix external protocol handling for OOPIFs Replace using the wrong render_process_id for launching external protocol requests in chrome_resource_dispatcher_host_delegate.cc. Previously, handlers for external protocols like 'mailto:' weren't working for OOPIFs because the render_process_id used to retrieve the content::WebContents object was incorrect. BUG=668289 TEST=See bug for repro steps ========== to ========== fix external protocol handling for OOPIFs Replace using the wrong render_process_id for launching external protocol requests in chrome_resource_dispatcher_host_delegate.cc. Previously, handlers for external protocols like 'mailto:' weren't working for OOPIFs because the render_process_id used to retrieve the content::WebContents object was incorrect. BUG=668289 TEST=See bug for repro steps Review-Url: https://codereview.chromium.org/2538353002 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== fix external protocol handling for OOPIFs Replace using the wrong render_process_id for launching external protocol requests in chrome_resource_dispatcher_host_delegate.cc. Previously, handlers for external protocols like 'mailto:' weren't working for OOPIFs because the render_process_id used to retrieve the content::WebContents object was incorrect. BUG=668289 TEST=See bug for repro steps Review-Url: https://codereview.chromium.org/2538353002 ========== to ========== fix external protocol handling for OOPIFs Replace using the wrong render_process_id for launching external protocol requests in chrome_resource_dispatcher_host_delegate.cc. Previously, handlers for external protocols like 'mailto:' weren't working for OOPIFs because the render_process_id used to retrieve the content::WebContents object was incorrect. BUG=668289 TEST=See bug for repro steps Committed: https://crrev.com/1fe2ac646c67872405598ad551dc4b2b430cfce4 Cr-Commit-Position: refs/heads/master@{#439654} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/1fe2ac646c67872405598ad551dc4b2b430cfce4 Cr-Commit-Position: refs/heads/master@{#439654} |
