|
|
Created:
5 years, 6 months ago by nasko Modified:
5 years, 5 months ago 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. |
DescriptionMove 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
Messages
Total messages: 12 (3 generated)
nasko@chromium.org changed reviewers: + nick@chromium.org
Hey Nick, Can you review this CL for me? It expands you idea of gating swapped out behind a method call to a few more places. If it looks fine, feel free to send it to the CQ. Thanks in advance! Nasko
The CQ bit was checked by nick@chromium.org
lgtm I was actually about to send you substantially exactly the same diffs, as part of the larger https://codereview.chromium.org/1208143002/#ps80001, and had debating splitting them off into a smaller CL like this. So, thanks! I have a couple nits, but none of them are functional. Since our changes will be in conflict, I'll just lgtm + cq this and roll the nit fixes into my own CL. 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()) 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. https://codereview.chromium.org/1209393002/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1209393002/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:511: bool IsSwappedOutStateForbidden() { I don't like having IsSwappedOutStateForbidden on both RenderFrameProxy and RenderFrameImpl. I'll consolidate them in my change. https://codereview.chromium.org/1209393002/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:702: switches::kSitePerProcess)) { I think you meant to remove this command line check.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1209393002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/ba9691626c0f736844efe4c2b64f289e5438c0bb Cr-Commit-Position: refs/heads/master@{#336449}
Message was sent while issue was closed.
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/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. https://codereview.chromium.org/1209393002/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1209393002/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:511: bool IsSwappedOutStateForbidden() { On 2015/06/26 20:27:17, ncarter wrote: > I don't like having IsSwappedOutStateForbidden on both RenderFrameProxy and > RenderFrameImpl. I'll consolidate them in my change. Acknowledged. https://codereview.chromium.org/1209393002/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:702: switches::kSitePerProcess)) { On 2015/06/26 20:27:17, ncarter wrote: > I think you meant to remove this command line check. D'oh!
Message was sent while issue was closed.
alexmos@chromium.org changed reviewers: + alexmos@chromium.org
Message was sent while issue was closed.
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. 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.
Message was sent while issue was closed.
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. > 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.
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? |