New issue
Advanced search Search tips

Issue 718516 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocked on:
issue 791796



Sign in to add a comment

Figure out process model for hosted apps (in default mode, --top-document-isolation and "isolate a subset of sites" modes)

Project Member Reported by lukasza@chromium.org, May 4 2017

Issue description

We need to figure out the desired process model for hosted apps, in particular which frames should go to which process given the following frames:
1. hosted-app.site.com/covered-by-app-extent
2. hosted-app.site.com/not-covered-by-app-extent
3. web.site.com (same-site, but cross-origin from the hosted app)
4. cross.site.web.content (cross-site from everything above)

Currently, --site-per-process isolation model puts 1 in a different process from 2 and 3, which breaks synchronous scripting across same-origin frames (1 and 2) or across same-site frames (assumming 1 and 3 use window.domain="http://site.com").
 
For some more context, please also see  issue 679011 .

Comment 2 by creis@chromium.org, Oct 31 2017

Blocking: 780133

Comment 3 by creis@chromium.org, Nov 11 2017

Owner: creis@chromium.org
Status: Assigned (was: Untriaged)
Starting to take a look.  I'm pretty sure we'll want to keep 2 and 3 in the same process as the hosted app, even in --site-per-process.  (We'll make an exception for 3 if it's marked as an isolated origin, in which case it will be an OOPIF.)  Case 4 will be an OOPIF if we have a policy asking for that (e.g., --site-per-process).

Implementing this may be tricky.
AFAIK hosted apps (case 1 above) are process-per-site, if "background" permission AND allow_js_access is true (it defaults to true).  If (as suggested in #c3) we keep 2 and 3 in the same process as the hosted app, then I think it means that the whole site has to become process-per-site.  Putting a popular site (like google.com) into a process-per-site mode might be problematic (AFAIU all frames from this site would go into a single, gigantic process in this case [except isolated origins]).

I am not sure how to work around this issue.  Maybe a step in the right direction would be to first get a better, non-process-per-site behavior for hosted apps that do NOT have background permission and/or do NOT have allow_js_access setting.

Comment 5 by creis@chromium.org, Nov 14 2017

Comment 4: I don't think process-per-site is necessary.  We already do a process swap if case 2 opens case 1 in a new window, so we don't need to preserve the ability to script in that case.

I think the key is treating most of the site as usual, but any same-site frames navigated from within the hosted app will stay in the hosted app process.  We'll be able to construct cases that same-origin frames can't script each other, but that's already true today.  Does that sound reasonable?

(Hopefully someday we can remove hosted apps from the process model, but we'll need to do something in the short term.)
Project Member

Comment 6 by bugdroid1@chromium.org, Nov 22 2017

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

commit 6331f2221ea59a29f988c649ddf6d1dd74926d33
Author: Charles Reis <creis@chromium.org>
Date: Wed Nov 22 01:26:21 2017

Ensure isolated origins aren't treated as hosted apps in process map.

BUG=718516
TEST=No visible behavior change.

Change-Id: I6fb344953717c7180a13a5bee03eb02f29444dca
Reviewed-on: https://chromium-review.googlesource.com/781423
Commit-Queue: Charlie Reis <creis@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518483}
[modify] https://crrev.com/6331f2221ea59a29f988c649ddf6d1dd74926d33/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[modify] https://crrev.com/6331f2221ea59a29f988c649ddf6d1dd74926d33/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[modify] https://crrev.com/6331f2221ea59a29f988c649ddf6d1dd74926d33/tools/metrics/histograms/enums.xml

Comment 7 by creis@chromium.org, Nov 30 2017

Labels: -OS-All OS-Chrome OS-Linux OS-Mac OS-Windows
Owner: alex...@chromium.org
Status: Started (was: Assigned)
alexmos@ is taking over my earlier attempt at this from https://chromium-review.googlesource.com/c/chromium/src/+/792605, since I'm busy with another bug.  (Thanks!)

Comment 8 by creis@chromium.org, Nov 30 2017

Blocking: -780133
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 2 2017

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

commit 25c64bbe1ea8684ae7eac2e07dc8b078b3484d68
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Sat Dec 02 02:50:11 2017

Avoid same-site OOPIFs in hosted apps in --site-per-process.

This CL adds several process model tests for hosted apps and tries
to keep same-site iframes in the hosted app's process.  There are
additional ways to improve --site-per-process behavior around hosted
apps (e.g., same-site popups), but this CL doesn't address them yet.

This CL is a continuation of creis@'s
https://chromium-review.googlesource.com/c/chromium/src/+/792605.

Bug: 718516
Change-Id: I10c6ecc6799aa92bbd789efb5e8dcfebd1aa90da
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Reviewed-on: https://chromium-review.googlesource.com/798120
Commit-Queue: Charlie Reis <creis@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521188}
[modify] https://crrev.com/25c64bbe1ea8684ae7eac2e07dc8b078b3484d68/chrome/browser/extensions/app_process_apitest.cc
[modify] https://crrev.com/25c64bbe1ea8684ae7eac2e07dc8b078b3484d68/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[modify] https://crrev.com/25c64bbe1ea8684ae7eac2e07dc8b078b3484d68/chrome/test/data/frame_tree/cross_origin_but_same_site_frames.html
[modify] https://crrev.com/25c64bbe1ea8684ae7eac2e07dc8b078b3484d68/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/25c64bbe1ea8684ae7eac2e07dc8b078b3484d68/content/browser/frame_host/render_frame_host_manager.h
[modify] https://crrev.com/25c64bbe1ea8684ae7eac2e07dc8b078b3484d68/content/browser/frame_host/render_frame_host_manager_unittest.cc
[modify] https://crrev.com/25c64bbe1ea8684ae7eac2e07dc8b078b3484d68/content/browser/renderer_host/render_process_host_unittest.cc
[modify] https://crrev.com/25c64bbe1ea8684ae7eac2e07dc8b078b3484d68/content/browser/site_instance_impl.cc
[modify] https://crrev.com/25c64bbe1ea8684ae7eac2e07dc8b078b3484d68/content/browser/site_instance_impl.h
[modify] https://crrev.com/25c64bbe1ea8684ae7eac2e07dc8b078b3484d68/content/browser/site_instance_impl_unittest.cc
[modify] https://crrev.com/25c64bbe1ea8684ae7eac2e07dc8b078b3484d68/content/public/test/test_utils.cc
[modify] https://crrev.com/25c64bbe1ea8684ae7eac2e07dc8b078b3484d68/content/public/test/test_utils.h

