> > - if (instance =3D=3D NULL)
> > + This =3D (PluginInstance *) instance->pdata;
> > + if (instance =3D=3D NULL || This =3D=3D NULL)
> > return NPERR_INVALID_INSTANCE_ERROR;
> Is it possible for instance to be NULL here? Then the browser will =
> when you try to get instance->pdata...
Some one on the list said it happened one time. If it's null, then this =
code makes sure the browser will not die.
> Instead of doing this, why not loop in NPP_Destroy checking if the
> process still exists? [You can use kill(This->pid, 0) for this]
That is better. I didn't know this one.
> Also, does This get reinitialized before this function so possibly
> This->pid isn't set to anything valid?
This->pid gets 0 at the very beginning so the check is correct.
> Is it possible for This or This->td to be freed while the playPlaylist
> thread is still running?
I think it's not running anymore when it's freed.
> Using kill -15 here is much more polite than kill -9.
But if u send 9 the cache process dont get killed and you get a stuck =
> Might I suggest, though, instead of sending a signal, to write =
> directly to the MPlayer control stream, from the parent thread? Then
> you wouldn't have the synchronization problem I mention later in this
> message. But that relies on the stdio structure's synchronization in
> order to work properly. (Actually, after thinking about this, the
> control stream could be NULL, so this is probably not a good idea, as
> well as being racy if a playPlaylist thread is in the middle of =
> up mplayer).
I do that to. Is on kevins original code.
> Alas, you will probably still need to check This->pid if you want to
> make sure the process is really gone before returning, by using kill =
> to test the process's existence.
> What if the control stream is NULL? Does this get called by the
> parent process at all, or only the child? If yes, then there is
> certainly a race where control can be NULL...
Maybe I could check that too. It's called only on parent process.
> What effect do setsid() and setpgid() have here?
This one was a little bit tricky to find. If you don't do that, mozilla =
> Maybe the buffers should be disabled (with setvbuf) at least on the
> control stream, so you don't have to fflush in sendCommand. But maybe
> it's not worth worrying about.
I think it's not worth it. fflush makes sure.
> Possible problem: This->pid is set by one thread (the playPlaylist
> thread), and read in another (the main thread). You might want to
> protect it with a mutex... I don't know if this is safe, since it
> depends on whether some other synchronization would protect This->pid =
> happenstance, so you might end up with the parent process checking a
> stale This->pid value when sending mplayer a signal, thus causing it =
> to stop the right mplayer.
I don't know if its needed. You always have only one player running per =
This struct. And This->pid is set only on the thread.
> You might want to add a mutex, that you hold
> across process creation as well to prevent the main thread thinking it
> has killed the mplayer process but that is still hanging around. You
> would also hold it right when you fclose the mplayer stdout, because =
> need to read This->pid there too, as a result of...:
Definitly the code is not right. I'll see if these mutexes solve the =
> ...needing a "mypclose" that calls waitpid, otherwise you'll get =
> here. Also, you don't close the This->control FILE* stream=20
There is other way to not get zombies. Call signal(SIGCHLD, SIG_IGN). As =
kill -0 is easier to use than a signal handler I think it's better.
Thanks for the comments. I'll make some changes on my patch and sync =
against latest cvs.