After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 740995 - Starting totem with a file as parameter adds the file twice to the playlist since 3.14.1
Starting totem with a file as parameter adds the file twice to the playlist s...
Status: RESOLVED FIXED
Product: totem
Classification: Core
Component: Movie player
3.14.x
Other Linux
: Normal normal
: ---
Assigned To: General Totem maintainer(s)
General Totem maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2014-12-01 17:33 UTC by Sebastian Keller
Modified: 2015-04-22 11:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
main: Fix Totem doubling files added on the cmd line (797 bytes, patch)
2015-01-06 22:28 UTC, Bastien Nocera
committed Details | Review
Fix Totem doubling files added on the cmd line (bgo#740995). (3.42 KB, patch)
2015-04-17 13:10 UTC, Carlos Maddela
none Details | Review
Original Fix (938 bytes, patch)
2015-04-21 23:43 UTC, Carlos Maddela
none Details | Review
Fix Without Atomic Calls (3.08 KB, patch)
2015-04-21 23:46 UTC, Carlos Maddela
none Details | Review
main: Fix (again) doubling files added on the cmd line (2.84 KB, patch)
2015-04-22 11:38 UTC, Bastien Nocera
committed Details | Review

Description Sebastian Keller 2014-12-01 17:33:41 UTC
Make sure totem is currently not running and then start it via "totem somefile.mp4". The file has now been added to the playlist twice.
Comment 1 surabhi 2014-12-28 11:40:34 UTC
Correct me Sebastian if i am wrong but how did u saw the playlist because i don’t know if there there is any option to see the playlist in totem ,and also when i played a file using totem i don’t see it playing again once finished.
I am  currently using opensuse 13.2 64 bit and gnome 3.14.0 and totem 3.14
Comment 2 Sebastian Keller 2014-12-28 15:49:26 UTC
I'm not aware of any way to display the playlist directly, but the previous/next buttons go through the entries in the playlist. When opening a file using the totem binary directly rather than using bus activation, the "next" button gets enabled and clicking it plays the same file again, so it got added to the playlist twice. Opening a file in nautilus doesn't run the totem binary directly but rather uses bus activation, so this won't show the problem. But if you use something like gpodder (or the commandline) to launch totem, this runs the binary and results in the file being added twice. You also can't see this behavior in 3.14.0 because the bug got introduced in 3.14.1. I put 3.14.2 in the title of this report by mistake, sorry for the confusion.
Comment 3 Sylvain Pasche 2014-12-30 03:49:53 UTC
I can confirm this also happens with the latest totem git version.

I played a bit with gdb and noticed that when totem is not already running and is launched with some files as arguments, totem_pl_parser_parse_async is called twice resulting in duplicates items in the playlist. Here are the two stacks:

$ gdb --args totem foo.m3u

(gdb) b totem_pl_parser_parse_async
(gdb) r
Starting program: /home/spasche/jhbuild/install/bin/totem foo.m3u

Breakpoint 1, totem_pl_parser_parse_async (parser=0xca5ab0, uri=0x1226ad0 "file:///home/spasche/foo.m3u", fallback=0, cancellable=0x0, callback=0x7ffff7b86eb4 <add_mrls_cb>, user_data=0x1223900)
    at totem-pl-parser.c:2149
2149		totem_pl_parser_parse_with_base_async (parser, uri, NULL, fallback, cancellable, callback, user_data);
(gdb) bt
  • #0 totem_pl_parser_parse_async
    at totem-pl-parser.c line 2149
  • #1 totem_playlist_add_mrls
    at totem-playlist.c line 1351
  • #2 totem_object_open_files_list
    at totem-object.c line 2709
  • #3 totem_object_open_files
    at totem-object.c line 2634
  • #4 totem_object_app_activate
    at totem-object.c line 246
  • #5 _g_closure_invoke_va
    at gclosure.c line 831
  • #6 g_signal_emit_valist
    at gsignal.c line 3201
  • #7 g_signal_emit
    at gsignal.c line 3348
  • #8 remote_command_cb
    at totem-menu.c line 281
  • #9 g_closure_invoke
    at gclosure.c line 768
  • #10 signal_emit_unlocked_R
    at gsignal.c line 3536
  • #11 g_signal_emit_valist
    at gsignal.c line 3292
  • #12 g_signal_emit
    at gsignal.c line 3348
  • #13 g_simple_action_activate
    at gsimpleaction.c line 211
  • #14 g_action_activate
    at gaction.c line 390
  • #15 totem_send_remote_command
    at totem-options.c line 77
  • #16 totem_options_process_for_server
    at totem-options.c line 114
  • #17 totem_object_app_handle_local_options
    at totem-object.c line 272
  • #18 ffi_call_unix64
    from /lib64/libffi.so.6
  • #19 ffi_call
    from /lib64/libffi.so.6
  • #20 g_cclosure_marshal_generic_va
    at gclosure.c line 1541
  • #21 _g_closure_invoke_va
    at gclosure.c line 831
  • #22 g_signal_emit_valist
    at gsignal.c line 3201
  • #23 g_signal_emit
    at gsignal.c line 3348
  • #24 g_application_real_local_command_line
    at gapplication.c line 961
  • #25 g_application_run
    at gapplication.c line 2259
  • #26 main
    at totem.c line 83
  • #0 totem_pl_parser_parse_async
    at totem-pl-parser.c line 2149
  • #1 totem_playlist_add_mrl
    at totem-playlist.c line 1070
  • #2 totem_object_remote_command
    at totem-object.c line 2850
  • #3 remote_command_cb
    at totem-menu.c line 288
  • #4 g_closure_invoke
    at gclosure.c line 768
  • #5 signal_emit_unlocked_R
    at gsignal.c line 3536
  • #6 g_signal_emit_valist
    at gsignal.c line 3292
  • #7 g_signal_emit
    at gsignal.c line 3348
  • #8 g_simple_action_activate
    at gsimpleaction.c line 211
  • #9 g_action_activate
    at gaction.c line 390
  • #10 totem_send_remote_command
    at totem-options.c line 77
  • #11 totem_options_process_for_server
    at totem-options.c line 114
  • #12 totem_object_app_handle_local_options
    at totem-object.c line 272
  • #13 ffi_call_unix64
    from /lib64/libffi.so.6
  • #14 ffi_call
    from /lib64/libffi.so.6
  • #15 g_cclosure_marshal_generic_va
    at gclosure.c line 1541
  • #16 _g_closure_invoke_va
    at gclosure.c line 831
  • #17 g_signal_emit_valist
    at gsignal.c line 3201
  • #18 g_signal_emit
    at gsignal.c line 3348
  • #19 g_application_real_local_command_line
    at gapplication.c line 961
  • #20 g_application_run
    at gapplication.c line 2259
  • #21 main
    at totem.c line 83

Comment 4 Bastien Nocera 2015-01-06 22:28:43 UTC
Created attachment 293988 [details] [review]
main: Fix Totem doubling files added on the cmd line
Comment 5 Bastien Nocera 2015-01-13 20:47:42 UTC
Attachment 293988 [details] pushed as 751c1da - main: Fix Totem doubling files added on the cmd line
Comment 6 Sebastian Keller 2015-02-08 08:29:12 UTC
Reopening this bug as I can still reproduce the problem in 3.14.2.
Comment 7 Carlos Maddela 2015-04-17 13:10:27 UTC
Created attachment 301834 [details] [review]
Fix Totem doubling files added on the cmd line (bgo#740995).

This bug is reproducible in Ubuntu Vivid's Totem (Version 3.14.2-0ubuntu1).

The problem results from the fact that the command line files are both processed in totem_options_process_for_server() and totem_object_app_activate().  When I traced the program, totem_options_process_for_server() always seemed to be called before totem_object_app_activate(), so I'm not sure if my solution is a little overkill. I don't know if there would ever be cases where the handlers would be called in reverse, or even repeatedly called. The solution should be sufficient if this is the case.
Comment 8 Bastien Nocera 2015-04-21 18:15:35 UTC
(In reply to Carlos Maddela from comment #7)
> Created attachment 301834 [details] [review] [review]
> Fix Totem doubling files added on the cmd line (bgo#740995).
> 
> This bug is reproducible in Ubuntu Vivid's Totem (Version 3.14.2-0ubuntu1).
> 
> The problem results from the fact that the command line files are both
> processed in totem_options_process_for_server() and
> totem_object_app_activate().

That should be:
totem_object_app_activate
and
totem_object_app_handle_local_options

Both of which are GApplication class methods.

>  When I traced the program,
> totem_options_process_for_server() always seemed to be called before
> totem_object_app_activate(), so I'm not sure if my solution is a little
> overkill.

From the docs, ->handle_local_options is called after parsing of command-line options has occurred. Looking at g_application_real_local_command_line() in glib/gio/gapplication.c ->handle_local_options will be called before activate (if activate is called rather than ->open).

> I don't know if there would ever be cases where the handlers would
> be called in reverse, or even repeatedly called. The solution should be
> sufficient if this is the case.

I don't think that those 2 functions can be called in parallel in any case, so I'm not too sure what this patch is trying to achieve. 

Can you explain how you got to this fix?
Comment 9 Carlos Maddela 2015-04-21 23:39:54 UTC
(In reply to Bastien Nocera from comment #8)
> (In reply to Carlos Maddela from comment #7)
> > Created attachment 301834 [details] [review] [review] [review]
> > Fix Totem doubling files added on the cmd line (bgo#740995).
> > 
> > This bug is reproducible in Ubuntu Vivid's Totem (Version 3.14.2-0ubuntu1).
> > 
> > The problem results from the fact that the command line files are both
> > processed in totem_options_process_for_server() and
> > totem_object_app_activate().
> 
> That should be:
> totem_object_app_activate
> and
> totem_object_app_handle_local_options
> 
> Both of which are GApplication class methods.
> 
> >  When I traced the program,
> > totem_options_process_for_server() always seemed to be called before
> > totem_object_app_activate(), so I'm not sure if my solution is a little
> > overkill.
> 
> From the docs, ->handle_local_options is called after parsing of
> command-line options has occurred. Looking at
> g_application_real_local_command_line() in glib/gio/gapplication.c
> ->handle_local_options will be called before activate (if activate is called
> rather than ->open).
> 
> > I don't know if there would ever be cases where the handlers would
> > be called in reverse, or even repeatedly called. The solution should be
> > sufficient if this is the case.
> 
> I don't think that those 2 functions can be called in parallel in any case,
> so I'm not too sure what this patch is trying to achieve. 
> 
> Can you explain how you got to this fix?

I did say that it may be a little overkill, but I just went for the safer option.

My original solution was just to remove the code that handled the command line parameters from totem_object_app_activate(). While this worked fine for the limited tests I did, I thought it had a good chance of introducing side effects.

So my second solution was based on your attempt to fix this bug: by nullifying optionstate.filenames to indicate the command line has already been processed.  Your attempt didn't work because totem_object_app_handle_local_options() is called before totem_object_app_activate(). I've just realised though that my solution resorts to accessing the file names through the global optionstate struct, when it should be accessing it through a const pointer. So it's introduced some ugly code.

As for concurrency issues, my first attempt to remove the atomic pointer calls from my solution resulted in a crash. It seems like you need to nullify optionstate.filenames very early on before other function calls are made. I'm still not entirely convinced that this would be safe code.

I'll attach these patches for you to review.
Comment 10 Carlos Maddela 2015-04-21 23:43:46 UTC
Created attachment 302100 [details] [review]
Original Fix
Comment 11 Carlos Maddela 2015-04-21 23:46:53 UTC
Created attachment 302102 [details] [review]
Fix Without Atomic Calls
Comment 12 Bastien Nocera 2015-04-22 11:38:53 UTC
Created attachment 302141 [details] [review]
main: Fix (again) doubling files added on the cmd line

When handling local command-line options, we would process all the
options and send them as "remote" commands through totem. But the
"remote-command" action would activate the application first. Thus we
would do something like:
- process command-line options in totem_object_app_handle_local_options()
which calls totem_options_process_for_server(), which sends out
"remote-command" actions
- when handling "remote-command", to make sure that the application is
actually ready, we call "activate".
- but we didn't clear the filenames struct member in optionstate, so we
process it again in "activate"

With help from Carlos Maddela <maddela@labyrinth.net.au>
Comment 13 Bastien Nocera 2015-04-22 11:39:43 UTC
Attachment 302141 [details] pushed as ea29374 - main: Fix (again) doubling files added on the cmd line