[Freeciv-Dev] [bug #24935] Version message not shown at Qt-client

[mir3x]

Update of bug #24935 (project freeciv):

  Status:  Ready For Test => Fixed  
 Assigned to:None => mir3x  
 Open/Closed:Open => Closed 


___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #24935] Version message not shown at Qt-client

[Louis Moureaux]

Follow-up Comment #10, bug #24935 (project freeciv):

Thank you Gtk for giving us a well-tested client :-) Fixed

Maybe the SDL client has the same problem

(file #28287, file #28288)
___

Additional Item Attachment:

File name: qt-metaserver-version-trunk-v3.patch Size:3 KB
File name: qt-metaserver-version-s25-v3.patch Size:5 KB


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #24935] Version message not shown at Qt-client

[Marko Lindqvist]

Follow-up Comment #9, bug #24935 (project freeciv):

That would be exactly what comment about gtk client implementation of
server_scan_error() says about the reason it does not have the
server_scan_finish() call at all.

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #24935] Version message not shown at Qt-client

[Louis Moureaux]

Follow-up Comment #8, bug #24935 (project freeciv):

> Is that still server_scan_finish() call
Yes

Using gdb to watch meta_scan, I could see the metaserver thread nulling it
just before the crash. I think what happens is:
- [metaserver thread] server_scan_error() is called
- [meta] server_scan_error() calls server_scan_finish()
- [meta] server_scan_error() locks a mutex
- [main thread] server_scan_finish() is called
- [main] server_scan_finish() tries to lock the mutex, waits
- [meta] server_scan_finish() frees meta_scan, unlocks the mutex
- [main] server_scan_finish() frees meta_scan again
If I'm right, the key problems are:
- if (meta_scan) happens before any mutex is locked
- meta_scan is a pointer and not updated atomically anyway
- meta_scan = NULL happens after all mutexes are unlocked
Does it look realistic ?


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #24935] Version message not shown at Qt-client

[Marko Lindqvist]

Follow-up Comment #7, bug #24935 (project freeciv):

> Client crashes with "double free or corruption" in
fc_client::destroy_server_scans at pages.cpp:903.

Is that still server_scan_finish() call, or does the patch change line numbers
so it's not? That would indicate that 'meta_scan' is still not NULL, so it's
not set to NULL by server_scan_error().

Anyway, "double server_scan_finish()" would be same as "double free" as it
does free for several objects.

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #24935] Version message not shown at Qt-client

[Louis Moureaux]

Follow-up Comment #6, bug #24935 (project freeciv):

> Its only visible when metaserver is up but returning errors.
Also fixed this one. To test, I modified default_server_host in client config
file to some random URL not allowing POST. The error appears in the status
bar. It won't appear in chat, but at least no crash.

> u have fc_client::eventFilter()
As I understand it, eventFilter() is for filtering events going to other
objects. I think [this->]installEventFilter(this) looks weird.

I still get a (maybe related) crash. To reproduce:
- Turn off network system-wide
- Connect to Network Game
- Cancel
Client crashes with "double free or corruption" in
fc_client::destroy_server_scans at pages.cpp:903. It's weird because there is
no free() nor delete there. In the attached GDB output, I interrupt the client
just after clicking Cancel, then let it continue and crash.

Patch is for trunk only because there is still a crash. Will make 2.x versions
if you ask.

(file #28284, file #28285)
___

Additional Item Attachment:

File name: qt-metaserver-version-trunk-v2.patch Size:3 KB
File name: qt-crashing-gdb.log.bz2Size:5 KB


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #24935] Version message not shown at Qt-client

[mir3x]

Follow-up Comment #5, bug #24935 (project freeciv):

it will still crash in that case : bug #24944
Its only visible when metaserver is up but returning errors.
Another thread will come and try just append_output something

and u have fc_client::eventFilter()
no need to create fc_client::event().

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #24935] Version message not shown at Qt-client

[Louis Moureaux]

Follow-up Comment #4, bug #24935 (project freeciv):

The patch is still not thread-safe because the main thread accesses the status
bar QLabel without locking mut_ex.

Attached patch is thread-safe *unless* the version message can be received
after either gui() or the QApplication are destroyed (need help here: is it
the case?). It is thread-safe because QCoreApplication::postEvent() is
thread-safe, and QObject::event() is called from the thread the object was
created in (see Thread Affinity in the QObject doc).

The trunk patch also applies to 2.6.

(file #28276, file #28277)
___

Additional Item Attachment:

File name: qt-metaserver-version-trunk.patch Size:3 KB
File name: qt-metaserver-version-s25.patch Size:4 KB


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #24935] Version message not shown at Qt-client

[mir3x]

Follow-up Comment #3, bug #24935 (project freeciv):

>> Also, the variable this_thread isn't destroyed properly. 

Mb bc its not variable but pointer, and its not allocated to new object.

(file #28274, file #28275)
___

Additional Item Attachment:

File name: qt_meta_msg.patch  Size:3 KB
File name: qt_meta_msg-ftrunk.patch   Size:2 KB


___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #24935] Version message not shown at Qt-client

[Louis Moureaux]

Follow-up Comment #2, bug #24935 (project freeciv):

The proposed fix isn't thread-safe, because the underlying Qt classes
(QStringList, QLabel) are not. You need a synchronization primitive (a mutex
or some kind of atomic variable). Thread-safety is easier to get right when
using Qt signals together with separation of concerns.

Also, the variable this_thread isn't destroyed properly.


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #24935] Version message not shown at Qt-client

[mir3x]

Update of bug #24935 (project freeciv):

  Status:None => Ready For Test 
 Planned Release: => 2.5.6, 2.6, 3.0
 Contains string changes:None => No 

___

Follow-up Comment #1:

Patch attached

(file #28235, file #28236)
___

Additional Item Attachment:

File name: qt_meta_msg-ftrunk.patch   Size:2 KB
File name: qt_meta_msg.patch  Size:2 KB


___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev