Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
10 years, 8 months ago by Matt Perry Modified:
9 years, 7 months ago Reviewers:
Charlie Reis, darin (slow to review) CC:
chromium-reviews Visibility:
Public. |
DescriptionForce extensions to run in their shared processes, even with --process-per-tab.
BUG=36617
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=45849
Patch Set 1 #
Total comments: 6
Patch Set 2 : force_swap #
Total comments: 4
Patch Set 3 : feedback #
Created: 10 years, 8 months ago
Messages
Total messages: 10 (0 generated)
http://codereview.chromium.org/1723016/diff/1/3 File chrome/browser/tab_contents/render_view_host_manager.cc (right): http://codereview.chromium.org/1723016/diff/1/3#newcode290 chrome/browser/tab_contents/render_view_host_manager.cc:290: entry.url().SchemeIs(chrome::kExtensionScheme); This feels like too much of a point fix to me. This method was more about the process model being used, and it's overridden in test_tab_contents.h to test various process models. (Incidentally, if we decide to keep the new signature, you'll need to update that version of it so the test still works.) I see why this is important for navigating to an extension page, so that we don't break the extension. Isn't it also important for the other cases in ShouldSwapProcessesForNavigation, though? Even when we're using --process-per-tab, we probably don't want to stay in the extension process when navigating away. Ditto for DOM UI renderers. I'll propose a fix below off the top of my head, but it might be worth thinking this through and writing some tests to show what we expect to happen in these cases. Alternatively, we could remove support for --process-per-tab, since an increasing number of features are relying on swapping a process within a tab. (It'd be nice to keep it if we can figure out a reasonable behavior for those features, though.) http://codereview.chromium.org/1723016/diff/1/3#newcode576 chrome/browser/tab_contents/render_view_host_manager.cc:576: SiteInstance* new_instance = curr_instance; Can we move the call to ShouldSwapProcessForNavigation (on line 581) to here, storing it as forceProcessSwap? Then we could include it in both of the if statements below, on lines 577 and 580. That means that ShouldSwapProcessForNavigation will take effect regardless of the process model, which might have been the original intent of that method.
http://codereview.chromium.org/1723016/diff/1/3 File chrome/browser/tab_contents/render_view_host_manager.cc (right): http://codereview.chromium.org/1723016/diff/1/3#newcode290 chrome/browser/tab_contents/render_view_host_manager.cc:290: entry.url().SchemeIs(chrome::kExtensionScheme); On 2010/04/27 20:53:48, creis wrote: > This feels like too much of a point fix to me. This method was more about the > process model being used, and it's overridden in test_tab_contents.h to test > various process models. (Incidentally, if we decide to keep the new signature, > you'll need to update that version of it so the test still works.) Actually it looks like the test_tab_contents.h override is obsolete. No one actually calls it, and it is overriding a non-existent method in TabContents, not this class. > > I see why this is important for navigating to an extension page, so that we > don't break the extension. Isn't it also important for the other cases in > ShouldSwapProcessesForNavigation, though? Even when we're using > --process-per-tab, we probably don't want to stay in the extension process when > navigating away. Ditto for DOM UI renderers. Good point. Hmm.. > I'll propose a fix below off the top of my head, but it might be worth thinking > this through and writing some tests to show what we expect to happen in these > cases. Alternatively, we could remove support for --process-per-tab, since an > increasing number of features are relying on swapping a process within a tab. > (It'd be nice to keep it if we can figure out a reasonable behavior for those > features, though.) I'm somewhat inclined to removing support for process-per-tab. If its original purpose was to experiment with different types of process grouping, I think it has outlived that purpose. We've settled on process-per-site for a while, and the other options just complicate the code. What do you think?
On 2010/04/27 21:07:58, Matt Perry wrote: > http://codereview.chromium.org/1723016/diff/1/3 > File chrome/browser/tab_contents/render_view_host_manager.cc (right): > > http://codereview.chromium.org/1723016/diff/1/3#newcode290 > chrome/browser/tab_contents/render_view_host_manager.cc:290: > entry.url().SchemeIs(chrome::kExtensionScheme); > On 2010/04/27 20:53:48, creis wrote: > > This feels like too much of a point fix to me. This method was more about the > > process model being used, and it's overridden in test_tab_contents.h to test > > various process models. (Incidentally, if we decide to keep the new > signature, > > you'll need to update that version of it so the test still works.) > > Actually it looks like the test_tab_contents.h override is obsolete. No one > actually calls it, and it is overriding a non-existent method in TabContents, > not this class. > Ack-- that might mean we're not testing what we think we are. I'll take a look and file a bug if it's the wrong behavior. (If we decide to remove --process-per-tab, it might not matter.) > > > > I see why this is important for navigating to an extension page, so that we > > don't break the extension. Isn't it also important for the other cases in > > ShouldSwapProcessesForNavigation, though? Even when we're using > > --process-per-tab, we probably don't want to stay in the extension process > when > > navigating away. Ditto for DOM UI renderers. > > Good point. Hmm.. > > > I'll propose a fix below off the top of my head, but it might be worth > thinking > > this through and writing some tests to show what we expect to happen in these > > cases. Alternatively, we could remove support for --process-per-tab, since an > > increasing number of features are relying on swapping a process within a tab. > > (It'd be nice to keep it if we can figure out a reasonable behavior for those > > features, though.) > > I'm somewhat inclined to removing support for process-per-tab. If its original > purpose was to experiment with different types of process grouping, I think it > has outlived that purpose. We've settled on process-per-site for a while, and > the other options just complicate the code. What do you think? Looping in Darin re: removing --process-per-tab flag. I'd say its main value at this point is for debugging, since it provides a way to selectively turn off process swaps for cross-site navigations (without falling back to the potentially buggy --single-process). I haven't used it for debugging for a while, but it could be a useful tool. I'm also curious how many people use it in practice, but we don't have histograms for that at the moment. It's probably seldom used for the right reasons anyway, and it probably just exposes those users to more untested code paths than usual. I think I'd be ok with killing it. Darin, what do you think? Charlie
http://codereview.chromium.org/1723016/diff/1/3 File chrome/browser/tab_contents/render_view_host_manager.cc (right): http://codereview.chromium.org/1723016/diff/1/3#newcode576 chrome/browser/tab_contents/render_view_host_manager.cc:576: SiteInstance* new_instance = curr_instance; On 2010/04/27 20:53:48, creis wrote: > Can we move the call to ShouldSwapProcessForNavigation (on line 581) to here, > storing it as forceProcessSwap? Then we could include it in both of the if > statements below, on lines 577 and 580. That means that > ShouldSwapProcessForNavigation will take effect regardless of the process model, > which might have been the original intent of that method. BTW, I think this won't work, because new_instance might change as a result of calling GetSiteInstanceForEntry below.
http://codereview.chromium.org/1723016/diff/1/3 File chrome/browser/tab_contents/render_view_host_manager.cc (right): http://codereview.chromium.org/1723016/diff/1/3#newcode576 chrome/browser/tab_contents/render_view_host_manager.cc:576: SiteInstance* new_instance = curr_instance; On 2010/04/27 22:25:55, Matt Perry wrote: > BTW, I think this won't work, because new_instance might change as a result of > calling GetSiteInstanceForEntry below. I think that's the point. Don't we want to change new_instance if ShouldSwapProcessForNavigation returns true? It sounds like the current behavior is broken, given that you're adding an exception to ShouldTransitionCrossSite for extension schemes to fix this bug. Is there something that would break if we moved that check earlier and OR'd it into the two if statements? The check itself doesn't depend on new_instance, so I think its behavior would stay the same.
http://codereview.chromium.org/1723016/diff/1/3 File chrome/browser/tab_contents/render_view_host_manager.cc (right): http://codereview.chromium.org/1723016/diff/1/3#newcode576 chrome/browser/tab_contents/render_view_host_manager.cc:576: SiteInstance* new_instance = curr_instance; On 2010/04/27 23:07:38, creis wrote: > On 2010/04/27 22:25:55, Matt Perry wrote: > > BTW, I think this won't work, because new_instance might change as a result of > > calling GetSiteInstanceForEntry below. > > I think that's the point. Don't we want to change new_instance if > ShouldSwapProcessForNavigation returns true? It sounds like the current > behavior is broken, given that you're adding an exception to > ShouldTransitionCrossSite for extension schemes to fix this bug. > > Is there something that would break if we moved that check earlier and OR'd it > into the two if statements? The check itself doesn't depend on new_instance, so > I think its behavior would stay the same. Ah, sorry, you're right. The problem I was seeing was because there was no current entry in the case I was testing. Here's what things look like with the fix you suggested - I had to modify ShouldSwapProcessesForNavigation to fix the case I saw.
I like it, and I think we can keep support for the flag for the time being. Would it be hard to add a test to web_contents_unittest.cc to ensure that we swap to the correct extension renderer if one already exists? Hmm-- actually, that probably requires a fix to 42675 (the bug I filed for the TestTabContents issue). I'll make a note of it there. http://codereview.chromium.org/1723016/diff/9001/10002 File chrome/browser/tab_contents/render_view_host_manager.cc (right): http://codereview.chromium.org/1723016/diff/9001/10002#newcode286 chrome/browser/tab_contents/render_view_host_manager.cc:286: // to use our transition logic in that case. We can undo this change to the comment now. http://codereview.chromium.org/1723016/diff/9001/10002#newcode295 chrome/browser/tab_contents/render_view_host_manager.cc:295: return true; Are there ever any cases when new_entry is null? (It doesn't look like it.) If that's the case, we could make this a little easier to read by first DCHECK'ing new_entry and removing both null checks for new_entry in the if statements.
http://codereview.chromium.org/1723016/diff/9001/10002 File chrome/browser/tab_contents/render_view_host_manager.cc (right): http://codereview.chromium.org/1723016/diff/9001/10002#newcode286 chrome/browser/tab_contents/render_view_host_manager.cc:286: // to use our transition logic in that case. On 2010/04/28 00:06:38, creis wrote: > We can undo this change to the comment now. Done. http://codereview.chromium.org/1723016/diff/9001/10002#newcode295 chrome/browser/tab_contents/render_view_host_manager.cc:295: return true; On 2010/04/28 00:06:38, creis wrote: > Are there ever any cases when new_entry is null? (It doesn't look like it.) If > that's the case, we could make this a little easier to read by first DCHECK'ing > new_entry and removing both null checks for new_entry in the if statements. Done.
LGTM |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