Labels: -Pri-3 Pri-2
I've verified on Mac canary 65.0.3284.0, first one which includes r521188 from #9, that our fix is working properly.  My repro steps:

1. Put together a sample hosted app manifest.json with the following content (thanks creis@ for providing this):
{
  "app": {
     "launch": {
        "web_url": "https://csreis.github.io/tests"
     },
     "urls": [ "https://csreis.github.io/tests" ]
  },
  "background": {
     "allow_js_access": false
  },
  "name": "csreis hosted app",
  "manifest_version": 2,
  "version": "1.0"
}

2. Start Chrome with --site-per-process and load the hosted app from 1) from chrome://extensions.

3. Go to https://csreis.github.io/tests/cross-site-iframe.html and verify that the tab shows up as an "App: " in process manager.

4. Click "Go cross-site (simple page)".  This navigates the subframe to https://csreis.github.io/, which is not part of the app's extent, but same-site with the main frame.

5. Verify that there is no OOPIF for this in process manager.  Prior to #9, we were creating an OOPIF for this.  Clicking "Go cross-site (complex page)" correctly kicks the chromium.org navigation into an OOPIF, and then clicking "Go cross-site (simple page)" again correctly bring it back into the app process.

Blockedon: 791796
Cc: lukasza@chromium.org
#9 fixes the problem with unnecessary OOPIFs with --site-per-process in the most common cases (e.g., OOPIFs on https://docs.google.com with the Drive app installed).  However, there are still a few things to follow up on:

1. Figure out the popups cases - see all the popups tests with a TODO in r521188.  For popups to an app URL opened from a same-site non-app URL, the popup should swap processes and open in the app process (thanks to  issue 89272 ), even though this breaks scripting.  However, popups from an app to a same-site non-app URL should stay in the app process.  Basically, once we're in the app process, we should stay there for all new same-site frames.

