Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Origin isolation: a new strategy for window.originIsolationRestricted" #24841

Merged
merged 1 commit into from Aug 1, 2020

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

This reverts commit 7ac0b9ed68d57270b680e39b9e21e1ede1b8c774.

Reason for revert: Possible cause of flaky navigation/error-related test failures on https://ci.chromium.org/p/chromium/builders/ci/Linux%20Tests and https://ci.chromium.org/p/chromium/builders/ci/Mac10.10%20Tests, see e.g. https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8873201719841161216/+/steps/content_browsertests_on__none__GPU_on_Mac_on_Mac-10.10/0/logs/Flaky_failure:_All__x2f_NavigationControllerBrowserTest.ErrorPageReplacement__x2f_0__status_CRASH_SUCCESS_/0 and https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8873205275892139456/+/steps/storage_service_unsandboxed_content_browsertests/0/logs/Deterministic_failure:_All__x2f_SitePerProcessIgnoreCertErrorsBrowserTest.SubresourceWithCertificateErrors__x2f_0__status_FAILURE_/0.

If this revert does not help, I will reland.
Original change's description:

Origin isolation: a new strategy for window.originIsolationRestricted

In https://chromium-review.googlesource.com/c/chromium/src/+/2243994 I
introduced an implementation for window.originIsolationRestricted which
pipes the isolation state from NavigationRequest to the navigated-to
LocalDOMWindow. However, this does not work for the case of the initial
about:blank, where no navigation is performed. Furthermore, it does not
match the spec, where the Window property just reflects an agent
cluster-wide property.

This CL introduces an alternate approach, more similar to what is done
for self.crossOriginIsolated in
https://chromium-review.googlesource.com/c/chromium/src/+/2247463, which
is another agent cluster-wide value. The origin isolation state is
stored in the renderer-side Agent class. Then the LocalDOMWindow getter
can just pick it up from the surrounding agent, as in the spec. Note
that unlike the implementation for self.crossOriginIsolated, the value
is per-Agent instead of static (process-wide).

Currently the value is set several times per agent (roughly once on
every navigation). This is redundant, but we don't yet have a good place
to set it once (i.e., we don't have a browser-side "time of agent
creation"). If that gets fixed, we can likely stop piping the value
through navigation params. See
https://docs.google.com/document/d/1MTnmyWAoAIKDH4yWaRthIUdi05MsjlML8gctvKP7-h8/edit
for discussions around fixing that.

This fixes the issue with about:blank iframes embedded in origin-isolated
pages reporting false, because the agent's origin-isolated boolean was
previously set to true by the containing frame.

This does not yet fix the issue with data: URLs reporting false, tested in
external/wpt/origin-isolation/getter-special-cases/data-url.https.html.
However, that will be doable as a followup, by changing the
navigation-time computation to pass true for them instead of false.
(Currently it passes false because data: URLs don't get their own
process, but it should pass true because they do get their own agent
cluster.)

Bug: 1095653
Change-Id: I8dfa8fc4a4766efc0611d43a255673662c422776
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2300237
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#793799}

TBR=dcheng@chromium.org,creis@chromium.org,alexmos@chromium.org,domenic@chromium.org,wjmaclean@chromium.org

Change-Id: Ia9174b00ac61178cff9bf24801182d23779399c5
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1095653
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2333651
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#793885}

…stricted"

This reverts commit 7ac0b9ed68d57270b680e39b9e21e1ede1b8c774.

Reason for revert: Possible cause of flaky navigation/error-related test failures on https://ci.chromium.org/p/chromium/builders/ci/Linux%20Tests and https://ci.chromium.org/p/chromium/builders/ci/Mac10.10%20Tests, see e.g. https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8873201719841161216/+/steps/content_browsertests_on__none__GPU_on_Mac_on_Mac-10.10/0/logs/Flaky_failure:_All__x2f_NavigationControllerBrowserTest.ErrorPageReplacement__x2f_0__status_CRASH_SUCCESS_/0 and https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8873205275892139456/+/steps/storage_service_unsandboxed_content_browsertests/0/logs/Deterministic_failure:_All__x2f_SitePerProcessIgnoreCertErrorsBrowserTest.SubresourceWithCertificateErrors__x2f_0__status_FAILURE_/0.

If this revert does not help, I will reland.
Original change's description:
> Origin isolation: a new strategy for window.originIsolationRestricted
>
> In https://chromium-review.googlesource.com/c/chromium/src/+/2243994 I
> introduced an implementation for window.originIsolationRestricted which
> pipes the isolation state from NavigationRequest to the navigated-to
> LocalDOMWindow. However, this does not work for the case of the initial
> about:blank, where no navigation is performed. Furthermore, it does not
> match the spec, where the Window property just reflects an agent
> cluster-wide property.
>
> This CL introduces an alternate approach, more similar to what is done
> for self.crossOriginIsolated in
> https://chromium-review.googlesource.com/c/chromium/src/+/2247463, which
> is another agent cluster-wide value. The origin isolation state is
> stored in the renderer-side Agent class. Then the LocalDOMWindow getter
> can just pick it up from the surrounding agent, as in the spec. Note
> that unlike the implementation for self.crossOriginIsolated, the value
> is per-Agent instead of static (process-wide).
>
> Currently the value is set several times per agent (roughly once on
> every navigation). This is redundant, but we don't yet have a good place
> to set it once (i.e., we don't have a browser-side "time of agent
> creation"). If that gets fixed, we can likely stop piping the value
> through navigation params. See
> https://docs.google.com/document/d/1MTnmyWAoAIKDH4yWaRthIUdi05MsjlML8gctvKP7-h8/edit
> for discussions around fixing that.
>
> This fixes the issue with about:blank iframes embedded in origin-isolated
> pages reporting false, because the agent's origin-isolated boolean was
> previously set to true by the containing frame.
>
> This does not yet fix the issue with data: URLs reporting false, tested in
> external/wpt/origin-isolation/getter-special-cases/data-url.https.html.
> However, that will be doable as a followup, by changing the
> navigation-time computation to pass true for them instead of false.
> (Currently it passes false because data: URLs don't get their own
> process, but it should pass true because they _do_ get their own agent
> cluster.)
>
> Bug: 1095653
> Change-Id: I8dfa8fc4a4766efc0611d43a255673662c422776
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2300237
> Commit-Queue: Domenic Denicola <domenic@chromium.org>
> Reviewed-by: Charlie Reis <creis@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#793799}

TBR=dcheng@chromium.org,creis@chromium.org,alexmos@chromium.org,domenic@chromium.org,wjmaclean@chromium.org

Change-Id: Ia9174b00ac61178cff9bf24801182d23779399c5
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1095653
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2333651
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#793885}
Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review process for this patch is being conducted in the Chromium project.

@chromium-wpt-export-bot chromium-wpt-export-bot merged commit 43899f5 into master Aug 1, 2020
@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-d0f75cc6ec branch August 1, 2020 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants