On Fri, Oct 30, 2009 at 3:26 PM, Jeremy Orlow <[email protected]> wrote:
> On Fri, Oct 30, 2009 at 3:17 PM, Antoine Labour <[email protected]> wrote: > >> On Fri, Oct 30, 2009 at 3:12 PM, Jeremy Orlow <[email protected]>wrote: >> >>> I've spent a good deal of this week trying to track down what turned out >>> to be a simple but fairly common problem: I forgot virtual dispatch only >>> partially works in destructors. There have been several email threads about >>> this, but it still bites us form time to time, so I thought it was worth >>> another reminder. >>> >>> >>> Details: >>> I subclassed ChromeThread which subclasses base::Thread. base::Thread >>> calls CleanUp on the thread right before termination. CleanUp is virtual. >>> Both ChromeThread and my class override CleanUp(). base::Thread calls >>> Stop() in its destructor to stop the thread (if it hasn't already been >>> stopped). But by the time you hit destruction, the vtable is no longer >>> available and thus the destructor of base::Thread (and anything it calls) >>> does NOT have access to the vtable of ChromeThread (or my class). So, if >>> you don't explicitly call Stop(), your subclass's CleanUp method will NOT be >>> called. Thus the thread was going away without my CleanUp method ever being >>> called. >>> >>> Obviously this affects more than just base::Thread. And this is also how >>> you can hit errors with pure virtual methods being called. >>> >>> J >>> >> >> Suggestion: don't call CleanUp in the destructor, but check that someone >> did. >> > > I assume you mean Stop()? > Yes, sorry, looking at the code, that's what I meant. > > The problem is when you have a class like ChromeThread inherit from > base::Thread and then call Stop() in its destructor and then have someone > else subclass ChromeThread and expect its CleanUp to be called. > Yup, the pattern is dangerous, so instead of relying on Stop being called from the destructor, it should be explicitly done by the client (and moving Stop() to ~ChromeThread will only move the problem), and the destructor should check that it has been done. Antoine > > > On Fri, Oct 30, 2009 at 3:16 PM, Adam Barth <[email protected]> wrote: > > I'm sorry for introducing this pattern in base::Thread. It's bitten >> use several times over the course of the project. If you see a better >> design, please don't hesitate to fix it. >> >> Adam > > > Already filed a bug: http://crbug.com/26365 > --~--~---------~--~----~------------~-------~--~----~ Chromium Developers mailing list: [email protected] View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~----------~----~----~----~------~----~------~--~---