2. Dealing with hosted apps with multiple sites - see  issue 791796 .

3. Cross-site subframes are still broken in TDI mode.  When a hosted app main frame embeds a cross-site subframe, the subframe doesn't currently go into the subframe process, thanks to    ChromeContentBrowserClientExtensionsPart::ShouldFrameShareParentSiteInstanceDespiteTopDocumentIsolation, which keeps all frames in the main frame's SiteInstance if the main frame is a hosted app.   r521188 didn't attempt to fix this, but there's a TODO there to enable some parts of the tests there once this is fixed.  Looks like this was added by lukasza@ as part of fixing  issue 679011 , but now that we compare real (not effective) URLs in IsCurrentlySameSite, we might be able to remove this whole codepath.  Lukasz, do you agree?
Labels: Merge-Request-64
Requesting a merge of r521188 to M64.  This fixes a correctness issue with --site-per-process and hosted apps (namely, without this fix, a hosted app that embeds a same-site iframe outside of the app's extent won't be able to script it, as we will kick it out to an OOPIF).  It also reduces process overhead for --site-per-process.  It has been verified on canary (see #10), where's it's been baking for the past few days.
Project Member

Comment 14 by sheriffbot@chromium.org, Dec 10 2017

Labels: -Merge-Request-64 Hotlist-Merge-Approved Merge-Approved-64
Your change meets the bar and is auto-approved for M64. Please go ahead and merge the CL to branch 3282 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

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

Labels: -merge-approved-64 merge-merged-3282
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e165a76f908b6ac9a324ca5df0df772708e4065f

commit e165a76f908b6ac9a324ca5df0df772708e4065f
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Mon Dec 11 23:04:24 2017

Avoid same-site OOPIFs in hosted apps in --site-per-process (Merge to M63)

This CL adds several process model tests for hosted apps and tries
to keep same-site iframes in the hosted app's process.  There are
additional ways to improve --site-per-process behavior around hosted
apps (e.g., same-site popups), but this CL doesn't address them yet.

This CL is a continuation of creis@'s
https://chromium-review.googlesource.com/c/chromium/src/+/792605.

TBR=alexmos@chromium.org

(cherry picked from commit 25c64bbe1ea8684ae7eac2e07dc8b078b3484d68)

Bug: 718516
Change-Id: I10c6ecc6799aa92bbd789efb5e8dcfebd1aa90da
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Reviewed-on: https://chromium-review.googlesource.com/798120
Commit-Queue: Charlie Reis <creis@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#521188}
Reviewed-on: https://chromium-review.googlesource.com/820933
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/branch-heads/3282@{#156}
Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840}
[modify] https://crrev.com/e165a76f908b6ac9a324ca5df0df772708e4065f/chrome/browser/extensions/app_process_apitest.cc
[modify] https://crrev.com/e165a76f908b6ac9a324ca5df0df772708e4065f/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[modify] https://crrev.com/e165a76f908b6ac9a324ca5df0df772708e4065f/chrome/test/data/frame_tree/cross_origin_but_same_site_frames.html
[modify] https://crrev.com/e165a76f908b6ac9a324ca5df0df772708e4065f/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/e165a76f908b6ac9a324ca5df0df772708e4065f/content/browser/frame_host/render_frame_host_manager.h
[modify] https://crrev.com/e165a76f908b6ac9a324ca5df0df772708e4065f/content/browser/frame_host/render_frame_host_manager_unittest.cc
[modify] https://crrev.com/e165a76f908b6ac9a324ca5df0df772708e4065f/content/browser/renderer_host/render_process_host_unittest.cc
[modify] https://crrev.com/e165a76f908b6ac9a324ca5df0df772708e4065f/content/browser/site_instance_impl.cc
[modify] https://crrev.com/e165a76f908b6ac9a324ca5df0df772708e4065f/content/browser/site_instance_impl.h
[modify] https://crrev.com/e165a76f908b6ac9a324ca5df0df772708e4065f/content/browser/site_instance_impl_unittest.cc
[modify] https://crrev.com/e165a76f908b6ac9a324ca5df0df772708e4065f/content/public/test/test_utils.cc
[modify] https://crrev.com/e165a76f908b6ac9a324ca5df0df772708e4065f/content/public/test/test_utils.h

Sorry, made a typo in the CL description in #c15: the merge was for M64, not M63.  
Project Member

Comment 17 by bugdroid1@chromium.org, Apr 9 2018

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

commit a854a5e7aebc93dfe1dc3d77aeebdcd62ad048a6
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Mon Apr 09 23:37:14 2018

Keep same-site, non-app popups from hosted apps in the app process.

This CL fixes the scenario where a frame in a hosted app process would
open a popup to a same-site URL outside the app's extent, and this
would end up in a separate, non-app process, preventing the popup from
scripting its same-site opener.

This is done by having RenderFrameHostManager::IsCurrentlySameSite()
ignore effective URLs in certain main frame cases - namely, when the
source frame uses an effective URL but the destination URL does not,
and there's another page that will be able to script the new frame.
This builds on the earlier hosted app fixes for subframes from issue
718516.

Bug: 828720, 718516
Change-Id: I97d6c0339a72f93b6412b62f4b083625450edd7e
Reviewed-on: https://chromium-review.googlesource.com/998178
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549321}
[modify] https://crrev.com/a854a5e7aebc93dfe1dc3d77aeebdcd62ad048a6/chrome/browser/extensions/app_process_apitest.cc
[modify] https://crrev.com/a854a5e7aebc93dfe1dc3d77aeebdcd62ad048a6/chrome/browser/extensions/isolated_app_browsertest.cc
[modify] https://crrev.com/a854a5e7aebc93dfe1dc3d77aeebdcd62ad048a6/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[modify] https://crrev.com/a854a5e7aebc93dfe1dc3d77aeebdcd62ad048a6/content/browser/frame_host/render_frame_host_manager.cc

