#15420 closed defect (fixed)

MaxWidthCalculator bug in wxDataViewCtrl

Reported by: Spencer_Parkin Owned by:
Priority: normal Milestone: 3.0.0
Component: GUI-generic Version:
Keywords: Cc: vaclavslavik
Blocked By: Blocking:
Patch: yes

Description

While using the wxDataViewCtrl, if the user tries to fit a column to optimal width, this may fail with an index out-of-bounds error.

I believe that the problem is in the MaxWidthCalculator class that is defined locally within the function GetBestColumnWidth of wxDataViewCtrl. This class erroneously uses the "m_column" member to index into the columns of the control.

The wxDataViewCtrl should support a routine called something like "GetModelColumnAt", which searches its column list for the column having the given identifier, which identifier _is not_ an index.

For more information about this bug, please see the following post in wxForum.

http://forums.wxwidgets.org/viewtopic.php?f=1&t=37895

I would submit a patch, but I am not familiar with SVN.

Attachments (2)

0001-fix-bug-in-GetBestColumnWidth.patch download (5.4 KB) - added by Spencer_Parkin 14 months ago.
A patch created against the git mirror of the latest wxWidget SVN repo.
0001-expose-bug-in-auto-resize-column-header-calculator.patch download (1.3 KB) - added by Spencer_Parkin 13 months ago.
This patch exposes the auto-resize column header calculation bug

Download all attachments as: .zip

Change History (13)

comment:1 Changed 14 months ago by vadz

I'll try to have a look at this but a patch would be welcome and can be done without svn, please see our HowToSubmitPatches guide. If you use Git, you can also submit a pull request to wxWidgets repository on Github.

Also, it would be really useful to know how can the problem be reproduced and, ideally, have a unit test verifying that it's not only fixed now but remains fixed in the future.

Changed 14 months ago by Spencer_Parkin

A patch created against the git mirror of the latest wxWidget SVN repo.

comment:2 Changed 14 months ago by Spencer_Parkin

I just added an attachment: a patch created using git.

Is that it? Do I still need to do the pull request?

I've tested the changes with my own program that uses the wxDataViewCtrl. Even if the patch can't be put into wxWidgets as is, I think that someone should take a look at it when they have the time.

Thanks. Please let me know if you have any further questions.

comment:3 Changed 14 months ago by Spencer_Parkin

Sigh...

The line containing exactly "(column == 0
" in "datavgen.cpp" should probably be removed. That condition _should_ never happen, so it's not hurting anything by being there, but it should probably be removed.

comment:4 Changed 14 months ago by vadz

  • Cc vaclavslavik added
  • Milestone set to 3.0

Thanks for the patch, that is it, no need for the pull request.

I'll try to look at this soon (or maybe Vaclav will as he wrote this code relatively recently if I'm not mistaken), the only remaining question is how can the bug be reproduced? It would be really helpful to check that it's fixed by these changes.

Thanks again!

comment:5 Changed 14 months ago by vadz

  • Patch set

comment:6 Changed 14 months ago by Spencer_Parkin

Here's a repro...

1) Create a wxDataViewCtrl with 3 columns, each having "model column" identifiers of 0, 1, and 2.
2) Delete the column having identifier 1.
3) Double click on the column header boundary for the column having identifier 2.

You then get an array out-of-bounds indexing assert, because wxWidgets tries to use a "model column" identifier of 2 to index into an array of size 2.

comment:7 Changed 14 months ago by vadz

Sorry, there must be more needed to reproduce this because AFAICS step (1) is already exactly what the second page of the dataview sample does, but applying this patch as step 2:

  • samples/dataview/dataview.cpp

    diff --git a/samples/dataview/dataview.cpp b/samples/dataview/dataview.cpp
    index 5deed63..e0ebd96 100644
    a b void MyFrame::BuildDataViewCtrl(wxPanel* parent, unsigned int nPanel, unsigned l 
    637637                                     new MyCustomRenderer, 
    638638                                     MyListModel::Col_Custom) 
    639639            ); 
     640 
     641            m_ctrl[1]->DeleteColumn(m_ctrl[1]->GetColumn(MyListModel::Col_IconText)); 
    640642        } 
    641643        break; 
    642644 

and then doing the step (3) manually doesn't seem to result in any problems.

As I still have some trouble understanding what exactly the patch does (i.e. which indices are model indices and which are the column positions in the control, exactly?), I'd really like to reproduce the problem before applying it. Could you please try to make a minimal patch to the dataview sample showing the bug?

Thanks again!

Changed 13 months ago by Spencer_Parkin

This patch exposes the auto-resize column header calculation bug

comment:8 Changed 13 months ago by Spencer_Parkin

I have added a patch to the sample that, when applied, exposes the bug. I have also verified that the patch I originally gave you fixes the problem in this case.

It is worth mentioning that what led to this bug, I believe, is a problem with variable naming semantics, and so I believe it may be worth combing over the wxDataViewCtrl and associated code to find all places, if any, where the proper meaning of "model_column" has been violated. For example, I should be able to choose "model_column" IDs of 999, 123, -4567, and so on, as "model_column" identifiers, (if I understand the API correctly), and this should be _okay_ to do. If the internal code ever tries to use a "model_column" as an index into the list of columns, then I believe that that is not what the original author of the wxDataViewCtrl intended.

Thanks for writing wxWidgets. I like it.

comment:9 Changed 13 months ago by Spencer_Parkin

Hi, I'm just writing here to touch base and ask how things are going with this bug.

If there is anything further I can do to help, please let me know. Did the repro-patch help?

comment:10 Changed 13 months ago by vadz

  • Status changed from new to confirmed

No, everything is perfect, thanks a lot for the bug report, the patch, the explanation, the test case and finally for your kind words. I simply didn't have time to look at this until now but now that I do, I agree that this is definitely the right thing to do and will commit your fix soon. And yes, we probably should review wxDVC code for more cases of mixups like this because I wouldn't be surprised at all to find more of them, the difference between model and view column indices is tricky and is, unfortunately, absolutely not enforced by the C++ type system (just to be clear: this is our fault for writing the code this way, not fault of C++).

The only thing that I changed was to remove the method documentation as it's private, in the sense that it's not part of the official API, if only because it only exists in the generic version and so is unavailable under the other platforms (but also because it's mostly an implementation detail anyhow).

Thanks again!

comment:11 Changed 13 months ago by VZ

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

(In [74889]) Fix crash when auto-sizing a wxDataViewCtrl column.

The code was confused about the difference between the model and view columns
indices and incorrectly used the former as the latter, which could result in
an out of bound array access.

Closes #15420.

Note: See TracTickets for help on using tickets.