OK. It's also fixit week so feel free to fix Fixit bugs ;-) -Ben
On Mon, Apr 6, 2009 at 10:13 AM, Jay Campan <[email protected]> wrote: > I'll be looking at it this week, starting today. > > Jay > > On Mon, Apr 6, 2009 at 10:10 AM, Ben Goodger (Google) <[email protected]> > wrote: >> Hi Jay, thanks for volunteering. What's your timeframe for looking at this? >> >> -Ben >> >> On Mon, Apr 6, 2009 at 10:02 AM, Jay Campan <[email protected]> wrote: >>> I'll take a look at it since I am rewriting the SSL UI tests to be in-proc. >>> >>> Jay >>> >>> On Mon, Apr 6, 2009 at 9:58 AM, Darin Fisher <[email protected]> wrote: >>>> Anyone want to try to make this happen? :-) >>>> >>>> -Darin >>>> >>>> On Mon, Apr 6, 2009 at 5:47 AM, Marc-Antoine Ruel <[email protected]> >>>> wrote: >>>>> >>>>> I think this is a great idea and definitely implementable. Filed: >>>>> http://code.google.com/p/chromium/issues/detail?id=9764 >>>>> >>>>> >>>>> On Mon, Apr 6, 2009 at 1:20 AM, Darin Fisher <[email protected]> wrote: >>>>> > 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 -~----------~----~----~----~------~----~------~--~---