Project Member

Comment 18 by bugdroid1@chromium.org, Apr 10 2018

Labels: merge-merged-3359
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/49a93847efd7126c2a968416ae6c76c2c49f8fe0

commit 49a93847efd7126c2a968416ae6c76c2c49f8fe0
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Tue Apr 10 21:11:35 2018

Keep same-site, non-app popups from hosted apps in the app process. (Merge to M66)

This CL fixes the scenario where a frame in a hosted app process would
open a popup to a same-site URL outside the app's extent, and this
would end up in a separate, non-app process, preventing the popup from
scripting its same-site opener.

This is done by having RenderFrameHostManager::IsCurrentlySameSite()
ignore effective URLs in certain main frame cases - namely, when the
source frame uses an effective URL but the destination URL does not,
and there's another page that will be able to script the new frame.
This builds on the earlier hosted app fixes for subframes from issue
718516.

TBR=alexmos@chromium.org

(cherry picked from commit a854a5e7aebc93dfe1dc3d77aeebdcd62ad048a6)

Bug: 828720, 718516
Change-Id: I97d6c0339a72f93b6412b62f4b083625450edd7e
Reviewed-on: https://chromium-review.googlesource.com/998178
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#549321}
Reviewed-on: https://chromium-review.googlesource.com/1005987
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#664}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/49a93847efd7126c2a968416ae6c76c2c49f8fe0/chrome/browser/extensions/app_process_apitest.cc
[modify] https://crrev.com/49a93847efd7126c2a968416ae6c76c2c49f8fe0/chrome/browser/extensions/isolated_app_browsertest.cc
[modify] https://crrev.com/49a93847efd7126c2a968416ae6c76c2c49f8fe0/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[modify] https://crrev.com/49a93847efd7126c2a968416ae6c76c2c49f8fe0/content/browser/frame_host/render_frame_host_manager.cc

Hi all. 
Changes in https://chromium.googlesource.com/chromium/src/+/49a93847efd7126c2a968416ae6c76c2c49f8fe0
have broken our statistics for navigations from Yandex search in Chromium NTP.

Steps to reproduce:
1. Choose Yandex as default search engine.
2. Open NTP
3. Start typing "onlime" in NTP search editbox
4. Select and click with mouse first navigational suggest in suggest popup - "onlime.ru"
5. Another tab opens with onlime.ru site open.
6. Open devtools in onlime.ru tab, select console and enter "document.referrer"

Expected value - not empty referrer string containin something like "https://yandex.ru/clck/jsredir?from=yandex.ru%3Bsuggest..."
Actual value - empty string.

