On Dienstag, 21. Dezember 2010, Einar Rünkaru wrote: > On Mon, Dec 20, 2010 at 11:39 PM, Johannes Sixt <[email protected]> wrote: > > On Sonntag, 19. Dezember 2010, Einar Rünkaru wrote: > >> On Sun, Dec 19, 2010 at 10:41 PM, Johannes Sixt <[email protected]> wrote: > >> > On Sonntag, 19. Dezember 2010, Einar Rünkaru wrote: > >> >> diff --git a/cinelerra/pluginclient.h b/cinelerra/pluginclient.h > >> >> index f1a340b..2b20e66 100644 > >> >> --- a/cinelerra/pluginclient.h > >> >> +++ b/cinelerra/pluginclient.h > >> >> @@ -112,6 +112,7 @@ void thread_class::run() \ > >> >> int result = window->run_window(); \ > >> >> /* This is needed when the GUI is closed from itself */ \ > >> >> if(result) plugin->client_side_close(); \ > >> >> + plugin->thread = 0; \ > >> >> } > >> > > >> > But PLUGIN_DESTRUCTOR_MACRO says this: > >> > > >> > #define PLUGIN_DESTRUCTOR_MACRO \ > >> > if(thread) \ > >> > { \ > >> > /* This is needed when the GUI is closed from elsewhere than itself */ > >> > \ /* Since we now use autodelete, this is all that has to be done, > >> > thread will take care of itself ... */ \ > >> > /* Thread join will wait if this was not called from the thread itself > >> > or go on if it was */ \ > >> > thread->window->lock_window("PLUGIN_DESTRUCTOR_MACRO"); > >> > \ thread->window->set_done(0); \ > >> > thread->window->unlock_window(); \ > >> > thread->join(); \ > >> > ... > >> > > >> > How does this make sense when you have to set plugin->thread to NULL > >> > manually? I mean, even when the window is "closed from elsewhere", > >> > run_window() will return eventually, and only after that can the > >> > desctructor run (so I hope, otherwise we are in bigger trouble). And > >> > in this case, the destructor expects thread to be non-NULL. It would > >> > not be the case anymore with your patch. > >> > > >> > I haven't analyzed what's going on, but am only reasoning from what I > >> > see in the code fragment and comments. I must be missing something. > >> > Please help. > >> > >> Scenario (names from macro): > >> 1. Plugin creates thread_class and starts it. thread_class::run() > >> writes pointer to itself > >> into plugin. > >> 2. thread_class:run() returns, immediately after that thread_class > >> object will be destroyed and memory freed. > >> 3. Freed memory will be used and written over by another obect. > >> 4. Plugin destructor uses thread ptr to access already destroyed > >> object, but this ptr points to nowhere (if step 3 happened). > >> > >> It's definitely a bug if you try to reference to already destroyed > >> object. plugin->thread in thread_class and thread in > >> PLUGIN_DESTRUCTOR_MACRO are the same and point to thread_class object > >> whitch may be already destroyed when plugin destructor runs. It is > >> hard to understand, but I was able to create quite reliable crash > >> situation at that point. > > > > But why don't you delete the reference to thread from > > PLUGIN_DESTRUCTOR_MACRO? Isn't it worthless because it is alway NULL? > > > > And if you do remove it, how is the "closed from elsewhere" situation > > taken care of? > > > > Did you understand what the point of commit d5ff675ddb is that touched > > PLUGIN_DESTRUCTOR_MACRO? > > Remember that thread_class created with synchronous=0 and > autodelete=1, this means that thread exits > immediately afrer returning from run and deletes thread_class object. > There is no need for thread_join (but it does not harm). > PLUGIN_DESTRUCTOR_MACRO must not use thread ptr after thread_class has > returned (look guicast/thread.C) > > "closed from elsewere" means, that plugin destructor was called while > thread_class is running. In this case plugin tells to thread_class to > close window and return from run (and delete itself). > > There is no problem if thread_class is not created - thread ptr is > null and destructor works as expected. Thread ptr is set in > thread_class::run to indicate when thread_class is ready and it is > logical to clear it while thread_class prepares to exit. > > In the Ui the thread_class is plugin's configuration window. And user > can remove plugin - plugin destructor must destroy > plugin's configuration window - or user can simply close configuration > window - then plugin destructor must not use thread ptr - thread > object is immediately deleted after close of the window (autodelete > means this). > > I am thinking about removing the autodelete feature, because of hard > to understand and debug side effects. The only place, where autodelete > is used is PLUGIN_THREAD_OBJECT macro. But before all plugins must be > checked - may be some of them makes special use of the feature.
I observed this in the debugger. I was only able to trigger the case where the destructor runs first while thread is still non-NULL. How do I trigger the case where run() terminates before the destructor investigates if (thread != NULL)? > >> >> Second patch fixes audio-video synchronization while using ALSA sound > >> >> output. > ... > I looked at jerkyness problem... Do I understand correctly that you keep the issue on your radar? I don't know what to do about this one. Since it is a regression in my case, I'm hestant to push the patch out. What do others think? -- Hannes _______________________________________________ Cinelerra mailing list [email protected] https://init.linpro.no/mailman/skolelinux.no/listinfo/cinelerra
