Differential D5919

Fix crash when closing tabs with search box open
AbandonedPublic
Actions

Authored by elvisangelaccio on May 19 2017, 4:13 PM.

Details

Reviewers
anthonyfieroni
emmanuelp
Summary

Fixes a regression introduced by 6af0dad2eeba.

DolphinViewContainer::setActive() unconditionally calls setActive() on
the url navigator, view and search box. However this has side effects,
as DolphinSearchBox activates itself if it receives a FocusIn event.

This is what happens in the bug:

  1. The searchbox of the first tab gets disactivated (as side effect of disactivating its container).
  2. Second tab gets closed, we call removeTab(1) in the tab widget.
  3. The searchbox of the first tab gets a FocusIn event and calls setActive(true).
  4. Since it was disactivated, it emits activated() and this results in the activeViewChanged() signal which causes the crash, because the currentIndex() of the tab page is still 1 but its widget has already been invalidated.

This patch temporarily delays the activation of the search box, so that
we are sure that activeViewChanged() is emitted only after the tab has
been properly removed.

BUG: 379135
FIXED-IN: 17.04.2

Test Plan

Open dolphin, open the search box, open a new tab, close the new tab.

Diff Detail

Repository
R318 Dolphin
Lint
Lint Skipped
Unit
Unit Tests Skipped
Restricted Application added subscribers: Dolphin, Konqueror. · View Herald TranscriptMay 19 2017, 4:13 PM
elvisangelaccio edited the summary of this revision. (Show Details)May 19 2017, 4:14 PM
anthonyfieroni edited edge metadata.May 19 2017, 5:12 PM

It's not so easy.

  1. Open split view
  2. Open 2-nd tab
  3. Return to split view

4 Bug 1: 2 views kurlnavigator is inactive Bug 2: right click on non-focused view is not triggered

elvisangelaccio added a comment.May 19 2017, 5:23 PM

You are right, thanks for testing. We need to find another fix.

elvisangelaccio planned changes to this revision.May 19 2017, 5:23 PM
anthonyfieroni added a comment.May 19 2017, 5:39 PM

It has a basically one problem:

  1. setActive(false) to previous tab with split view by clicking on other tab
  2. click back to tab with split view -> setActive(true) cause DolphinTabPage::slotViewActivated (view active from false to true)
elvisangelaccio added a comment.EditedMay 19 2017, 6:09 PM

Bah, I give up. Can't figure out a clean way to fix it.
At this point I think we can go with @anthonyfieroni's initial fix (i.e. just check if tabPage is valid in DolphinMainWindow::updateSplitAction()).

elvisangelaccio added a comment.May 19 2017, 6:15 PM
In D5919#110836, @elvisangelaccio wrote:

Bah, I give up. Can't figure out a clean way to fix it.
At this point I think we can go with @anthonyfieroni's initial fix (i.e. just check if tabPage is valid in DolphinMainWindow::updateSplitAction()).

Even though it's conceptually wrong that DolphinTabWidget::currentTabPage() returns a nullptr. The other fix I can think of is disconneting the activeViewChanged signal before removing a tab.

elvisangelaccio updated this revision to Diff 14694.May 19 2017, 6:37 PM
elvisangelaccio edited the summary of this revision. (Show Details)
  • Disconnect the activeViewChanged signal before removing the tab.
elvisangelaccio edited the summary of this revision. (Show Details)May 19 2017, 6:38 PM
anthonyfieroni added a comment.May 19 2017, 7:06 PM

You can't do this since DolphinMainWindow::activeViewChanged have a dangling pointer to viewContainer or use QPointer for m_activeViewContainer :)

elvisangelaccio updated this revision to Diff 14705.May 20 2017, 8:39 AM
elvisangelaccio edited the summary of this revision. (Show Details)
elvisangelaccio added a project: Dolphin.

New strategy: just delay the activation of the search box.

anthonyfieroni added a comment.EditedMay 20 2017, 9:53 AM

This is clearly a hack and it will became a worst case. It has one more bug, if you not noticed it.

  1. Open split view (wih different urls)
  2. Open 2-nd tab
  3. Switching between tabs, split view changes internal view, this is because view moves from inactive to active and fire DolphinTabPage::slotViewActivated
elvisangelaccio added a comment.May 20 2017, 10:59 AM
In D5919#110911, @anthonyfieroni wrote:

This is clearly a hack and it will became a worst case. It has one more bug, if you not noticed it.

  1. Open split view (wih different urls)
  2. Open 2-nd tab
  3. Switching between tabs, split view changes internal view, this is because view moves from inactive to active and fire DolphinTabPage::slotViewActivated

This happens also on unpatched 17.04 branch, it seems.

anthonyfieroni added a comment.May 20 2017, 11:18 AM

Introduced by same commit ;)

elvisangelaccio abandoned this revision.May 24 2017, 9:30 AM

Superseded by D5864.

Revision Contents
Changeset List

PathPackages
Msrc/search/dolphinsearchbox.cpp (9 lines)

Diff 14705

View Options

src/search/dolphinsearchbox.cpp

Loading...