Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
355223002:
Add ExtensionsTestBrowserContext class for test. (Closed)
DescriptionAdd ExtensionsTestBrowserContext class for test.
and Use ExtensionTestBrowserContext for ImageLoader unittest.
ExtensionsTestBrowserContext class does
set TestExtensionsBrowserClient from constructor and
call DestroyBrowserContextServices from destructor.
ExtensionTestBrowserContext will be used instead of TestingProfile.
R=yoz@chromium.org, rdevlin@chromium.org
BUG=354046
TEST=unit_tests --gtest_filter=ImageLoaderTest.DeleteExtensionWhileWaitingForCache
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284330
Patch Set 1 #Patch Set 2 : content/DEPS doesn't allow #Patch Set 3 : ExtensionsTestBrowserContext #
Total comments: 6
Patch Set 4 : #
Total comments: 6
Patch Set 5 : #
Total comments: 5
Patch Set 6 : move to e/browser #Patch Set 7 : #
Total comments: 4
Patch Set 8 : make as member #
Total comments: 1
Messages
Total messages: 32 (0 generated)
PTAL. This is part of https://codereview.chromium.org/346193002/. After struggling, I was able to find a method :) if this pattern looks good, then I'll remove all the Profile in /extensions in seperate CL.
Wow - this was tricky! You have the right problem, but you actually discovered a much larger problem with TestBrowserContext. Namely, one destruct, the TestBrowserContext doesn't call BrowserContextDependencyManager::DestroyBrowserContextServices(), which means that any mappings it creates are left around after the fact. My proposed solution: create an ExtensionsTestBrowserContext class which creates and sets a TestExtensionsBrowserClient (if one doesn't already exist) on construction, and destroys that client and calls DestoryBrowserContextServices() on deletion. We can then use this ExtensionsTestBrowserContext in our testing code. A shorter version: Put a call to DestoryBrowserContextServices() in ~TestBrowserContext() (which should probably happen anyway). Downside is that we still frequently need an ExtensionsBrowserClient (because ExtensionRegistry::Get() uses it). +yoz for his opinion. Sorry that this turned out to be more work than you probably intended, but thanks for discovering the issue! :)
On 2014/06/27 22:05:54, Devlin wrote: > Wow - this was tricky! > > You have the right problem, but you actually discovered a much larger problem > with TestBrowserContext. Namely, one destruct, the TestBrowserContext doesn't > call BrowserContextDependencyManager::DestroyBrowserContextServices(), which > means that any mappings it creates are left around after the fact. > > My proposed solution: > create an ExtensionsTestBrowserContext class which creates and sets a > TestExtensionsBrowserClient (if one doesn't already exist) on construction, and > destroys that client and calls DestoryBrowserContextServices() on deletion. We > can then use this ExtensionsTestBrowserContext in our testing code. > > A shorter version: > Put a call to DestoryBrowserContextServices() in ~TestBrowserContext() (which > should probably happen anyway). Downside is that we still frequently need an > ExtensionsBrowserClient (because ExtensionRegistry::Get() uses it). > > +yoz for his opinion. > > Sorry that this turned out to be more work than you probably intended, but > thanks for discovering the issue! :) Yes, I agree that we should fix TestBrowserContext to actually destroy its services. Of course, in doing so, you might discover that a number of tests depend on the current brokenness... Then, we should decide what to do depending on how many such tests there are.
On 2014/06/28 00:10:54, Yoyo Zhou (OOO till July 6) wrote: > On 2014/06/27 22:05:54, Devlin wrote: > > Wow - this was tricky! > > > > You have the right problem, but you actually discovered a much larger problem > > with TestBrowserContext. Namely, one destruct, the TestBrowserContext doesn't > > call BrowserContextDependencyManager::DestroyBrowserContextServices(), which > > means that any mappings it creates are left around after the fact. > > > > My proposed solution: > > create an ExtensionsTestBrowserContext class which creates and sets a > > TestExtensionsBrowserClient (if one doesn't already exist) on construction, > and > > destroys that client and calls DestoryBrowserContextServices() on deletion. > We > > can then use this ExtensionsTestBrowserContext in our testing code. > > > > A shorter version: > > Put a call to DestoryBrowserContextServices() in ~TestBrowserContext() (which > > should probably happen anyway). Downside is that we still frequently need an > > ExtensionsBrowserClient (because ExtensionRegistry::Get() uses it). > > > > +yoz for his opinion. > > > > Sorry that this turned out to be more work than you probably intended, but > > thanks for discovering the issue! :) > > Yes, I agree that we should fix TestBrowserContext to actually destroy its > services. Of course, in doing so, you might discover that a number of tests > depend on the current brokenness... Then, we should decide what to do depending > on how many such tests there are. The Best way would be Put a call DestroyBrowserContextService() in ~TestBrowserContext() And create an ExtensionTestBrowserContext class that handle TestExtensionBrowserClient. Right? I think this eventually will fix the root cause. and we can use Extension specific logic(TestExtensionBrowserClient). But there must be many tests that I have fix.
On 2014/06/29 20:32:35, Sungguk Lim (OOO till July 5) wrote: > The Best way would be > Put a call DestroyBrowserContextService() in ~TestBrowserContext() > And create an ExtensionTestBrowserContext class that handle > TestExtensionBrowserClient. > Right? > Yes, that approach sounds good to me. > I think this eventually will fix the root cause. and we can use Extension > specific logic(TestExtensionBrowserClient). > But there must be many tests that I have fix. Hopefully, there won't be too many tests. Looking at the codebase, it seems like only 37 files even use TestBrowserContext, and I suspect that many (most?) of those don't also use BrowserContextKeyedServices. My suggestion would be to build it and send it through a trybot run, and get a feel for how many tests fail. If it's an exorbitant number, we can re-evaluate (and maybe just make ExtensionTestBrowserContext in the meantime so we don't have to keep writing tests that rely on the brokenness).
Could you take a look? Sadly, We're not able to add ""+components/keyed_service" to content/DEPS.(Patch set2). So, I took Devlin's suggestion(Create ExtensionTestBrowerContext) instead.
This mostly lg, but would appreciate a sanity check from yoz@. https://codereview.chromium.org/355223002/diff/160001/extensions/common/exten... File extensions/common/extensions_test_browser_context.h (right): https://codereview.chromium.org/355223002/diff/160001/extensions/common/exten... extensions/common/extensions_test_browser_context.h:12: class ExtensionsTestBrowserContext : public content::TestBrowserContext { Document the class; in particular, what is different between this and TestBrowserContext.
https://codereview.chromium.org/355223002/diff/160001/chrome/chrome_tests_uni... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/355223002/diff/160001/chrome/chrome_tests_uni... chrome/chrome_tests_unit.gypi:286: '../extensions/common/extensions_test_browser_context.h', I think it'd be better to put these in extensions.gyp's extensions_test_support and add that in the dependencies here. https://codereview.chromium.org/355223002/diff/160001/extensions/common/exten... File extensions/common/extensions_test_browser_context.cc (right): https://codereview.chromium.org/355223002/diff/160001/extensions/common/exten... extensions/common/extensions_test_browser_context.cc:14: ExtensionsBrowserClient::Set(new TestExtensionsBrowserClient(this)); We should also CreateBrowserContextServices here.
PTAL! And added +Lei for /chrome. https://codereview.chromium.org/355223002/diff/160001/chrome/chrome_tests_uni... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/355223002/diff/160001/chrome/chrome_tests_uni... chrome/chrome_tests_unit.gypi:286: '../extensions/common/extensions_test_browser_context.h', On 2014/07/15 19:27:30, Yoyo Zhou wrote: > I think it'd be better to put these in extensions.gyp's extensions_test_support > and add that in the dependencies here. Done. https://codereview.chromium.org/355223002/diff/160001/extensions/common/exten... File extensions/common/extensions_test_browser_context.cc (right): https://codereview.chromium.org/355223002/diff/160001/extensions/common/exten... extensions/common/extensions_test_browser_context.cc:14: ExtensionsBrowserClient::Set(new TestExtensionsBrowserClient(this)); On 2014/07/15 19:27:30, Yoyo Zhou wrote: > We should also CreateBrowserContextServices here. Used CreateBrowserContextServicesForTest instead. It seems better approach for test, and TestingProfile uses it as well. https://codereview.chromium.org/355223002/diff/160001/extensions/common/exten... File extensions/common/extensions_test_browser_context.h (right): https://codereview.chromium.org/355223002/diff/160001/extensions/common/exten... extensions/common/extensions_test_browser_context.h:12: class ExtensionsTestBrowserContext : public content::TestBrowserContext { On 2014/07/15 17:30:39, Devlin wrote: > Document the class; in particular, what is different between this and > TestBrowserContext. Done. Hopefully, no grammertical mistake!
chrome bit lgtm
Mostly good! https://codereview.chromium.org/355223002/diff/240001/extensions/common/exten... File extensions/common/extensions_test_browser_context.h (right): https://codereview.chromium.org/355223002/diff/240001/extensions/common/exten... extensions/common/extensions_test_browser_context.h:15: // bring up a necessary settings and will replace TestingProfile. Let's change this to something like "It will set necessary extension state like the ExtensionsBrowserClient and any keyed services, and cleans up after itself on destruction." https://codereview.chromium.org/355223002/diff/240001/extensions/common/exten... extensions/common/extensions_test_browser_context.h:23: BrowserContextDependencyManager* browser_context_dependency_manager_; Since we only need this twice, let's just use the GetInstance(), rather than caching it. https://codereview.chromium.org/355223002/diff/240001/extensions/common/exten... extensions/common/extensions_test_browser_context.h:24: }; Need DISALLOW_COPY_AND_ASSIGN().
PTAL! https://codereview.chromium.org/355223002/diff/240001/extensions/common/exten... File extensions/common/extensions_test_browser_context.h (right): https://codereview.chromium.org/355223002/diff/240001/extensions/common/exten... extensions/common/extensions_test_browser_context.h:15: // bring up a necessary settings and will replace TestingProfile. On 2014/07/16 20:09:50, Devlin wrote: > Let's change this to something like "It will set necessary extension state like > the ExtensionsBrowserClient and any keyed services, and cleans up after itself > on destruction." Done. https://codereview.chromium.org/355223002/diff/240001/extensions/common/exten... extensions/common/extensions_test_browser_context.h:23: BrowserContextDependencyManager* browser_context_dependency_manager_; On 2014/07/16 20:09:50, Devlin wrote: > Since we only need this twice, let's just use the GetInstance(), rather than > caching it. Done. https://codereview.chromium.org/355223002/diff/240001/extensions/common/exten... extensions/common/extensions_test_browser_context.h:24: }; On 2014/07/16 20:09:50, Devlin wrote: > Need DISALLOW_COPY_AND_ASSIGN(). Done.
https://codereview.chromium.org/355223002/diff/260001/extensions/DEPS File extensions/DEPS (right): https://codereview.chromium.org/355223002/diff/260001/extensions/DEPS#newcode2 extensions/DEPS:2: "+components/keyed_service", This should be a dep in extensions/browser rather than extensions https://codereview.chromium.org/355223002/diff/260001/extensions/common/exten... File extensions/common/extensions_test_browser_context.cc (right): https://codereview.chromium.org/355223002/diff/260001/extensions/common/exten... extensions/common/extensions_test_browser_context.cc:8: #include "extensions/browser/extensions_browser_client.h" Since you're including extensions/browser headers here, this file must be in extensions/browser, not extensions/common. Sorry I didn't catch that earlier.
https://codereview.chromium.org/355223002/diff/260001/extensions/common/exten... File extensions/common/extensions_test_browser_context.cc (right): https://codereview.chromium.org/355223002/diff/260001/extensions/common/exten... extensions/common/extensions_test_browser_context.cc:8: #include "extensions/browser/extensions_browser_client.h" On 2014/07/17 18:16:10, Yoyo Zhou wrote: > Since you're including extensions/browser headers here, this file must be in > extensions/browser, not extensions/common. Sorry I didn't catch that earlier. Shoot, I missed that, too... good catch, Yoyo.
PTAL again. https://codereview.chromium.org/355223002/diff/260001/extensions/DEPS File extensions/DEPS (right): https://codereview.chromium.org/355223002/diff/260001/extensions/DEPS#newcode2 extensions/DEPS:2: "+components/keyed_service", On 2014/07/17 18:16:10, Yoyo Zhou wrote: > This should be a dep in extensions/browser rather than extensions Done. https://codereview.chromium.org/355223002/diff/260001/extensions/common/exten... File extensions/common/extensions_test_browser_context.cc (right): https://codereview.chromium.org/355223002/diff/260001/extensions/common/exten... extensions/common/extensions_test_browser_context.cc:8: #include "extensions/browser/extensions_browser_client.h" On 2014/07/17 18:16:10, Yoyo Zhou wrote: > Since you're including extensions/browser headers here, this file must be in > extensions/browser, not extensions/common. Sorry I didn't catch that earlier. Done. Absolutely no problem. I blame that I didn't know that!
https://codereview.chromium.org/355223002/diff/320001/extensions/browser/exte... File extensions/browser/extensions_test_browser_context.cc (right): https://codereview.chromium.org/355223002/diff/320001/extensions/browser/exte... extensions/browser/extensions_test_browser_context.cc:16: ExtensionsBrowserClient::Set(new TestExtensionsBrowserClient(this)); It looks like this is leaked. Can you mean the TestExtensionsBrowserClient a member of this class?
I hope I understand what you mean. (Sorry about my English!) https://codereview.chromium.org/355223002/diff/320001/extensions/browser/exte... File extensions/browser/extensions_test_browser_context.cc (right): https://codereview.chromium.org/355223002/diff/320001/extensions/browser/exte... extensions/browser/extensions_test_browser_context.cc:16: ExtensionsBrowserClient::Set(new TestExtensionsBrowserClient(this)); On 2014/07/18 01:47:13, Yoyo Zhou wrote: > It looks like this is leaked. Can you mean the TestExtensionsBrowserClient a > member of this class? This is necessary because from the ChromeExtensionsBrowserClient::GetPrefServiceForContext, ChromeExtensionsBrowserClient::GetOriginalContext, ChromeExtensionsBrowserClient::GetOfftheRecordContext do 'static_cast' to Profile. and call Profile::GetPrefs(), GetOriginalProfile(), HasOfftheRecordProfile(). but BrowserContext doesn't have those methods. So there's crash. Yeah, we eventually have to fix ChromeExtensionsBrowserClinet to handle pure BrowserContext. I think.
https://codereview.chromium.org/355223002/diff/320001/extensions/browser/exte... File extensions/browser/extensions_test_browser_context.cc (right): https://codereview.chromium.org/355223002/diff/320001/extensions/browser/exte... extensions/browser/extensions_test_browser_context.cc:16: ExtensionsBrowserClient::Set(new TestExtensionsBrowserClient(this)); On 2014/07/18 02:22:25, Sungguk Lim wrote: > On 2014/07/18 01:47:13, Yoyo Zhou wrote: > > It looks like this is leaked. Can you mean the TestExtensionsBrowserClient a > > member of this class? > > This is necessary because from the > ChromeExtensionsBrowserClient::GetPrefServiceForContext, > ChromeExtensionsBrowserClient::GetOriginalContext, > ChromeExtensionsBrowserClient::GetOfftheRecordContext do 'static_cast' to > Profile. and call Profile::GetPrefs(), GetOriginalProfile(), > HasOfftheRecordProfile(). but BrowserContext doesn't have those methods. So > there's crash. > > Yeah, we eventually have to fix ChromeExtensionsBrowserClinet to handle pure > BrowserContext. I think. Sorry, I meant to write "make" instead of "mean" above. I mean the part: new TestExtensionsBrowserClient... This TestExtensionsBrowserClient never gets deleted. But if we make it a member of ExtensionsTestBrowserContext, and we call ExtensionsBrowserClient::Set(&test_extensions_browser_client_); that would fix it.
Could you take a look? https://codereview.chromium.org/355223002/diff/320001/extensions/browser/exte... File extensions/browser/extensions_test_browser_context.cc (right): https://codereview.chromium.org/355223002/diff/320001/extensions/browser/exte... extensions/browser/extensions_test_browser_context.cc:16: ExtensionsBrowserClient::Set(new TestExtensionsBrowserClient(this)); On 2014/07/18 02:38:45, Yoyo Zhou wrote: > On 2014/07/18 02:22:25, Sungguk Lim wrote: > > On 2014/07/18 01:47:13, Yoyo Zhou wrote: > > > It looks like this is leaked. Can you mean the TestExtensionsBrowserClient a > > > member of this class? > > > > This is necessary because from the > > ChromeExtensionsBrowserClient::GetPrefServiceForContext, > > ChromeExtensionsBrowserClient::GetOriginalContext, > > ChromeExtensionsBrowserClient::GetOfftheRecordContext do 'static_cast' to > > Profile. and call Profile::GetPrefs(), GetOriginalProfile(), > > HasOfftheRecordProfile(). but BrowserContext doesn't have those methods. So > > there's crash. > > > > Yeah, we eventually have to fix ChromeExtensionsBrowserClinet to handle pure > > BrowserContext. I think. > > Sorry, I meant to write "make" instead of "mean" above. > > I mean the part: > new TestExtensionsBrowserClient... > This TestExtensionsBrowserClient never gets deleted. But if we make it a member > of ExtensionsTestBrowserContext, and we call > ExtensionsBrowserClient::Set(&test_extensions_browser_client_); > that would fix it. Done. Yes exactly, there was a leak. Fixed.
Great, LGTM
Thank you for the review!
The CQ bit was checked by limasdf@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/limasdf@gmail.com/355223002/360001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
The CQ bit was checked by limasdf@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/limasdf@gmail.com/355223002/360001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
Message was sent while issue was closed.
Change committed as 284330
Message was sent while issue was closed.
https://codereview.chromium.org/355223002/diff/360001/extensions/browser/exte... File extensions/browser/extensions_test_browser_context.cc (right): https://codereview.chromium.org/355223002/diff/360001/extensions/browser/exte... extensions/browser/extensions_test_browser_context.cc:16: ->CreateBrowserContextServicesForTest(this); I don't think you can do this. If an earlier unit test registered browser context service factories this will attempt to create the services. If the services depend on a Profile and not a BrowserContext things will crash (usually in valgrind bots). I had this problem with https://codereview.chromium.org/381283002 We need a better solution to this problem. I'm not sure yet what it is. I think only running extensions unittests in the extensions_unittest binary would help.
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/406093004/ by jamescook@chromium.org. The reason for reverting is: This is breaking the Chrome OS valgrind bots: http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20(val... For example: http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28v... This is a problem with mixing tests that use TestingProfile and TestBrowserContext. See crbug.com/395820. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
