Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2739193004:
Check for already existing entry when adding to SiteProcessMap. (Closed)
DescriptionCheck for already existing entry when adding to SiteProcessMap.
The SiteProcessMap::RegisterProcess can overwrite existing entries
in the map, which can lead to incorrect process being mapped to
a site in process-per-site mode. This CL checks for existence and
avoids the overwrite.
BUG=700514
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2739193004
Cr-Commit-Position: refs/heads/master@{#456920}
Committed: https://chromium.googlesource.com/chromium/src/+/0e9d619f88ffabc3c237cbdf75603df8af9925e6
Patch Set 1 #Patch Set 2 : Check for value equality if key exists. #Patch Set 3 : Fix. #
Total comments: 19
Patch Set 4 : Remove the CHECK for now, will add in separate CL. #Patch Set 5 : Fixes based on Devlin's review. #
Total comments: 6
Patch Set 6 : More fixes based on reviews. #
Total comments: 8
Patch Set 7 : Add extra comment. #Messages
Total messages: 54 (40 generated)
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Fix.
Description was changed from ========== Check for already existing entry when adding to SiteProcessMap. BUG= ========== to ========== Check for already existing entry when adding to SiteProcessMap. The SiteProcessMap::RegisterProcess can overwrite existing entries in the map, which can lead to incorrect process being mapped to a site in process-per-site mode. This CL checks for existence and avoids the overwrite. BUG=700514 ==========
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nasko@chromium.org changed reviewers: + creis@chromium.org, rdevlin.cronin@chromium.org
Hey Charlie and Devlin, Can you review this CL for me? Devlin, the extension I had to write is pretty trivial, but also has an extra html file that needs to be present to be blocked. Does that make sense to create the extension in C++ code? Thanks in advance! Nasko
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks for tracking this down!! :) https://codereview.chromium.org/2739193004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/process_management_browsertest.cc (right): https://codereview.chromium.org/2739193004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:404: // Load an extension, which will block a request for a specifig page in it. *specific https://codereview.chromium.org/2739193004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:423: content::TestNavigationObserver observer(web_contents); Doesn't NavigateToURL by default block until navigation is complete? Also, from my (more limited) investigation, it seemed like if the extension had a background page, this wouldn't have failed (because the site instance would have been created). It might be good to mention that somewhere, since normally we'd have this handled by the background page. https://codereview.chromium.org/2739193004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:436: web_contents, "chrome.tabs.create({url: 'about:blank'});")); Does this have to be done through the extension API? It might be clearer to just use e.g. NavigateToURL() with disposition of NEW_[FOREGROUND|BACKGROUND]_TAB. https://codereview.chromium.org/2739193004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:441: if (new_web_contents->GetLastCommittedURL() != GURL(url::kAboutBlankURL)) { The above might also solve this, since NavigateToURL should block until complete. https://codereview.chromium.org/2739193004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:453: EXPECT_TRUE(content::ExecuteScript(new_web_contents, script)); Same question - does this need to be through ExecuteScript? If so (possible), why? (extend comments) If not, prefer NavigateToURL(). https://codereview.chromium.org/2739193004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:462: // extensions use the process-per-site model, each extension URL is registered nit: process-per-extension? process-per-origin? Does "site" have a defined meaning? https://codereview.chromium.org/2739193004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:468: extension->GetResourceURL("")); nit: extension->url() https://codereview.chromium.org/2739193004/diff/40001/chrome/test/data/extens... File chrome/test/data/extensions/web_request_blocking/manifest.json (right): https://codereview.chromium.org/2739193004/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/web_request_blocking/manifest.json:6: "permissions": ["webRequest", "webRequestBlocking", "tabs", "<all_urls>"], is tabs needed? https://codereview.chromium.org/2739193004/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2739193004/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:338: CHECK_EQ(i->second, process); Looks like this CHECK is failing on bots :(
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:80001) has been deleted
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Devlin, can you take another look? I've fixed all of the review comments. https://codereview.chromium.org/2739193004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/process_management_browsertest.cc (right): https://codereview.chromium.org/2739193004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:404: // Load an extension, which will block a request for a specifig page in it. On 2017/03/11 01:52:17, Devlin wrote: > *specific Done. https://codereview.chromium.org/2739193004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:423: content::TestNavigationObserver observer(web_contents); On 2017/03/11 01:52:17, Devlin wrote: > Doesn't NavigateToURL by default block until navigation is complete? Good point :). I had so many iterations of this code that required an observer that forgot to remove it at the end. > Also, from my (more limited) investigation, it seemed like if the extension had > a background page, this wouldn't have failed (because the site instance would > have been created). It might be good to mention that somewhere, since normally > we'd have this handled by the background page. I don't know if background pages are accounted differently, but SiteInstance is already created by this navigation. I can experiment with background pages as a follow up. https://codereview.chromium.org/2739193004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:436: web_contents, "chrome.tabs.create({url: 'about:blank'});")); On 2017/03/11 01:52:18, Devlin wrote: > Does this have to be done through the extension API? It might be clearer to > just use e.g. NavigateToURL() with disposition of > NEW_[FOREGROUND|BACKGROUND]_TAB. I was trying to mimic as closely what I observed. It is a good point that these should be equivalent. https://codereview.chromium.org/2739193004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:441: if (new_web_contents->GetLastCommittedURL() != GURL(url::kAboutBlankURL)) { On 2017/03/11 01:52:17, Devlin wrote: > The above might also solve this, since NavigateToURL should block until > complete. Done. https://codereview.chromium.org/2739193004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:453: EXPECT_TRUE(content::ExecuteScript(new_web_contents, script)); On 2017/03/11 01:52:17, Devlin wrote: > Same question - does this need to be through ExecuteScript? If so (possible), > why? (extend comments) If not, prefer NavigateToURL(). This needs to be a renderer initiated navigation and NavigateToURL is browser-initiated. I have a comment about it on the redirect_url declaration, but I can add a bit more here too. https://codereview.chromium.org/2739193004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:462: // extensions use the process-per-site model, each extension URL is registered On 2017/03/11 01:52:17, Devlin wrote: > nit: process-per-extension? process-per-origin? Does "site" have a defined > meaning? Our codebase and docs already talk about process-per-site process model, so I'd rather use that instead of inventing new name for it. https://www.chromium.org/developers/design-documents/process-models https://cs.chromium.org/chromium/src/content/public/browser/site_instance.h?r... https://codereview.chromium.org/2739193004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:468: extension->GetResourceURL("")); On 2017/03/11 01:52:17, Devlin wrote: > nit: extension->url() Done. https://codereview.chromium.org/2739193004/diff/40001/chrome/test/data/extens... File chrome/test/data/extensions/web_request_blocking/manifest.json (right): https://codereview.chromium.org/2739193004/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/web_request_blocking/manifest.json:6: "permissions": ["webRequest", "webRequestBlocking", "tabs", "<all_urls>"], On 2017/03/11 01:52:18, Devlin wrote: > is tabs needed? Yes, it was used by tabs.create API call made in the test. I've removed it after fixing the test. https://codereview.chromium.org/2739193004/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2739193004/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:338: CHECK_EQ(i->second, process); On 2017/03/11 01:52:18, Devlin wrote: > Looks like this CHECK is failing on bots :( Yes, I'm purposefully hitting it with my test too :). It was there to see how broken things are. I will submit this CL without it and follow up with a DumpWithoutCrashing, so we can understand whether this happens in practice.
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2739193004/diff/40001/chrome/test/data/extens... File chrome/test/data/extensions/web_request_blocking/manifest.json (right): https://codereview.chromium.org/2739193004/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/web_request_blocking/manifest.json:6: "permissions": ["webRequest", "webRequestBlocking", "tabs", "<all_urls>"], On 2017/03/13 17:22:03, nasko (slow) wrote: > On 2017/03/11 01:52:18, Devlin wrote: > > is tabs needed? > > Yes, it was used by tabs.create API call made in the test. I've removed it after > fixing the test. Fun fact: tabs.create doesn't require the tabs permission. https://codereview.chromium.org/2739193004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/process_management_browsertest.cc (right): https://codereview.chromium.org/2739193004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/process_management_browsertest.cc:406: LoadExtension(test_data_dir_.AppendASCII("web_request_blocking")); nit: can we name the new extension something slightly different? This is eerily similar to e.g. webrequest/test_blocking. https://codereview.chromium.org/2739193004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/process_management_browsertest.cc:426: content::WindowedNotificationObserver tab_added_observer( Is this observer still needed?
Hmm, I have a concern about this fix below. (Overall, very nice investigation and test!) https://codereview.chromium.org/2739193004/diff/100001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2739193004/diff/100001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:336: SiteToProcessMap::iterator i = map_.find(site); This should have a comment, since at first glance it's not obvious why this can happen or what the right thing to do is (e.g., ignore, overwrite, or crash). And actually, that makes me realize there may be a problem here. Thanks for the detailed explanation of the scenario in https://crbug.com/700514#c1, which helps a lot. Suppose in that case, we left out steps 1 and 2 and someone else (perhaps the user) navigated to the blocked URL in a blank SiteInstance. Wouldn't that mean we'd register the blocked error page's SiteInstance in this map, preventing a real extension process from being registered later? (Or would real extension pages get forced into this error page SiteInstance, which might lack bindings and/or be sharing a process with web pages?) It seems like the real problem here is that we're treating the error URL as the SiteInstance's site URL. I know we want to change the committed URL for blocked navigations at some point after this CL and https://codereview.chromium.org/2738643002/, but I wonder if we can do a better job fixing this aspect now. For example, maybe the blank SiteInstance's site URL shouldn't be derived from the "committed" URL if it was an unsuccessful navigation? (If we did go that route, we'd probably still want to decide what the right behavior here is, whether's it's NOTREACHED, ignore, or overwrite. I suppose we can also get here if we race two blank SiteInstances to the same site?)
Description was changed from ========== Check for already existing entry when adding to SiteProcessMap. The SiteProcessMap::RegisterProcess can overwrite existing entries in the map, which can lead to incorrect process being mapped to a site in process-per-site mode. This CL checks for existence and avoids the overwrite. BUG=700514 ========== to ========== Check for already existing entry when adding to SiteProcessMap. The SiteProcessMap::RegisterProcess can overwrite existing entries in the map, which can lead to incorrect process being mapped to a site in process-per-site mode. This CL checks for existence and avoids the overwrite. BUG=700514 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Patchset #6 (id:120001) has been deleted
https://codereview.chromium.org/2739193004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/process_management_browsertest.cc (right): https://codereview.chromium.org/2739193004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/process_management_browsertest.cc:406: LoadExtension(test_data_dir_.AppendASCII("web_request_blocking")); On 2017/03/13 19:19:56, Devlin wrote: > nit: can we name the new extension something slightly different? This is eerily > similar to e.g. webrequest/test_blocking. Done. https://codereview.chromium.org/2739193004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/process_management_browsertest.cc:426: content::WindowedNotificationObserver tab_added_observer( On 2017/03/13 19:19:56, Devlin wrote: > Is this observer still needed? Probably not. Waiting for commit should be guaranteed to wait long enough for the WebContents to be added to the tabstrip. Removed. https://codereview.chromium.org/2739193004/diff/100001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2739193004/diff/100001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:336: SiteToProcessMap::iterator i = map_.find(site); On 2017/03/13 19:57:30, Charlie Reis (slow) wrote: > This should have a comment, since at first glance it's not obvious why this can > happen or what the right thing to do is (e.g., ignore, overwrite, or crash). Comment added. > And actually, that makes me realize there may be a problem here. Thanks for the > detailed explanation of the scenario in https://crbug.com/700514#c1, which helps > a lot. > > Suppose in that case, we left out steps 1 and 2 and someone else (perhaps the > user) navigated to the blocked URL in a blank SiteInstance. Wouldn't that mean > we'd register the blocked error page's SiteInstance in this map, preventing a > real extension process from being registered later? (Or would real extension > pages get forced into this error page SiteInstance, which might lack bindings > and/or be sharing a process with web pages?) I think the latter will happen and we will put the extension in a process without bindings. It is the safer fallback though, so it shouldn't result in security bugs. > It seems like the real problem here is that we're treating the error URL as the > SiteInstance's site URL. I know we want to change the committed URL for blocked > navigations at some point after this CL and > https://codereview.chromium.org/2738643002/, but I wonder if we can do a better > job fixing this aspect now. > > For example, maybe the blank SiteInstance's site URL shouldn't be derived from > the "committed" URL if it was an unsuccessful navigation? Indeed. And as discussed, I'm adding code to not call SetSite on the empty SiteInstance in the case of an error page. > (If we did go that route, we'd probably still want to decide what the right > behavior here is, whether's it's NOTREACHED, ignore, or overwrite. I suppose we > can also get here if we race two blank SiteInstances to the same site?) I think a race can indeed cause us to get here. I think not overwriting is the approach easiest to reason about.
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! I think it's almost ready, depending on whether there are concrete problems in the case below. https://codereview.chromium.org/2739193004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/process_management_browsertest.cc (right): https://codereview.chromium.org/2739193004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/process_management_browsertest.cc:451: // the process is the same for all SiteInstances. Thanks for explaining, but I think this is missing the part saying what we're actually trying to verify. Maybe add: This allows us to verify that the site-to-process map for the extension hasn't been overwritten by the process of the blocked_url. https://codereview.chromium.org/2739193004/diff/140001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2739193004/diff/140001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:629: // error page. In that case, the SiteInstance can still be considered unused Leaving it unassigned for the error page is something we may want to revisit later (or possibly now?). It's not terrible, since the error page itself is under Chrome's control and shouldn't pose a risk to whatever ends up in the process next, but I do wonder what would happen if you went to an allowed extension URL next. Would we end up giving the process bindings? Would that result in a kill or a crash because of the mix of non-bindings and bindings in the same process? (For example, start with about:blank, then go to the blocked extension URL, getting here. We leave the site unassigned. Then go to a real extension URL-- do we stay in the same process? If so, do we crash when going back because the bindings on the NavigationEntry don't match the RFH?) If that is a problem, we could consider setting the site URL to data: for the error page, so that it isn't reused later for the next navigation.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2739193004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/process_management_browsertest.cc (right): https://codereview.chromium.org/2739193004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/process_management_browsertest.cc:446: // Very subtle check for content/ internal functionality :(. I was thinking more and more about this test and whether a follow up should be a content/ internal test that forces Chrome in --process-per-site mode and verifies the same properties hold. https://codereview.chromium.org/2739193004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/process_management_browsertest.cc:451: // the process is the same for all SiteInstances. On 2017/03/13 22:31:33, Charlie Reis (slow) wrote: > Thanks for explaining, but I think this is missing the part saying what we're > actually trying to verify. Maybe add: > > This allows us to verify that the site-to-process map for the extension hasn't > been overwritten by the process of the blocked_url. Done. https://codereview.chromium.org/2739193004/diff/140001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2739193004/diff/140001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:629: // error page. In that case, the SiteInstance can still be considered unused On 2017/03/13 22:31:33, Charlie Reis (slow) wrote: > Leaving it unassigned for the error page is something we may want to revisit > later (or possibly now?). It's not terrible, since the error page itself is > under Chrome's control and shouldn't pose a risk to whatever ends up in the > process next, but I do wonder what would happen if you went to an allowed > extension URL next. We correctly transfer it to the extension process :) > Would we end up giving the process bindings? Would that > result in a kill or a crash because of the mix of non-bindings and bindings in > the same process? The process is no longer used, so it goes away. > (For example, start with about:blank, then go to the blocked extension URL, > getting here. We leave the site unassigned. Then go to a real extension URL-- > do we stay in the same process? If so, do we crash when going back because the > bindings on the NavigationEntry don't match the RFH?) I tried renderer-initiated direct navigation to the extension allowed URL, renderer-initiated navigation to server that redirects to the allowed URL and browser-initiated navigation to the same URL. They all resulted in committing in the extension process with this CL in place. > If that is a problem, we could consider setting the site URL to data: for the > error page, so that it isn't reused later for the next navigation.
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks. LGTM if the case below isn't problematic. https://codereview.chromium.org/2739193004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/process_management_browsertest.cc (right): https://codereview.chromium.org/2739193004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/process_management_browsertest.cc:446: // Very subtle check for content/ internal functionality :(. On 2017/03/14 16:56:02, nasko (slow) wrote: > I was thinking more and more about this test and whether a follow up should be a > content/ internal test that forces Chrome in --process-per-site mode and > verifies the same properties hold. I like that idea. https://codereview.chromium.org/2739193004/diff/140001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2739193004/diff/140001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:629: // error page. In that case, the SiteInstance can still be considered unused On 2017/03/14 16:56:02, nasko (slow) wrote: > On 2017/03/13 22:31:33, Charlie Reis (slow) wrote: > > Leaving it unassigned for the error page is something we may want to revisit > > later (or possibly now?). It's not terrible, since the error page itself is > > under Chrome's control and shouldn't pose a risk to whatever ends up in the > > process next, but I do wonder what would happen if you went to an allowed > > extension URL next. > > We correctly transfer it to the extension process :) > > > Would we end up giving the process bindings? Would that > > result in a kill or a crash because of the mix of non-bindings and bindings in > > the same process? > > The process is no longer used, so it goes away. > > > (For example, start with about:blank, then go to the blocked extension URL, > > getting here. We leave the site unassigned. Then go to a real extension > URL-- > > do we stay in the same process? If so, do we crash when going back because > the > > bindings on the NavigationEntry don't match the RFH?) > > I tried renderer-initiated direct navigation to the extension allowed URL, > renderer-initiated navigation to server that redirects to the allowed URL and > browser-initiated navigation to the same URL. They all resulted in committing in > the extension process with this CL in place. > > > If that is a problem, we could consider setting the site URL to data: for the > > error page, so that it isn't reused later for the next navigation. Ok-- I'm glad you tested the redirect case, since I know that was needed in your browsertest. Maybe it's not an issue. I'm still a bit nervous about it, but hopefully our plans to change how error pages work can resolve this aspect down the road. One other case to test, and then I'll be happy. :) What happens if you visit blocked.html and get to the error page, do a renderer-initiated navigation to a web page (redirecting if needed, so that we stay in process), then change the onBeforeRequest listener in the extension to stop blocking, then go back? Would we end up loading blocked.html in the process that has now been used for web pages?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2739193004/diff/140001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2739193004/diff/140001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:629: // error page. In that case, the SiteInstance can still be considered unused On 2017/03/14 23:46:47, Charlie Reis (slow) wrote: > On 2017/03/14 16:56:02, nasko (slow) wrote: > > On 2017/03/13 22:31:33, Charlie Reis (slow) wrote: > > > Leaving it unassigned for the error page is something we may want to revisit > > > later (or possibly now?). It's not terrible, since the error page itself is > > > under Chrome's control and shouldn't pose a risk to whatever ends up in the > > > process next, but I do wonder what would happen if you went to an allowed > > > extension URL next. > > > > We correctly transfer it to the extension process :) > > > > > Would we end up giving the process bindings? Would that > > > result in a kill or a crash because of the mix of non-bindings and bindings > in > > > the same process? > > > > The process is no longer used, so it goes away. > > > > > (For example, start with about:blank, then go to the blocked extension URL, > > > getting here. We leave the site unassigned. Then go to a real extension > > URL-- > > > do we stay in the same process? If so, do we crash when going back because > > the > > > bindings on the NavigationEntry don't match the RFH?) > > > > I tried renderer-initiated direct navigation to the extension allowed URL, > > renderer-initiated navigation to server that redirects to the allowed URL and > > browser-initiated navigation to the same URL. They all resulted in committing > in > > the extension process with this CL in place. > > > > > If that is a problem, we could consider setting the site URL to data: for > the > > > error page, so that it isn't reused later for the next navigation. > > Ok-- I'm glad you tested the redirect case, since I know that was needed in your > browsertest. Maybe it's not an issue. I'm still a bit nervous about it, but > hopefully our plans to change how error pages work can resolve this aspect down > the road. > > One other case to test, and then I'll be happy. :) What happens if you visit > blocked.html and get to the error page, do a renderer-initiated navigation to a > web page (redirecting if needed, so that we stay in process), then change the > onBeforeRequest listener in the extension to stop blocking, then go back? Would > we end up loading blocked.html in the process that has now been used for web > pages? Commits as expected in the extension process :). However, I should note that changing the JS required the extension to be restarted, so it resulted in a new process. I tried making the JS dynamic, such that it will only block the first request and allow afterwards. That also resulted in the page to be put in the extension process.
The CQ bit was checked by nasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2739193004/#ps160001 (title: "Add extra comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1489537950726600,
"parent_rev": "fd0523d1fb0087eb86205242d06a8044a5107ceb", "commit_rev":
"0e9d619f88ffabc3c237cbdf75603df8af9925e6"}
Message was sent while issue was closed.
Description was changed from ========== Check for already existing entry when adding to SiteProcessMap. The SiteProcessMap::RegisterProcess can overwrite existing entries in the map, which can lead to incorrect process being mapped to a site in process-per-site mode. This CL checks for existence and avoids the overwrite. BUG=700514 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Check for already existing entry when adding to SiteProcessMap. The SiteProcessMap::RegisterProcess can overwrite existing entries in the map, which can lead to incorrect process being mapped to a site in process-per-site mode. This CL checks for existence and avoids the overwrite. BUG=700514 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2739193004 Cr-Commit-Position: refs/heads/master@{#456920} Committed: https://chromium.googlesource.com/chromium/src/+/0e9d619f88ffabc3c237cbdf7560... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001) as https://chromium.googlesource.com/chromium/src/+/0e9d619f88ffabc3c237cbdf7560...
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:160001) has been created in https://codereview.chromium.org/2749543004/ by tzik@chromium.org. The reason for reverting is: Speculative revert to see a bot failure. SubresourceFilterBrowserTest.FailedProvisionalLoadInMainframe got flaky around this CL, and this is relatively more suspicious in the failing range. BUG=701663. |