Problem is not reproduced with same steps if I create tab and navigate to yandex.ru and repeat steps 3-6 from above.
I have created video with steps to reproduce https://drive.google.com/open?id=1ZNvTwOwfePv_YlR0gVYds0mauEDFdqLX
 

This problem have affected calculation of our search navigation statistics and we would be grateful if you fixed it or helped us to fix it.

I am currently analyzing and debugging problem. 
I see that new calculation of should_compare_effective_urls variable in IsCurrentlySameSite function somehow changed behaviour.
Ok, I have found how CL(https://chromium-review.googlesource.com/c/chromium/src/+/998178) broken our navigation stats:
I omit some details, to keep description readable.

Old behaviour:
0. User set Yandex as default search
1. User opens NTP - Yandex remote NTP page is opened, SiteInstance for it created, for example SiteInstance id=1
2. User types in Yandex NTP, in search editbox - "onlime.ru" and click with mouse on first navigation suggest.
3. Navigation to "https://yandex.ru/clck/jsredir?long_query_string" is initiated. To check if current NTP SiteInstance(id=1) can be reused for destination url, function RenderFrameHostManager::GetSiteInstanceForNavigationRequest calls indirectly RenderFrameHostManager::IsCurrentlySameSite function with "https://yandex.ru/clck/jsredir?long_query_string" as dest_url param. Old version of IsCurrentlySameSite function returned false for this request. New SiteInstance(id=2) was created for destination url, it navigated to "https://yandex.ru/clck/jsredir?long_query_string" which immediately redirected itself to "http://www.onlime.ru" - also in separate SiteInstance.
Referrer from SiteInstance(id=2) was correctly passed to "http://www.onlime.ru" site.

New behaviour:
Steps 0,1,2 - same as above.
3. Navigation to "https://yandex.ru/clck/jsredir?long_query_string" is initiated. To check if current NTP SiteInstance(id=1) can be used for destination url, function RenderFrameHostManager::GetSiteInstanceForNavigationRequest calls indirectly RenderFrameHostManager::IsCurrentlySameSite function with "https://yandex.ru/clck/jsredir?long_query_string" as dest_url param. New version of IsCurrentlySameSite returnes true. 
Old NTP SiteInstance(id=1) is reused to navigate to "https://yandex.ru/clck/jsredir?long_query_string". It navigates to this new link and when immediately redirected itself to "http://www.onlime.ru".
Referrer from SiteInstance(id=1) is cleared in function ChromeContentBrowserClient::OverrideNavigationParams while navigating to "http://www.onlime.ru" site. Function OverrideNavigationParams is implemented so it clears all referrers for transitions from NTP site instance for transition type PAGE_TRANSITION_LINK. This is done to fix https://codereview.chromium.org/2084953003.

What problems I see here:
1. Logic that clears all referrers and overrides transition type for all link navigation from NTP implemented in ChromeContentBrowserClient::OverrideNavigationParams seems like a very crude solution for https://codereview.chromium.org/2084953003. Yandex remote NTP contains link elements other than TopSites and clearing referrers for them is clearly incorrect.
2. New version of IsCurrentlySameSite function implemented in https://chromium-review.googlesource.com/c/chromium/src/+/998178 allows to reuse NTP SiteInstance for all transition to same-host URLs (that's how I understand it). I am not sure that this is intendend result of source CL.

What can solve problem: 
1. Revert CL https://chromium-review.googlesource.com/c/chromium/src/+/998178. Or change IsCurrentlySameSite so it will not reuse NTP SiteInstance for navigations.
2. Rework OverrideNavigationParams so it will more intelligently detect clicks on TopSites.

WDYT? For me, the simplest and fastest solution is revert of CL mentioned in item 1, and further work on both items. 

Project Member

Comment 22 by bugdroid1@chromium.org, Aug 15

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

commit 6aedc6ecdaf896a00e7e50c97ab072feb8ea0909
Author: Alexander Yashkin <a-v-y@yandex-team.ru>
Date: Wed Aug 15 08:58:45 2018

Fixed NTP site instance sharing bug

This fixes the bug when every window opened from remote NTP tab with
same host as remote NTP but different path would be considered to belong
to same SiteInstance as NTP. That could lead to nasty effects, such as
referrer reset for transfers from Yandex remote NTP.
Basically its a rewrite of
https://chromium-review.googlesource.com/c/chromium/src/+/998178 patch
so it would apply to hosted apps only, as intended, IMHO.

Bug:  859062 ,718516
Change-Id: I76e90c2ff6311ca99a234e75424f8cba9932c05a
Reviewed-on: https://chromium-review.googlesource.com/1141574
Commit-Queue: Alexander Yashkin <a-v-y@yandex-team.ru>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583199}
[modify] https://crrev.com/6aedc6ecdaf896a00e7e50c97ab072feb8ea0909/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/6aedc6ecdaf896a00e7e50c97ab072feb8ea0909/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/6aedc6ecdaf896a00e7e50c97ab072feb8ea0909/chrome/browser/chrome_content_browser_client_browsertest.cc
[modify] https://crrev.com/6aedc6ecdaf896a00e7e50c97ab072feb8ea0909/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[modify] https://crrev.com/6aedc6ecdaf896a00e7e50c97ab072feb8ea0909/chrome/browser/extensions/chrome_content_browser_client_extensions_part.h
[modify] https://crrev.com/6aedc6ecdaf896a00e7e50c97ab072feb8ea0909/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/6aedc6ecdaf896a00e7e50c97ab072feb8ea0909/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/6aedc6ecdaf896a00e7e50c97ab072feb8ea0909/content/public/browser/content_browser_client.h

