Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1018163002:
[Extensions] Skip injecting scripts into remote frames with site isolation turned on. (Closed)
|
Created:
5 years, 9 months ago by not at google - send to devlin Modified:
5 years, 9 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Extensions] Skip injecting scripts into remote frames with site isolation turned on.
This is admitting defeat by site isolation, for now. It's better than crashing.
It will be properly fixed when permission checks are moved into the browser.
BUG=454917
R=rdevlin.cronin@chromium.org
Committed: https://crrev.com/dfebefe64349581cb675dcdf19861397a28d1252
Cr-Commit-Position: refs/heads/master@{#321254}
Patch Set 1 #
Total comments: 5
Messages
Total messages: 16 (4 generated)
kalman@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
lgtm. Bummer, but you're right that it's better than just crashing everything. https://codereview.chromium.org/1018163002/diff/1/extensions/renderer/user_sc... File extensions/renderer/user_script_set.cc (right): https://codereview.chromium.org/1018163002/diff/1/extensions/renderer/user_sc... extensions/renderer/user_script_set.cc:208: blink::WebDocument top_document = web_frame->top()->document(); nit: This seems like it should be one of the first (if not the first) checks. Why not put it at the start of the method?
https://codereview.chromium.org/1018163002/diff/1/extensions/renderer/user_sc... File extensions/renderer/user_script_set.cc (right): https://codereview.chromium.org/1018163002/diff/1/extensions/renderer/user_sc... extensions/renderer/user_script_set.cc:208: blink::WebDocument top_document = web_frame->top()->document(); (I'll defer to nasko that this is a suitable differentiator between OOPI and not)
https://codereview.chromium.org/1018163002/diff/1/extensions/renderer/user_sc... File extensions/renderer/user_script_set.cc (right): https://codereview.chromium.org/1018163002/diff/1/extensions/renderer/user_sc... extensions/renderer/user_script_set.cc:208: blink::WebDocument top_document = web_frame->top()->document(); On 2015/03/18 22:50:47, Devlin wrote: > (I'll defer to nasko that this is a suitable differentiator between OOPI and > not) Another check would be to see if the top frame is remote (isWebRemoteFrame()) - but I deliberately didn't do that. I can never remember if Remote is still Remote with site isolation, and this is all highly temporary, and either way it's a hack. https://codereview.chromium.org/1018163002/diff/1/extensions/renderer/user_sc... extensions/renderer/user_script_set.cc:208: blink::WebDocument top_document = web_frame->top()->document(); On 2015/03/18 22:49:31, Devlin wrote: > nit: This seems like it should be one of the first (if not the first) checks. > Why not put it at the start of the method? It's a hack, and the least interesting check here really. Only necessary because the next line doesn't work without it. No need to draw attention.
(to reiterate - this will only have an observable effect if --site-per-process is enabled)
kalman@chromium.org changed reviewers: - nasko@chromium.org
Moving nasko to CC, he's probably gone home, and I might as well submit this now. Nasko please complain if I did something wrong.
The CQ bit was checked by kalman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1018163002/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/dfebefe64349581cb675dcdf19861397a28d1252 Cr-Commit-Position: refs/heads/master@{#321254}
Message was sent while issue was closed.
nasko@chromium.org changed reviewers: + nasko@chromium.org
Message was sent while issue was closed.
Thanks for changing the code to avoid the crashing. I've left a comment with my thoughts on your discussion. https://codereview.chromium.org/1018163002/diff/1/extensions/renderer/user_sc... File extensions/renderer/user_script_set.cc (right): https://codereview.chromium.org/1018163002/diff/1/extensions/renderer/user_sc... extensions/renderer/user_script_set.cc:208: blink::WebDocument top_document = web_frame->top()->document(); On 2015/03/18 22:54:54, kalman wrote: > On 2015/03/18 22:49:31, Devlin wrote: > > nit: This seems like it should be one of the first (if not the first) checks. > > Why not put it at the start of the method? > > It's a hack, and the least interesting check here really. Only necessary because > the next line doesn't work without it. No need to draw attention. Remote frames cannot host any documents or content, therefore it doesn't make sense to do any script injection for those. The comment above the method name also mentions this. I would've checked if the top() frame is WebRemoteFrame and bailed out early in the function. The whole notion of accessing the document is flawed with remote frames, so this code looks fragile to me.
Message was sent while issue was closed.
On 2015/03/19 17:33:48, nasko wrote: > Thanks for changing the code to avoid the crashing. I've left a comment with my > thoughts on your discussion. > > https://codereview.chromium.org/1018163002/diff/1/extensions/renderer/user_sc... > File extensions/renderer/user_script_set.cc (right): > > https://codereview.chromium.org/1018163002/diff/1/extensions/renderer/user_sc... > extensions/renderer/user_script_set.cc:208: blink::WebDocument top_document = > web_frame->top()->document(); > On 2015/03/18 22:54:54, kalman wrote: > > On 2015/03/18 22:49:31, Devlin wrote: > > > nit: This seems like it should be one of the first (if not the first) > checks. > > > Why not put it at the start of the method? > > > > It's a hack, and the least interesting check here really. Only necessary > because > > the next line doesn't work without it. No need to draw attention. > > Remote frames cannot host any documents or content, therefore it doesn't make > sense to do any script injection for those. The comment above the method name > also mentions this. > > I would've checked if the top() frame is WebRemoteFrame and bailed out early in > the function. The whole notion of accessing the document is flawed with remote > frames, so this code looks fragile to me. So Remote frames turn into Local frames if --site-per-process isn't on, i.e. Remote frames can only ever exist with --site-per-process is on?
Message was sent while issue was closed.
On 2015/03/19 17:41:35, kalman wrote: > On 2015/03/19 17:33:48, nasko wrote: > > Thanks for changing the code to avoid the crashing. I've left a comment with > my > > thoughts on your discussion. > > > > > https://codereview.chromium.org/1018163002/diff/1/extensions/renderer/user_sc... > > File extensions/renderer/user_script_set.cc (right): > > > > > https://codereview.chromium.org/1018163002/diff/1/extensions/renderer/user_sc... > > extensions/renderer/user_script_set.cc:208: blink::WebDocument top_document = > > web_frame->top()->document(); > > On 2015/03/18 22:54:54, kalman wrote: > > > On 2015/03/18 22:49:31, Devlin wrote: > > > > nit: This seems like it should be one of the first (if not the first) > > checks. > > > > Why not put it at the start of the method? > > > > > > It's a hack, and the least interesting check here really. Only necessary > > because > > > the next line doesn't work without it. No need to draw attention. > > > > Remote frames cannot host any documents or content, therefore it doesn't make > > sense to do any script injection for those. The comment above the method name > > also mentions this. > > > > I would've checked if the top() frame is WebRemoteFrame and bailed out early > in > > the function. The whole notion of accessing the document is flawed with remote > > frames, so this code looks fragile to me. > > So Remote frames turn into Local frames if --site-per-process isn't on, i.e. > Remote frames can only ever exist with --site-per-process is on? I'd suggest avoiding thinking whether the flag is present or not. Just think of WebLocalFrame being a frame as you know it from before, where we have a document and scripts running. Then think of a WebRemoteFrame as a placeholder that can't do anything real, but we can ask it about certain properties of it. Chromium content/ manages the creation of those objects and transforming them from one object to another. Where the flag comes is that it is not currently possible to get WebRemoteFrame for subframes without it. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
