Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9147051:
Don't swap processes for javascript: URLs. (Closed)
|
Created:
8 years, 11 months ago by nasko Modified:
8 years, 11 months ago CC:
chromium-reviews, creis+watch_chromium.org, jam, dpranke-watch+content_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, ajwong+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionDon't swap processes for javascript: URLs.
BUG=93199
TEST=Enter "javascript:alert(document.location.href);" in the omnibox while in a hosted app.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=117744
Patch Set 1 : Don't swap processes for "javascript:" URLs. #
Total comments: 10
Patch Set 2 : Addressing feedback from creis. #
Total comments: 8
Patch Set 3 : Fixed the unit test and restored the function to not be virtual. #
Total comments: 10
Patch Set 4 : Added extra test case in the unit test and cleaned up code style. #
Total comments: 8
Patch Set 5 : Fixing all variable style issues while at it. #Messages
Total messages: 16 (0 generated)
Charlie, Can you review the changes I've made for properly handling javascript URLs for hosted apps? Thanks in advance, Nasko
Great start. Just a few nits and a suggestion to expand the test. http://codereview.chromium.org/9147051/diff/5001/content/browser/site_instanc... File content/browser/site_instance.cc (right): http://codereview.chromium.org/9147051/diff/5001/content/browser/site_instanc... content/browser/site_instance.cc:133: // we want to run in the same procses. Nit: Let's say "we want to keep it in the same process" just to be clear. Also, s/procses/process/ http://codereview.chromium.org/9147051/diff/5001/content/browser/site_instance.h File content/browser/site_instance.h (right): http://codereview.chromium.org/9147051/diff/5001/content/browser/site_instanc... content/browser/site_instance.h:77: virtual bool HasProcess() const; Nit: Let's add "Virtual for testing." to the comment. http://codereview.chromium.org/9147051/diff/5001/content/browser/site_instanc... File content/browser/site_instance_unittest.cc (right): http://codereview.chromium.org/9147051/diff/5001/content/browser/site_instanc... content/browser/site_instance_unittest.cc:154: bool HasProcess() const { Please add OVERRIDE, so that we'll remember to update this if the function name ever changes. http://codereview.chromium.org/9147051/diff/5001/content/browser/site_instanc... content/browser/site_instance_unittest.cc:557: TEST_F(SiteInstanceTest, SameProcessForJavascript) { Maybe we can make this a more general test and call it HasWrongProcessForURL, since we're not testing that anywhere else in here. That would let us also test calling it before HasProcess() is true.
I've addressed the points and submitted a new change. http://codereview.chromium.org/9147051/diff/5001/content/browser/site_instanc... File content/browser/site_instance.cc (right): http://codereview.chromium.org/9147051/diff/5001/content/browser/site_instanc... content/browser/site_instance.cc:133: // we want to run in the same procses. On 2012/01/12 22:01:14, creis wrote: > Nit: Let's say "we want to keep it in the same process" just to be clear. > > Also, s/procses/process/ Done. http://codereview.chromium.org/9147051/diff/5001/content/browser/site_instance.h File content/browser/site_instance.h (right): http://codereview.chromium.org/9147051/diff/5001/content/browser/site_instanc... content/browser/site_instance.h:77: virtual bool HasProcess() const; On 2012/01/12 22:01:14, creis wrote: > Nit: Let's add "Virtual for testing." to the comment. Done. http://codereview.chromium.org/9147051/diff/5001/content/browser/site_instanc... File content/browser/site_instance_unittest.cc (right): http://codereview.chromium.org/9147051/diff/5001/content/browser/site_instanc... content/browser/site_instance_unittest.cc:154: bool HasProcess() const { On 2012/01/12 22:01:14, creis wrote: > Please add OVERRIDE, so that we'll remember to update this if the function name > ever changes. Done. http://codereview.chromium.org/9147051/diff/5001/content/browser/site_instanc... content/browser/site_instance_unittest.cc:557: TEST_F(SiteInstanceTest, SameProcessForJavascript) { On 2012/01/12 22:01:14, creis wrote: > Maybe we can make this a more general test and call it HasWrongProcessForURL, > since we're not testing that anywhere else in here. > > That would let us also test calling it before HasProcess() is true. Added a test case before we've assigned a process to the site instance. The actual check will be harder to test and considering Albert's changes will touch this code path, I'd wait for adding more tests.
Drive-by got a few style nits. :) http://codereview.chromium.org/9147051/diff/5001/content/browser/site_instanc... File content/browser/site_instance_unittest.cc (right): http://codereview.chromium.org/9147051/diff/5001/content/browser/site_instanc... content/browser/site_instance_unittest.cc:557: TEST_F(SiteInstanceTest, SameProcessForJavascript) { On 2012/01/12 22:47:47, nasko wrote: > On 2012/01/12 22:01:14, creis wrote: > > Maybe we can make this a more general test and call it HasWrongProcessForURL, > > since we're not testing that anywhere else in here. > > > > That would let us also test calling it before HasProcess() is true. > > Added a test case before we've assigned a process to the site instance. The > actual check will be harder to test and considering Albert's changes will touch > this code path, I'd wait for adding more tests. Don't worry about me :) If you commit first, then its my job is to fix the new code! (also, not sure when I'd actually commit) http://codereview.chromium.org/9147051/diff/5004/content/browser/site_instance.h File content/browser/site_instance.h (right): http://codereview.chromium.org/9147051/diff/5004/content/browser/site_instanc... content/browser/site_instance.h:78: virtual bool HasProcess() const; It's unfortunate to have a partially-virtual interface -- especially if the motivating reason is just a unittest. Can we get away with a SetProcessForTest() that's private and then just friend the test using the FRIEND_TEST* macros? http://codereview.chromium.org/9147051/diff/5004/content/browser/site_instanc... File content/browser/site_instance_unittest.cc (right): http://codereview.chromium.org/9147051/diff/5004/content/browser/site_instanc... content/browser/site_instance_unittest.cc:158: bool hasProcess; incorrect naming convention. :( http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Variable_Names Also, a setter is preferable to a public variable even if this is a file private class. http://codereview.chromium.org/9147051/diff/5004/content/browser/site_instanc... content/browser/site_instance_unittest.cc:163: ~TestSiteInstance() { Since we're here, can we add a virtual in front of this? http://codereview.chromium.org/9147051/diff/5004/content/browser/site_instanc... content/browser/site_instance_unittest.cc:564: TestSiteInstance* instance = TestSiteInstance::CreateTestSiteInstance( Line indent on wrapping should be 4 spaces. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls
I redid the unit test and it no longer requires virtual function. http://codereview.chromium.org/9147051/diff/5001/content/browser/site_instanc... File content/browser/site_instance_unittest.cc (right): http://codereview.chromium.org/9147051/diff/5001/content/browser/site_instanc... content/browser/site_instance_unittest.cc:557: TEST_F(SiteInstanceTest, SameProcessForJavascript) { On 2012/01/12 22:56:41, awong wrote: > On 2012/01/12 22:47:47, nasko wrote: > > On 2012/01/12 22:01:14, creis wrote: > > > Maybe we can make this a more general test and call it > HasWrongProcessForURL, > > > since we're not testing that anywhere else in here. > > > > > > That would let us also test calling it before HasProcess() is true. > > > > Added a test case before we've assigned a process to the site instance. The > > actual check will be harder to test and considering Albert's changes will > touch > > this code path, I'd wait for adding more tests. > > Don't worry about me :) If you commit first, then its my job is to fix the new > code! (also, not sure when I'd actually commit) Done. http://codereview.chromium.org/9147051/diff/5004/content/browser/site_instance.h File content/browser/site_instance.h (right): http://codereview.chromium.org/9147051/diff/5004/content/browser/site_instanc... content/browser/site_instance.h:78: virtual bool HasProcess() const; On 2012/01/12 22:56:41, awong wrote: > It's unfortunate to have a partially-virtual interface -- especially if the > motivating reason is just a unittest. > > Can we get away with a SetProcessForTest() that's private and then just friend > the test using the FRIEND_TEST* macros? I rewrote the test differently and now it doesn't require the function to be virtual. http://codereview.chromium.org/9147051/diff/5004/content/browser/site_instanc... File content/browser/site_instance_unittest.cc (right): http://codereview.chromium.org/9147051/diff/5004/content/browser/site_instanc... content/browser/site_instance_unittest.cc:158: bool hasProcess; On 2012/01/12 22:56:41, awong wrote: > incorrect naming convention. :( > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Variable_Names > > Also, a setter is preferable to a public variable even if this is a file private > class. Moved to a different test that doesn't require modifying this test class. Still fixed up the class to be more inline with the style guide. http://codereview.chromium.org/9147051/diff/5004/content/browser/site_instanc... content/browser/site_instance_unittest.cc:163: ~TestSiteInstance() { On 2012/01/12 22:56:41, awong wrote: > Since we're here, can we add a virtual in front of this? Done. http://codereview.chromium.org/9147051/diff/5004/content/browser/site_instanc... content/browser/site_instance_unittest.cc:564: TestSiteInstance* instance = TestSiteInstance::CreateTestSiteInstance( On 2012/01/12 22:56:41, awong wrote: > Line indent on wrapping should be 4 spaces. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls Done.
Thanks for the other style fixes. A few more comments inline. http://codereview.chromium.org/9147051/diff/6004/content/browser/site_instance.h File content/browser/site_instance.h (right): http://codereview.chromium.org/9147051/diff/6004/content/browser/site_instanc... content/browser/site_instance.h:77: // Virtual to allow tests to extend it. No longer need this comment. http://codereview.chromium.org/9147051/diff/6004/content/browser/site_instanc... File content/browser/site_instance_unittest.cc (right): http://codereview.chromium.org/9147051/diff/6004/content/browser/site_instanc... content/browser/site_instance_unittest.cc:158: bool HasProcess() const OVERRIDE { We can't override this any more if it's not virtual, right? http://codereview.chromium.org/9147051/diff/6004/content/browser/site_instanc... content/browser/site_instance_unittest.cc:564: // Test to ensure that javascript URLs always run in the same process. Please update this comment as well. http://codereview.chromium.org/9147051/diff/6004/content/browser/site_instanc... content/browser/site_instance_unittest.cc:582: host.reset(instance->GetProcess()); Wow, I'm impressed if this works. Do you know if it actually spins up a whole renderer process, or just creates a test RenderProcessHost? http://codereview.chromium.org/9147051/diff/6004/content/browser/site_instanc... content/browser/site_instance_unittest.cc:589: Extra line can be deleted.
Cleaned up code style and added an extra test case. http://codereview.chromium.org/9147051/diff/6004/content/browser/site_instance.h File content/browser/site_instance.h (right): http://codereview.chromium.org/9147051/diff/6004/content/browser/site_instanc... content/browser/site_instance.h:77: // Virtual to allow tests to extend it. On 2012/01/13 18:37:46, creis wrote: > No longer need this comment. Done. http://codereview.chromium.org/9147051/diff/6004/content/browser/site_instanc... File content/browser/site_instance_unittest.cc (right): http://codereview.chromium.org/9147051/diff/6004/content/browser/site_instanc... content/browser/site_instance_unittest.cc:158: bool HasProcess() const OVERRIDE { On 2012/01/13 18:37:46, creis wrote: > We can't override this any more if it's not virtual, right? Done. http://codereview.chromium.org/9147051/diff/6004/content/browser/site_instanc... content/browser/site_instance_unittest.cc:564: // Test to ensure that javascript URLs always run in the same process. On 2012/01/13 18:37:46, creis wrote: > Please update this comment as well. Done. http://codereview.chromium.org/9147051/diff/6004/content/browser/site_instanc... content/browser/site_instance_unittest.cc:582: host.reset(instance->GetProcess()); On 2012/01/13 18:37:46, creis wrote: > Wow, I'm impressed if this works. Do you know if it actually spins up a whole > renderer process, or just creates a test RenderProcessHost? Yes, it indeed spawns a new process. http://codereview.chromium.org/9147051/diff/6004/content/browser/site_instanc... content/browser/site_instance_unittest.cc:589: On 2012/01/13 18:37:46, creis wrote: > Extra line can be deleted. Done.
LGTM, with the one change below. http://codereview.chromium.org/9147051/diff/14001/content/browser/site_instan... File content/browser/site_instance_unittest.cc (right): http://codereview.chromium.org/9147051/diff/14001/content/browser/site_instan... content/browser/site_instance_unittest.cc:158: void SetHasProcess(bool process) { set_has_process, since it's just a setter function.
LGTM w/ nits http://codereview.chromium.org/9147051/diff/14001/content/browser/site_instan... File content/browser/site_instance.cc (right): http://codereview.chromium.org/9147051/diff/14001/content/browser/site_instan... content/browser/site_instance.cc:134: if (IsURLSameAsAnySiteInstance(url)) nit: s/URL/Url/ http://codereview.chromium.org/9147051/diff/14001/content/browser/site_instan... File content/browser/site_instance_unittest.cc (right): http://codereview.chromium.org/9147051/diff/14001/content/browser/site_instan... content/browser/site_instance_unittest.cc:123: delete_counter_(deleteCounter) { nit: %s/deleteCounter/delete_counter/g http://codereview.chromium.org/9147051/diff/14001/content/browser/site_instan... content/browser/site_instance_unittest.cc:580: EXPECT_TRUE(host.get() != NULL); EXPECT_TRUE(host.get()) is jsut as common (maybe more common). Up to you if you like the explicit != NULL.
I've fixed the few comments and went through the entire file to fix variable naming inconsistency. http://codereview.chromium.org/9147051/diff/14001/content/browser/site_instan... File content/browser/site_instance.cc (right): http://codereview.chromium.org/9147051/diff/14001/content/browser/site_instan... content/browser/site_instance.cc:134: if (IsURLSameAsAnySiteInstance(url)) On 2012/01/13 21:19:04, awong wrote: > nit: s/URL/Url/ Done. http://codereview.chromium.org/9147051/diff/14001/content/browser/site_instan... File content/browser/site_instance_unittest.cc (right): http://codereview.chromium.org/9147051/diff/14001/content/browser/site_instan... content/browser/site_instance_unittest.cc:123: delete_counter_(deleteCounter) { On 2012/01/13 21:19:04, awong wrote: > nit: %s/deleteCounter/delete_counter/g Done. http://codereview.chromium.org/9147051/diff/14001/content/browser/site_instan... content/browser/site_instance_unittest.cc:158: void SetHasProcess(bool process) { On 2012/01/13 21:16:29, creis wrote: > set_has_process, since it's just a setter function. Function is not needed anymore, since I create a real process in the test. http://codereview.chromium.org/9147051/diff/14001/content/browser/site_instan... content/browser/site_instance_unittest.cc:580: EXPECT_TRUE(host.get() != NULL); On 2012/01/13 21:19:04, awong wrote: > EXPECT_TRUE(host.get()) is jsut as common (maybe more common). > > Up to you if you like the explicit != NULL. I was trying to be consistent with the rest of the file :).
Including John. John, can you review the changes I've made for this bug? Thanks in advance, Nasko
rubberstamp lgtm, creis and wong know this more than me
LGTM Thanks for fixing up the style!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nasko@chromium.org/9147051/15004
why wait for charlie when we can CQ it! ;) (I CQed it) On Fri, Jan 13, 2012 at 3:48 PM, <commit-bot@chromium.org> wrote: > CQ is trying da patch. Follow status at > https://chromium-status.**appspot.com/cq/nasko@chromium.** > org/9147051/15004<https://chromium-status.appspot.com/cq/nasko@chromium.org/9147051/15004> > > > http://codereview.chromium.**org/9147051/<http://codereview.chromium.org/9147... >
Change committed as 117744 |