Forgot to reference this bug, but just for reference, r587295 was another process model change for hosted apps in --site-per-process, where they started to be locked to origin, and hence cross-site parts of the same hosted app started using different processes.
Project Member

Comment 24 by bugdroid1@chromium.org, Sep 14

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

commit d97d105fb1575487cfce16135bbc1faf9642e14b
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Fri Sep 14 23:24:25 2018

Remove hosted app reload logic from ShouldFork.

This CL removes the logic that triggered process swaps in
ChromeExtensionsRendererClient::ShouldFork() for the case when the
main frame reloads a URL for which a hosted app was just installed or
uninstalled.  This logic isn't needed with --site-per-process, as that
mode will already force a process transfer.  This is covered by
AppApiTest.ReloadIntoAppProcess and
AppApiTest.ReloadIntoAppProcessWithJavaScript, which still pass with
that logic removed.

This CL also makes a fix on the browser side to keep this logic
working even without --site-per-process, by unconditionally checking
HasWrongProcessForURL() and forcing a process transfer if needed in
IsRendererTransferNeededForNavigation().  This is possible because
with PlzNavigate we will always go to the browser process and check
for transfers, even without --site-per-process.  With that fix, the
above two tests also pass without --site-per-process.

The above tweak also has a side effect of swapping processes when an
app opens a cross-site, non-app popup (also made possible by work on
 issue 794315 ) in non-site-per-process mode, which seems acceptable and
desirable - no reason for the cross-site popup to be in a process with
app permissions.

Bug:  883550 , 883549, 718516
Test: AppApiTest.ReloadIntoAppProcess and AppApiTest.ReloadIntoAppProcessWithJavaScript keep working, with and without --site-per-process.
Change-Id: Ibac63fff3a36318fabceb593ca7ae7967eefad89
Reviewed-on: https://chromium-review.googlesource.com/1226075
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591521}
[modify] https://crrev.com/d97d105fb1575487cfce16135bbc1faf9642e14b/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[modify] https://crrev.com/d97d105fb1575487cfce16135bbc1faf9642e14b/chrome/renderer/extensions/chrome_extensions_renderer_client.cc
[modify] https://crrev.com/d97d105fb1575487cfce16135bbc1faf9642e14b/content/browser/frame_host/render_frame_host_manager.cc

Project Member

Comment 25 by bugdroid1@chromium.org, Oct 12

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

commit 68e20c09efe0c5c16c19c10d260d81c9d06ae802
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Fri Oct 12 00:09:08 2018

Switch CanAccessDataForOrigin to use DetermineProcessLockURL.

