New issue
Advanced search Search tips

Issue 788837 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug

Blocking:
issue 780133



Sign in to add a comment

CWS doesn't work with --isolate-origins=https://google.com

Project Member Reported by creis@chromium.org, Nov 27 2017

Issue description

Chrome Version: 64.0.3278.0
OS: Win10

What steps will reproduce the problem?
(1) Start Chrome with --isolate-origins=https://google.com
(2) Visit https://chrome.google.com/webstore

What is the expected result?
The navigation succeeds and you're able to install extensions/

What happens instead?
The navigation never completes.

This may be because the isolated origins logic is taking precedence over the CWS process isolation?  Maybe we can fix in a similar way as the remote NTP in  issue 755595 ?
 
Cc: lukasza@chromium.org
Status: Started (was: Assigned)
I investigated this, and it's indeed very similar to  issue 755595 .  We aren't resolving the effective URL for a CWS URL if it corresponds to an isolated origin, which leads us to never put the CWS process into the extensions ProcessMap in SiteInstanceGotProcess.  Later, that causes us to kill the CWS renderer via CanCommitURL, since we think a non-CWS process is committing a CWS URL.

I think the fix is very similar to  issue 755595 , essentially always resolving CWS URLs to the right effective URLs, while letting isolated origins take precedence over all other hosted apps.  I've got a CL in progress.
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 28 2017

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

commit ca171375eb03132a47f1a986343f77be726c5511
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Tue Nov 28 16:11:31 2017

Fix Chrome Web Store loading in an isolated origin.

When a hosted app URL also corresponds to an isolated origin, the
isolated origin takes precedence, and the corresponding SiteInstance
won't use the effective URL for the hosted app.  However, this logic
needs to exclude CWS, which still needs to resolve to its effective
URL, so that the corresponding process ends up in the ProcessMap for
the CWS extension ID.  Otherwise, security checks such as CanCommitURL
won't allow CWS navigations to succeed.

Bug:  788837 
Change-Id: I2b8d03d044e72bb9b8f71cb4c3accfba8d907ac4
Reviewed-on: https://chromium-review.googlesource.com/792596
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@{#519701}
[modify] https://crrev.com/ca171375eb03132a47f1a986343f77be726c5511/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/ca171375eb03132a47f1a986343f77be726c5511/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[modify] https://crrev.com/ca171375eb03132a47f1a986343f77be726c5511/chrome/browser/extensions/chrome_content_browser_client_extensions_part.h
[modify] https://crrev.com/ca171375eb03132a47f1a986343f77be726c5511/chrome/browser/extensions/process_management_browsertest.cc

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

Blocking: 780133
I've just verified that this is fixed in Mac Canary 64.0.3280.0 - I verified that I can load CWS with --isolate-origins=https://google.com/ and successfully install an extension.  Requesting merge of r519701 to M63, as this is a blocker for  issue 760761 , which needs to go into M63.  Note that this will need to be merged together with the fix for  issue 755595 , which a similarly important blocker for  issue 760761 , so I'll also request a merge there.  Both are fairly short fixes and should be safe to merge.
Labels: Merge-Request-63
Actually adding the label this time.
Project Member

Comment 6 by sheriffbot@chromium.org, Nov 29 2017

Labels: -Merge-Request-63 Merge-Review-63 Hotlist-Merge-Review
This bug requires manual review: We are only 5 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Just to give a bit more detail on #4:

- The fixed codepaths only affect users of the --isolate-origins flag (and in particular specifying https://google.com/ or other URLs corresponding to remote NTP), which nobody should be using right now, so extra coverage on dev/beta wouldn't help.

- The fixes are small and well-contained, so should be low-risk.

- The issues fixed are deal-breakers for specifying a subset of isolated sites via enterprise policy ( issue 760761 ).  If the isolated origins policy includes https://google.com, not having those fixes would break the NTP and Chrome Web Store for everyone.   Issue 760761  is targeted for M63. 

Comment 8 by gov...@chromium.org, Nov 29 2017

Labels: -Merge-Review-63 Merge-Approved-63
Approving merges listed at #4 to M63 branch 3239 based on comments #4, #7 and per offline chat with  alexmos@ & creis@. 
Project Member

Comment 9 by bugdroid1@chromium.org, Nov 29 2017

Labels: -merge-approved-63 merge-merged-3239
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9ce7b41db04249696760719f95c2ede605095147

commit 9ce7b41db04249696760719f95c2ede605095147
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Wed Nov 29 23:13:39 2017

Fix Chrome Web Store loading in an isolated origin. (Merge to M63)

When a hosted app URL also corresponds to an isolated origin, the
isolated origin takes precedence, and the corresponding SiteInstance
won't use the effective URL for the hosted app.  However, this logic
needs to exclude CWS, which still needs to resolve to its effective
URL, so that the corresponding process ends up in the ProcessMap for
the CWS extension ID.  Otherwise, security checks such as CanCommitURL
won't allow CWS navigations to succeed.

TBR=alexmos@chromium.org

(cherry picked from commit ca171375eb03132a47f1a986343f77be726c5511)

Bug:  788837 
Change-Id: I2b8d03d044e72bb9b8f71cb4c3accfba8d907ac4
Reviewed-on: https://chromium-review.googlesource.com/792596
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@{#519701}
Reviewed-on: https://chromium-review.googlesource.com/798221
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#612}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/9ce7b41db04249696760719f95c2ede605095147/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/9ce7b41db04249696760719f95c2ede605095147/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[modify] https://crrev.com/9ce7b41db04249696760719f95c2ede605095147/chrome/browser/extensions/chrome_content_browser_client_extensions_part.h
[modify] https://crrev.com/9ce7b41db04249696760719f95c2ede605095147/chrome/browser/extensions/process_management_browsertest.cc

Status: Fixed (was: Started)

Sign in to add a comment