Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#5487 closed Bug (fixed)

Qt client crash when using "Open URL" action

Reported by: mike.dld Owned by: jordan
Priority: Normal Milestone: 2.83
Component: Qt Client Version: 2.82
Severity: Normal Keywords: backport-2.8x patch
Cc:

Description

Steps to reproduce:

  1. activate "Open..." from main menu, toolbar or using Ctrl+O hotkey
  2. cancel the dialog
  3. activate "Open URL..." from main menu, toolbar or using Ctrl+U hotkey

Result: application crashes inside TrMainWindow::addTorrent on access to myFileDialogOptionsCheck, where checkbox has already been destroyed while the pointer is not reset to NULL.

Attached is a possible [minimalistic] fix using QWeakPointer to track checkbox destruction. Note that it omits check for Prefs::OPTIONS_PROMPT as "Open URL" presumably should always result in dialog being shown (tell me if I'm wrong).

Attachments (3)

5487-track-checkbox-destruction.patch (2.0 KB) - added by mike.dld 8 years ago.
5487-dont-keep-private-pointer-to-checkbox.diff (3.6 KB) - added by jordan 8 years ago.
5487-dont-keep-private-pointer-to-checkbox-2.diff (4.4 KB) - added by mike.dld 8 years ago.

Download all attachments as: .zip

Change History (9)

Changed 8 years ago by mike.dld

comment:1 Changed 8 years ago by jordan

Mike, good catch.

So, two issues here.

  1. QWeakPointer isn't assignable from a raw pointer in Qt5, so this patch doesn't compile there.
  1. More importantly, IMO this fixes the wrong bug. Mainwin shouldn't be holding a pointer to a widget in a transient dialog. Even if we fixed the deletion bug, addTorrent(AddData?&) will do the Wrong Thing when we have >1 file dialog open at once and toggle their checkboxes to differing values.

One approach would be to get rid of the private pointer altogether, and look up the QCheckBox in AddTorrent?() iff caller() is a QFileDialog.

comment:2 Changed 8 years ago by jordan

  • Milestone changed from None Set to 2.83
  • Status changed from new to assigned

comment:3 Changed 8 years ago by mike.dld

I didn't notice the possibility to open several file dialogs at once, this surely makes situation even worse.

I generally agree with your patch (even though the code usually "smells" when someone uses sender()), but would consider moving options showing logic to TrMainWindow::addTorrents, which would allow to only determine whether to show options or not once.

comment:4 Changed 8 years ago by jordan

Yeah, that is better. That way the number of hops from slot to the sender() call is shorter. Also I like the way you made addTorrents() private.

comment:5 Changed 8 years ago by jordan

  • Resolution set to fixed
  • Status changed from assigned to closed

comment:6 Changed 8 years ago by jordan

  • Keywords backport-2.8x added
Note: See TracTickets for help on using tickets.