CanAccessDataForOrigin currently calls GetSiteForURL() to determine
what should be checked against the process's origin lock.  This isn't
entirely accurate, as GetSiteForURL() defaults to using effective
URLs, which we don't want to use for comparing origin locks.
Fortunately, we also pass in null for browser_context, which
effectively avoids effective URL resolution:
ChromeContentBrowserClient::GetEffectiveURL returns back the original
URL in that case.  This CL change this call to use
DetermineProcessLockForURL() instead, which is more correct.

This CL also removes a stale comment about hosted apps not being able
to set cookies.  That is no longer true, since we now lock hosted apps
to their underlying web origin, which doesn't get in the way of IO
thread enforcements.

Bug:  160576 , 718516,  794315 
Change-Id: I092e9bf89b3a9fc5807824bbe51d1de6589ddae3
Reviewed-on: https://chromium-review.googlesource.com/c/1276560
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599028}
[modify] https://crrev.com/68e20c09efe0c5c16c19c10d260d81c9d06ae802/content/browser/child_process_security_policy_impl.cc

Project Member

Comment 26 by bugdroid1@chromium.org, Dec 5

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

commit 25754cd4d3832e74532b6e67eea0eae260869900
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Wed Dec 05 22:43:08 2018

Remove isolated origin workarounds from CCBCEP::GetEffectiveURL().

Previously, if a hosted app's web extent matched one or more isolated
origins, we used complicated logic in
ChromeContentBrowserClientExtensionsPart::GetEffectiveURL to determine
when a URL was allowed to go into a hosted app process (by utilizing
an effective site URL) without compromising the security of isolated
origins.  Specifically, if a hosted app was contained entirely within
an isolated origin, then we decided it was safe to place any app URL
into an app process.  This is because we used to have an issue,
 https://crbug.com/791796 , where all origins covered by the app would
end up in the *same* app process, which could break process isolation
for isolated origins.

This logic is no longer needed after we started locking hosted app
processes to origin, which fixed  https://crbug.com/791796  and allowed
a hosted app to span multiple processes.  Given that work, if a hosted
app corresponds to different isolated origins, it's safe to simply
load those origins in different app processes.  Therefore, this CL
removes that logic and allows any URL covered by an app to load in an
app process.  If any of those URLs match isolated origins, they will
simply swap to a different app process -- no additional changes were
needed for this.

For example, suppose there's one isolated origin,
http://isolated.origin.com, and a hosted app's extent covers foo.com
and isolated.origin.com.  Suppose a main frame at foo.com (in app
process) embeds an iframe for http://isolated.origin.com.  Previously,
we'd kick the iframe into an OOPIF, with site URL of
http://isolated.origin.com, locked to http://isolated.origin.com, and
in a non-app process.  After this CL, we'd still kick the iframe into
an OOPIF, locked to https://isolated.origin.com, but it will have a
site URL of chrome-extension://appid/#https://isolated.origin.com, and
it will actually be in its own app process.  This is arguably more
correct, and should make hosted apps more compatible with isolated
origin uses.

Another motivation for this change is that it removes the only use of
ChildProcessSecurityPolicy::GetMatchingIsolatedOrigin outside of
content/.  That API will need to be updated to support dynamic
isolated origins (see issue 905513), and this will be easier to do if
this use case is removed.

Bug:  879722 , 718516, 905513
Change-Id: Ia60e1ad435d58c66a02730a1459ab0684fad89d1
Reviewed-on: https://chromium-review.googlesource.com/c/1343516
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614144}
[modify] https://crrev.com/25754cd4d3832e74532b6e67eea0eae260869900/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[modify] https://crrev.com/25754cd4d3832e74532b6e67eea0eae260869900/chrome/browser/extensions/chrome_content_browser_client_extensions_part.h
[modify] https://crrev.com/25754cd4d3832e74532b6e67eea0eae260869900/chrome/browser/extensions/chrome_content_browser_client_extensions_part_unittest.cc
[modify] https://crrev.com/25754cd4d3832e74532b6e67eea0eae260869900/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[modify] https://crrev.com/25754cd4d3832e74532b6e67eea0eae260869900/content/browser/child_process_security_policy_impl.h
[modify] https://crrev.com/25754cd4d3832e74532b6e67eea0eae260869900/content/public/browser/child_process_security_policy.h

Sign in to add a comment