I think a better design for the in-browser unit tests would be to redesign them to work more like a normal browser. Instead of trying to restart BrowserMain, we should go all the way back to ChromeMain and unload chrome.dll. To make that work, perhaps we could bake the unit tests and the Chrome code into a ChromeTest.dll. Then we could load that before each test, and invoke its ChromeMain with a special command line flag that would magically invoke the corresponding test. With some cleverness we could probably make this work. Then advantage is that we would have a clean Chrome for each test without having to teach Chrome how to do something (namely, shutting down and restarting without unloading) it never needs to know how to do otherwise. It seems way to fragile (read: recipe for flakiness) to try to patch all of the odd statics. -Darin
On Sat, Apr 4, 2009 at 1:00 PM, Mike Belshe <[email protected]> wrote: > Actually, I think its the opposite; the one which bit Ben this week was not > a leakysingleton. Actually, the leakysingleton would have worked fine in > this case.. He was forcing AtExit to be called, and then statics which held > LazyInitPtrs were effectively uninitialized (no bug here, this is a really > weird thing to do). Ironically, the LeakySingletons wouldn't have an issue > in Ben's case, because they didn't get cleaned up.... > If we want to support Ben's case, I think we need to have better > encapsulation of all globals. I suspect the only way to get there is to > eliminate most all statics. I think Dean made a lot of progress here, but > we still have a lot more statics. Until they are gone, there will always be > more to mop up. > > Incidentally, when looking at Apple's webkit build, I noticed that their > compiles will fail if new statics are introduced. I assume this is for > performance reasons, but maybe it's because webkit needs to be embedded in > other systems and statics make that difficult. Anyway, we could add > something to our build as well which also checked to see if statics were > created. > > Mike > > > On Sat, Apr 4, 2009 at 9:56 AM, Ben Goodger (Google) <[email protected]>wrote: > >> >> Thanks for the background, M-A and Dean. >> >> I started looking into this because I wrote an in-process browser test >> for a bug fix I wrote last week. I ended up finding one conflicting >> ad-hoc singleton and fixed the issue (see my changelist to extension >> error reporter) + one issue w/LazyInstanceHelper (see separate >> changelist). But you're right, there's likely more. >> >> Part of the challenge here is that the unit tests are run in an order >> that's unknown to me, and seems to be non-deterministic based on >> recency of modification and/or clobber build, but perhaps I'm >> overlooking something obvious? It definitely possible to encounter >> issues depending on what tests run before and after other tests in the >> same process. So it seems likely that seemingly unrelated tests could >> end up broken on the builders when people ran them locally and they >> passed. This would cause people to think the unrelated test bustage >> was "flakiness". >> >> Increased coverage will likely help here, along with requiring use of >> classes like Singleton/avoiding statics in code reviews. Starting this >> coming week, I am going to be a lot more aggressive about asking for >> in-process browser tests for at least the front end code reviews that >> I do. >> >> -Ben >> >> On Sat, Apr 4, 2009 at 9:48 AM, Marc-Antoine Ruel <[email protected]> >> wrote: >> > I recall when Singleton and AtExitManager were created, there was >> > discussion about controlled shutdown. I think this use case clearly >> > show why LeakySingletonTrait shouldn't be used whenever possible and >> > controled shutdown has important use-cases. If it is a multithreaded >> > issue, the culprit threads should be join-ed. >> > >> > Now, w.r.t. to what Ben said, I know adhoc singletons definitely >> > exists in a few place in webkit and in chromium. Finding them will be >> > a lot of fun but would be a smart thing to do. (And by the way I need >> > to thank Dean for having fixed a few of those in the past) >> > >> > Ben do you have a specific plan to fix this issue? >> > >> > M-A >> > >> > On Friday, April 3, 2009, Ben Goodger (Google) <[email protected]> >> wrote: >> >> >> >> The new in process browser test framework creates a headless >> >> browser-like environment where rich cross-sectional tests can be run >> >> as unit tests rather than as ui tests. UI tests are a PITA and so this >> >> is very enticing. >> >> >> >> One issue we've had with so far is that many objects implement >> >> singletons in an ad-hoc way, sometimes caching pointers to objects >> >> that later get destroyed. Since a browser startup sequence is >> >> simulated (by a call to BrowserMain) for each run of an in-process >> >> browser test, when you run multiple in-process browser tests in the >> >> same unit_tests.exe run you will eventually crash somewhere. >> >> >> >> One solution is to have all singleton objects derive from >> >> base::Singleton, and use the base::AtExitManager to shut them down >> >> cleanly at the end of each test run. The only question is - how many >> >> ad-hoc singletons are there? >> >> >> >> Does this conversion seem reasonable? What challenges do people >> foresee? >> >> >> >> On a related note, with the commencement of the fixit week next week >> >> we're going to get a lot more strict about requiring tests for >> >> functionality. Barring the above issue, the in-process browser testing >> >> framework makes it relatively simple to write tests for complex >> >> functionality. I think we should require tests be written using this >> >> tool if applicable and have the tests be disabled if they fail when >> >> run in combination with others, so they don't turn the buildbot red, >> >> with the idea of fixing the framework at the same time and then >> >> turning them all on. The tests will pass when run in isolation. >> >> >> >> -Ben >> >> >> >> >> >> >> >> > >> >> >> > > > > --~--~---------~--~----~------------~-------~--~----~ Chromium Developers mailing list: [email protected] View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~----------~----~----~----~------~----~------~--~---
