Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(145)

Issue 1209393002: Move more code behind IsSwappedOutStateForbidden() checks. (Closed)

Created:
5 years, 6 months ago by nasko
Modified:
5 years, 5 months ago
Reviewers:
ncarter (slow), alexmos
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, site-isolation-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Move more code behind IsSwappedOutStateForbidden() checks. BUG=357747 Committed: https://crrev.com/ba9691626c0f736844efe4c2b64f289e5438c0bb Cr-Commit-Position: refs/heads/master@{#336449}

Patch Set 1 #

Total comments: 7
Unified diffs Side-by-side diffs Delta from patch set Stats (+34 lines, -27 lines) Patch
M content/browser/frame_host/render_frame_host_manager.cc View 2 chunks +4 lines, -6 lines 3 comments Download
M content/renderer/render_frame_impl.cc View 7 chunks +27 lines, -19 lines 4 comments Download
M content/renderer/render_view_impl.cc View 1 chunk +3 lines, -2 lines 0 comments Download

Messages

Total messages: 12 (3 generated)
nasko
Hey Nick, Can you review this CL for me? It expands you idea of gating ...
5 years, 6 months ago (2015-06-26 14:50:43 UTC) #2
ncarter (slow)
lgtm I was actually about to send you substantially exactly the same diffs, as part ...
5 years, 6 months ago (2015-06-26 20:27:17 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1209393002/1
5 years, 6 months ago (2015-06-26 20:27:34 UTC) #5
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 6 months ago (2015-06-26 20:33:13 UTC) #6
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/ba9691626c0f736844efe4c2b64f289e5438c0bb Cr-Commit-Position: refs/heads/master@{#336449}
5 years, 6 months ago (2015-06-26 20:34:03 UTC) #7
nasko
https://codereview.chromium.org/1209393002/diff/1/content/browser/frame_host/render_frame_host_manager.cc File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1209393002/diff/1/content/browser/frame_host/render_frame_host_manager.cc#newcode889 content/browser/frame_host/render_frame_host_manager.cc:889: if (!IsSwappedOutStateForbidden()) On 2015/06/26 20:27:17, ncarter wrote: > I ...
5 years, 5 months ago (2015-06-29 08:06:20 UTC) #8
alexmos
https://codereview.chromium.org/1209393002/diff/1/content/browser/frame_host/render_frame_host_manager.cc File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1209393002/diff/1/content/browser/frame_host/render_frame_host_manager.cc#newcode889 content/browser/frame_host/render_frame_host_manager.cc:889: if (!IsSwappedOutStateForbidden()) On 2015/06/29 08:06:20, nasko (paris) wrote: > ...
5 years, 5 months ago (2015-06-29 14:18:05 UTC) #10
nasko
On 2015/06/29 14:18:05, alexmos wrote: > https://codereview.chromium.org/1209393002/diff/1/content/browser/frame_host/render_frame_host_manager.cc > File content/browser/frame_host/render_frame_host_manager.cc (right): > > https://codereview.chromium.org/1209393002/diff/1/content/browser/frame_host/render_frame_host_manager.cc#newcode889 > ...
5 years, 5 months ago (2015-06-29 14:58:30 UTC) #11
alexmos
5 years, 5 months ago (2015-06-30 03:27:11 UTC) #12
Message was sent while issue was closed.
On 2015/06/29 14:58:30, nasko (paris) wrote:
> On 2015/06/29 14:18:05, alexmos wrote:
> >
>
https://codereview.chromium.org/1209393002/diff/1/content/browser/frame_host/...
> > File content/browser/frame_host/render_frame_host_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/1209393002/diff/1/content/browser/frame_host/...
> > content/browser/frame_host/render_frame_host_manager.cc:889: if
> > (!IsSwappedOutStateForbidden())
> > On 2015/06/29 08:06:20, nasko (paris) wrote:
> > > On 2015/06/26 20:27:17, ncarter wrote:
> > > > I didn't realize this one fall under the SwappedOutForbidden() umbrella.
> > > > 
> > > > If we do this here, don't we need to do it for OnDidUpdateName too? They
> > seem
> > > to
> > > > be following the same pattern.
> > > 
> > > I'd consult Alex on that one, as he is working through naming and opener
> > chains
> > > right now.
> > 
> > Only if we also update this one:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> > 
> > I don't see how that's related to swapped-out removal though.  Without
> > --site-per-process, name updates are likely already broken today (if a
> > cross-process window updates its window.name, I don't see how other windows
in
> > the BrowsingInstance can reach it via its new window.name without this). 
But
> > fixing it may not be straightforward, as there were also past issues with
perf
> > benchmarks because of these updates (see comment where we send the
> > FrameHostMsg_DidChangeName).  So it seems we should resolve these separately
> > from swapped-out removal.
> 
> Yes, things might be broken today, but that doesn't mean they have to stay
> broken, right? We need to fix it at some point, but I do think it can be
> independent of swapped-out removal.

Yeah, agreed that we should fix it. I was just concerned that it may have other
complications 
and should be fixed without being tied to swapped-out if possible.   Sounds like
we agree. :)

> 
> > For that matter, Nasko, why this one is needed for swapped-out removal too? 
> I'm
> > guessing the latest top-level RemoteFrame origin was needed somewhere, but
> just
> > curious where.
> 
> window.location :) or anything that performs a check whether one frame can
> access another frame based on the origin.

I guess I was curious where we could get away with just frame-creation-time
origin replication as opposed to the origin updates that are sent here.  I.e.,
many checks might behave the same if a RemoteFrame is origin B vs C as long as
it's cross-origin from the checking frame A.  Origin updates do matter for
things like location.ancestorOrigins or error messages.  I guess it mattered for
window.location too?

Powered by Google App Engine
This is Rietveld 408576698