mplayerplug-in for mozilla / Bugs / #100 quit command hangs when mplayer is connecting

Cinquero - 2006-06-01

Logged In: YES
user_id=1018844

This is actually due to the playNode method.

https://stier.is-a-geek.com/~lxr/mplayerplugin/source/Source/plugin_threads.cpp?v=cvs20060601#792

I have patched the current cvs version so that the shutdown
is handled somewhat more harshly. I have not encountered any
problems with that so far.

http://portage.stier.is-a-geek.com/net-www/mplayerplug-in/files/mplayerplugin-fastshutdown.diff

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Kevin DeKorte - 2006-06-02

Logged In: YES
user_id=685413

I looked at this patch, and while it may work, I'm not
entirely sure it is correct. The locks you removed are to
protect the "cancelled" variable. It is related to the code
at line 794 in plugin-threads.c. It is possible that the
code will segfault if the read occurs after the pipe from
mplayer has closed. The thread locking protects that from
happening. Also, if mplayer is hanging it is probably
related to the TCP/IP stack and the connection just has to
wait to timeout.

Basically, you can't cancel during a read, so this lock
protects that.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Kevin DeKorte - 2006-06-02

assigned_to: nobody --> kdekorte
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Cinquero - 2006-06-02

Logged In: YES
user_id=1018844

As far as I see it, the pipe read is blocking
(plugin_threads.cpp?v=cvs20060601#792). Correct? The best
solution would probably be to make it non-blocking and just
let the reader sleep for a moment when there is no input.

The current situation is not good. Every time a video stream
server does not respond (quickly or at all), the whole
browser is locked up entirely for quite some time.

Instead of making the read non-blocking, we could also just
kill the mplayer in case the lock cannot be acquired
immediately or within some small amount of time.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Cinquero - 2006-06-02

Logged In: YES
user_id=1018844

BTW: how may a concurrent access to the same variable
produce a segfault? I'm just curious. My imagination was
that it would just return some undefined result, ie. 788578433.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Kevin DeKorte - 2006-06-02

Logged In: YES
user_id=685413

The problem is not with the variable it is with the

fread(&c, 1, 1, local_td->instance->player);

If you pass that function a NULL, it can segfault and since
it is a thread there is no way to protect that call other
than a mutex lock. Even an if (local_td->instance->player !=
NULL) test before the call is not safe enough due to the
threading. I worked with particular crash point for a few
days trying several things to solve it, until I started
using locking. BSD does not appear to have this issue, but
has issues with the thread locking and that is why the
ifndef BSD is there.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Cinquero - 2006-06-03

Logged In: YES
user_id=1018844

Ok, that should be a saner ansatz. It uses select() to wait
for incoming pipe data. I have also create a pipe signalling
shutdown. That allow one to signal the controller thread of
shutdown without the need to acquire a lock before.
Actually, that allows to place the select() statement inside
the read_mutex lock area in the controller thread.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Cinquero - 2006-06-03

Logged In: YES
user_id=1018844

Just in case it gets unnoticed, I have attached the file
"latest.diff".

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Cinquero - 2006-06-03

use select() to avoid blocking reads

latest.diff

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Kevin DeKorte - 2006-06-03

Logged In: YES
user_id=685413

How portable is the code you have sent?

What are the FD_ZERO, FD_SET & FD_ISSET macros doing?

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Cinquero - 2006-06-03

Logged In: YES
user_id=1018844

man select: 4.4BSD, typically portable to system using the
BSD socket layer.

The macros work on the file descriptor sets.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Kevin DeKorte - 2006-06-03

Logged In: YES
user_id=685413

I'm looking at the code and deciding what to do with it. I
see some advantages with it, but I want to make sure I
understand it all prior to merging in the patch.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Cinquero - 2006-06-04

Logged In: YES
user_id=1018844

Have you read the select() man page? It's all described
there. Including a simple example.

man 2 select

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Kevin DeKorte - 2006-06-06

Logged In: YES
user_id=685413

Rather than using your patch, which I am still not 100% sure
is thread safe. I changed the I/O of the player FD to
NON_BLOCK..

Index: Source/plugin-threads.cpp

RCS file:
/cvsroot/mplayerplug-in/mplayerplug-in/Source/plugin-threads.cpp,v
retrieving revision 1.227
diff -r1.227 plugin-threads.cpp
173a174
> FILE *retfd;
235c236,244
< return fdopen(filedesr[0], "r");
---
> retfd = fdopen(filedesr[0], "r");
> flags = fcntl(fileno(retfd), F_GETFL, 0);
> flags |= O_NONBLOCK;
> #ifndef BSD
> flags |= O_NDELAY;
> #endif
> fcntl(fileno(retfd), F_SETFL, flags);
>
> return retfd;

Very simple patch, see if that works against current CVS for
you... I don't have a test case to see if it would work for
you 100%.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Kevin DeKorte - 2006-06-07

Logged In: YES
user_id=685413

Well that patch made another bug appear which I have fixed,
but also created a high CPU load when it was running. So I
changed the flag to O_ASYNC and I think everything works
properly for me. I can close a tab while mplayer is
"Connecting" so I think this helps some. I have committed my
changes to CVS. So try CVS or tomorrows daily code drop.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Cinquero - 2006-06-08

Logged In: YES
user_id=1018844

Just tried the current CVS version. Does not work. AFAIK
reads cannot be interrupted.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Kevin DeKorte - 2006-06-08

Logged In: YES
user_id=685413

Can you please provide a test case so I can see exactly what
you are seeing as I cannot duplicate the hang at this point.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Cinquero - 2006-06-09

Logged In: YES
user_id=1018844

http://www.heute.de/ZDFheute

Click on some video link adjoining the articles in the
center of the page. Format: Windows Media, speed DSL. When
it displays "Playing mms:...", try to close the window with
the mplayerplug-in frame in it -- it won't as long as you
see that "Playing mms:..." message.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Kevin DeKorte - 2006-06-09

Logged In: YES
user_id=685413

Ok, Thanks for the test case. That helped me solve the
problem. I am able to close the window now while that video
is trying to be loaded. Change has been committed to CVS.
And I also believe it is in the daily source code drop.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Kevin DeKorte - 2006-06-13

Logged In: YES
user_id=685413

Closing bug... as it should be fixed now

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Kevin DeKorte - 2006-06-13

status: open --> closed-fixed
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

quit command hangs when mplayer is connecting

Group

Searches

Help

#100 quit command hangs when mplayer is connecting

Discussion

Index: Source/plugin-threads.cpp