Opened 9 months ago

Closed 6 months ago

Last modified 6 months ago

#15980 closed defect (fixed)

Appended text is truncated in multi-line wxTextCtrl

Reported by: Malco Owned by: VZ
Priority: normal Milestone: 3.0.1
Component: wxMSW Version: dev-latest
Keywords: wxTextCtrl AppendText Cc:
Blocked By: Blocking:
Patch: yes

Description

Dear all,

I found a bug with wxTextCtrl. When I append a long text, occasionally few characters are not inserted.
I am using wxWidgets-3.0.0 and building with MinGW32-4.8.1.
The easyiest way to reproduce the aforementioned issue is to build simple window including the following code. Then look at lines 133/134 of wxTextCtrl:

wxTextCtrl *textLog = new wxTextCtrl(panel, wxID_ANY, wxEmptyString, wxDefaultPosition, wxDefaultSize, wxTE_MULTILINE);
char txt[1000];
for (int i = 0; i < 150; i++) {

sprintf(txt, "[%3d] ", i);
textLog->AppendText(txt);
textLog->AppendText("aaazzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttyyyaaazzzeeerrrtttaaazzzeeerrrttteerrraaazzzeeerrraaazzzeeerrraaazzzeeerrraaazzzeeerrrtttaaazzzeeerrrtttaaazzzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttaaazzz\n");

}

You can also compile the attached cpp file.
What I got is the following:

[131] aaazzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttyyyaaazzzeeerrrtttaaazzzeeerrrttteerrraaazzzeeerrraaazzzeeerrraaazzzeeerrraaazzzeeerrrtttaaazzzeeerrrtttaaazzzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttaaazzz
[132] aaazzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttyyyaaazzzeeerrrtttaaazzzeeerrrttteerrraaazzzeeerrraaazzzeeerrraaazzzeeerrraaazzzeeerrrtttaaazzzeeerrrtttaaazzzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttaaazzz
[133] aaazzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttyyyaaazzzeeerrrtttaaazzzeeerrrttteerrraaazzzeeerrraaazzzeeerrraaazzzeeerrraaazzzeeerrrtttaaazzzeeerrrtttaaazzzzeeerrrtttaaazzzeeerrrtttaaazzze[134] aaazzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttyyyaaazzzeeerrrtttaaazzzeeerrrttteerrraaazzzeeerrraaazzzeeerrraaazzzeeerrraaazzzeeerrrtttaaazzzeeerrrtttaaazzzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttaaazzz
[135] aaazzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttyyyaaazzzeeerrrtttaaazzzeeerrrttteerrraaazzzeeerrraaazzzeeerrraaazzzeeerrraaazzzeeerrrtttaaazzzeeerrrtttaaazzzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttaaazzz
[136] aaazzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttyyyaaazzzeeerrrtttaaazzzeeerrrttteerrraaazzzeeerrraaazzzeeerrraaazzzeeerrraaazzzeeerrrtttaaazzzeeerrrtttaaazzzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttaaazzz
[137] aaazzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttyyyaaazzzeeerrrtttaaazzzeeerrrttteerrraaazzzeeerrraaazzzeeerrraaazzzeeerrraaazzzeeerrrtttaaazzzeeerrrtttaaazzzzeeerrrtttaaazzzeeerrrtttaaazzzeeerrrtttaaazzz

Good luck fixing this. Any way, thank you and congratulation for the high quality of wxWidgets.

Regards,
Malco.

Attachments (7)

MainWindow.cpp download (2.8 KB) - added by Malco 9 months ago.
Simple to reproduce the bug of wxTextCtrl
MainWindow.2.cpp download (2.8 KB) - added by Malco 9 months ago.
Sample to reproduce the bug of wxTextCtrl
textctrl-append.patch download (1.3 KB) - added by awi 8 months ago.
Patch to fix the issue.
textctrl-append_v2.patch download (2.6 KB) - added by awi 8 months ago.
New patch to fix the issue.
unit-test.patch download (1.8 KB) - added by awi 8 months ago.
Unit test.
textctrl-append-2.patch download (3.5 KB) - added by awi 7 months ago.
Length of inserted text is not a global value.
textctrl-load-file.patch download (646 bytes) - added by awi 7 months ago.
Patch fixing the issue with loading the text.

Download all attachments as: .zip

Change History (34)

Changed 9 months ago by Malco

Simple to reproduce the bug of wxTextCtrl

Changed 9 months ago by Malco

Sample to reproduce the bug of wxTextCtrl

comment:1 Changed 9 months ago by Malco

  • Status changed from new to confirmed

Changed 8 months ago by awi

Patch to fix the issue.

comment:2 Changed 8 months ago by awi

  • Keywords AppendText added
  • Patch set
  • Summary changed from Issue with appending text in multi-line wxTextCtrl to Appended text is trunctated in multi-line wxTextCtrl
  • Version changed from 3.0.0 to dev-latest

The reason of the problem is that when text size limit (apparently 30000 characters) for wxTextCtrl is reached then text buffer is automatically extended (in EN_MAXTEXT message handler) but just appended/inserted text is truncated (see http://msdn.microsoft.com/en-us/library/windows/desktop/bb761684%28v=vs.85%29.aspx).

To fix the issue there is necessary to check if the whole string has been successfully appended and if not then append it again - patch attached.

comment:3 Changed 8 months ago by Malco

  • Summary changed from Appended text is trunctated in multi-line wxTextCtrl to Appended text is truncated in multi-line wxTextCtrl

comment:4 Changed 8 months ago by vadz

Great, thanks for finding and fixing this!

Just one question: I'm a bit uncomfortable with potentially infinite loops. Could we perhaps increase the length limit before trying to insert the text? Or, at least store the required max length and set it from SetMaxLength() instead of just adding 32KB? Then we could, AFAICS, limit the loop execution to a single iteration only. And it would probably be more efficient...

Finally, it'd be really great to have a unit test this as it seems like something fragile enough to break again in the future...

Changed 8 months ago by awi

New patch to fix the issue.

Changed 8 months ago by awi

Unit test.

comment:5 Changed 8 months ago by awi

In the new patch the text buffer is extended in one step so in case of buffer overflow there is necessary to redo the operation only once.

Patch to build unit test is also attached.

comment:6 Changed 8 months ago by vadz

This is perfect, thanks! I've done only a few minor changes:

  1. Made the variable you added global instead of member. The comment I added explains one reason for it, but the other one is that like this I can apply the same patch to the trunk and 3.0, without breaking ABI in the latter.
  2. Fix the order of CPPUNIT_ASSERT_EQUAL() arguments (which you put in a wrong order as everybody else, including me, naturally does... I have no idea why but the "actual" value must come first).
  3. Slightly changed the variable semantics and renamed it as it seems more clear to me like this.

Please let me know if you have any objections to any of these changes.

And thanks again!

comment:7 Changed 8 months ago by VZ

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

(In [75936]) Fix wxTextCtrl contents corruption with long strings in wxMSW.

wxMSW automatically extended wxTextCtrl length limit beyond the tiny standard
32KB when it was exceeded, but part of the text being appended into the
control was lost when doing it.

Fix this by retrying insertion after extending the limit.

Closes #15980.

comment:8 Changed 8 months ago by VZ

(In [75940]) Fix wxTextCtrl contents corruption with long strings in wxMSW.

wxMSW automatically extended wxTextCtrl length limit beyond the tiny standard
32KB when it was exceeded, but part of the text being appended into the
control was lost when doing it.

Fix this by retrying insertion after extending the limit.

Closes #15980.

comment:9 Changed 8 months ago by VZ

(In [76058]) Fix harmless signed/unsigned comparison warning in a test.

Don't compare int with unsigned to avoid warnings that were introduced by the
changes of r75940.

See #15980.

comment:10 Changed 8 months ago by VZ

(In [76059]) Fix harmless signed/unsigned comparison warning in a test.

Don't compare int with unsigned to avoid warnings that were introduced by the
changes of r75936.

See #15980.

comment:11 Changed 7 months ago by vaclavslavik

  • Resolution fixed deleted
  • Status changed from closed to reopened

Unfortunately, the above patch breaks rich text controls' Replace() in interesting ways (tested on 3.0 branch).

To reproduce, apply this patch to samples/text

@@ -1444,6 +1444,8 @@ void MyFrame::OnQuit (wxCommandEvent& WXUNUSED(event) )
 
 void MyFrame::OnAbout( wxCommandEvent& WXUNUSED(event) )
 {
+    m_panel->m_textrich->Replace(4, 4, "Foo");
+#if 0
     wxBeginBusyCursor();
 
     wxMessageDialog dialog(this,
@@ -1458,6 +1460,7 @@ void MyFrame::OnAbout( wxCommandEvent& WXUNUSED(event) )
     dialog.ShowModal();
 
     wxEndBusyCursor();
+#endif
 }
 
 #if wxUSE_TOOLTIPS

This should keep adding "Foo" into the richtext control when you press Alt+A. It does the first time, subsequent uses have no effect. If you paste something else into the control, then the next Alt+A inserts "Foo", but subsequent ones won't. (In the current Poedit, this creates downright bizzare behavior with moving text that's hard to describe. This test is simpler and hopefully sufficient. Reverting r75940 fixes both Poedit and this testcase.)

comment:12 Changed 7 months ago by vadz

I'll look at this. Does the problem only happen with rich text controls? If so, we could probably just disable the patch for them...

comment:13 Changed 7 months ago by vadz

  • Milestone set to 3.0.1
  • Status changed from reopened to confirmed

I have to leave right now but I think there are at least 2 problems here:

  1. 0 shouldn't be used as a special value for gs_lenOfInsertedText because it's not special at all if an empty string is being written (which makes sense with selectionOnly == true and is how Remove() works). We should use -1 or something like that instead.
  2. Global gs_lenOfInsertedText can't be used at all because the code can be reentered if inserting the text into the control results in inserting more text into (some other) control from wxEVT_TEXT handler -- as it happens in the sample. We need to have global array/map instead of a single global variable to deal with this.

Changed 7 months ago by awi

Length of inserted text is not a global value.

comment:14 Changed 7 months ago by awi

If we made the variable storing length of inserted text (lenOfInsertedText) not a global one but member of the class then there would not be necessary to implement external objects to store and synchronize parameters of asynchronous EN_MAXTEXT calls.
Patch implementing this approach attached. It seems to fix the issue.

comment:15 Changed 7 months ago by vadz

The trouble is that I'd like to have something that works in 3.0 too, which rules out using an extra member variable. I'll implement the global map based idea...

Also, I still think that comparing the length of the window with the length of text being inserted is wrong, it could fail to detect truncation if only a small selection is replaced by a big insertion, couldn't it?

comment:16 Changed 7 months ago by vadz

Vaclav, could you please test this?

  • src/msw/textctrl.cpp

    commit e2fcb87fec40a2fff91c6ac0d521352fc6130062
    Author: Vadim Zeitlin <vadim@wxwidgets.org>
    Date:   Mon Mar 24 00:13:32 2014 +0100
    
        Fix reentrancy issues in wxMSW wxTextCtrl max length handling code.
        
        The changes of r75940 didn't work correctly if the handler of wxEVT_TEXT in
        some text control modified a (potentially) different text control, as the same
        global variable was reused with disastrous results. Avoid this by keeping a
        stack of insertion lengths instead.
        
        Using a per-control field would work too, but would be a bit wasteful as the
        size of the stack will rarely be more than 1 (and never much more) and this
        change can also be applied to 3.0 branch without breaking ABI.
        
        Additionally, fix another problem in r75940 which used 0 as a special marker
        for the insertion length, which result in redoing each insertion of empty
        string (which is another word for Remove()) twice unnecessarily, by using -1
        instead.
    
    diff --git a/src/msw/textctrl.cpp b/src/msw/textctrl.cpp
    index af37e0d..b7f2553 100644
    a b  
    4040    #include "wx/wxcrtvararg.h" 
    4141#endif 
    4242 
     43#include "wx/stack.h" 
    4344#include "wx/sysopt.h" 
    4445 
    4546#if wxUSE_CLIPBOARD 
    class UpdatesCountFilter 
    176177namespace 
    177178{ 
    178179 
    179 // The length of the text being currently inserted into the control. 
     180// This stack stores the length of the text being currently inserted into the 
     181// current control. 
    180182// 
    181 // This is used to pass information from DoWriteText() to AdjustSpaceLimit() 
    182 // and is global as text can only be inserted into one text control at a time 
    183 // by a single thread and this operation can only be done from the main thread 
    184 // (and we don't want to waste space in every wxTextCtrl object for this field 
    185 // unnecessarily). 
    186 int gs_lenOfInsertedText = 0; 
     183// It is used to pass information from DoWriteText() to AdjustSpaceLimit() 
     184// and is global as text can only be inserted into a few text controls at a 
     185// time (but possibly more than into one, if wxEVT_TEXT event handler does 
     186// something that results in another text control update), and we don't want to 
     187// waste space in every wxTextCtrl object for this field unnecessarily. 
     188wxStack<int> gs_lenOfInsertedText; 
    187189 
    188190} // anonymous namespace 
    189191 
    void wxTextCtrl::DoWriteText(const wxString& value, int flags) 
    11541156        // Remember the length of the text we're inserting so that 
    11551157        // AdjustSpaceLimit() could adjust the limit to be big enough for it: 
    11561158        // and also signal us whether it did it by resetting it to 0. 
    1157         gs_lenOfInsertedText = valueDos.length(); 
     1159        gs_lenOfInsertedText.push(valueDos.length()); 
    11581160 
    11591161        ::SendMessage(GetHwnd(), selectionOnly ? EM_REPLACESEL : WM_SETTEXT, 
    11601162                      // EM_REPLACESEL takes 1 to indicate the operation should be redoable 
    11611163                      selectionOnly ? 1 : 0, wxMSW_CONV_LPARAM(valueDos)); 
    11621164 
    1163         if ( !gs_lenOfInsertedText ) 
     1165        const int lenActuallyInserted = gs_lenOfInsertedText.top(); 
     1166        gs_lenOfInsertedText.pop(); 
     1167 
     1168        if ( lenActuallyInserted == -1 ) 
    11641169        { 
    11651170            // Text size limit has been hit and added text has been truncated. 
    11661171            // But the max length has been increased by the EN_MAXTEXT message 
    1167             // handler, which also reset gs_lenOfInsertedText to 0), so we 
    1168             // should be able to set it successfully now if we try again. 
     1172            // handler, which also reset the top of the lengths stack to -1), 
     1173            // so we should be able to set it successfully now if we try again. 
    11691174            if ( selectionOnly ) 
    11701175                Undo(); 
    11711176 
    11721177            ::SendMessage(GetHwnd(), selectionOnly ? EM_REPLACESEL : WM_SETTEXT, 
    11731178                          selectionOnly ? 1 : 0, wxMSW_CONV_LPARAM(valueDos)); 
    11741179        } 
    1175         else 
    1176         { 
    1177             // EN_MAXTEXT handler wasn't called, so reset the flag ourselves. 
    1178             gs_lenOfInsertedText = 0; 
    1179         } 
    11801180 
    11811181        if ( !ucf.GotUpdate() && (flags & SetValue_SendEvent) ) 
    11821182        { 
    bool wxTextCtrl::AdjustSpaceLimit() 
    21592159        // We need to increase the size of the buffer and to avoid increasing 
    21602160        // it too many times make sure that we make it at least big enough to 
    21612161        // fit all the text we are currently inserting into the control. 
    2162         unsigned long increaseBy = gs_lenOfInsertedText; 
     2162        unsigned long increaseBy = gs_lenOfInsertedText.top(); 
    21632163 
    2164         // Don't let it affect any future unrelated calls to this function. 
    2165         gs_lenOfInsertedText = 0; 
     2164        // Indicate to the caller that we increased the limit. 
     2165        gs_lenOfInsertedText.top() = -1; 
    21662166 
    21672167        // But also increase it by at least 32KB chunks -- again, to avoid 
    21682168        // doing it too often -- and round it up to 32KB in any case. 

TIA!

comment:17 Changed 7 months ago by awi

This should also work, but since gs_lenOfInsertedText is a global variable I would consider replacing these statements:

unsigned long increaseBy = gs_lenOfInsertedText.top();  
        // Indicate to the caller that we increased the limit.  
        gs_lenOfInsertedText.top() = -1;  

with atomic operation like this:

unsigned long increaseBy = ::InterlockedExchange(&gs_lenOfInsertedText.top(), -1);

comment:18 Changed 7 months ago by awi

Also these statement seem no to be perfectly safe in the multithreaded environment (we can pop not "our own" element):

    const int lenActuallyInserted = gs_lenOfInsertedText.top();  
    gs_lenOfInsertedText.pop();

but I have no idea how to refactor them into atomic operation. Atomic function like "get & pop" would be welcome.

comment:19 Changed 7 months ago by vaclavslavik

Vaclav, could you please test this?

Seems to work correctly in my tests, thanks a lot!

comment:20 Changed 7 months ago by vadz

We don't need to care about MT issues here, the GUI controls can only be used by the thread that created them, i.e. the main GUI thread.

comment:21 Changed 7 months ago by VZ

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

(In [76193]) Fix reentrancy issues in wxMSW wxTextCtrl max length handling code.

The changes of r75940 didn't work correctly if the handler of wxEVT_TEXT in
some text control modified a (potentially) different text control, as the same
global variable was reused with disastrous results. Avoid this by keeping a
stack of insertion lengths instead.

Using a per-control field would work too, but would be a bit wasteful as the
size of the stack will rarely be more than 1 (and never much more) and this
change can also be applied to 3.0 branch without breaking ABI.

Additionally, fix another problem in r75940 which used 0 as a special marker
for the insertion length, which result in redoing each insertion of empty
string (which is another word for Remove()) twice unnecessarily, by using -1
instead.

Closes #15980.

comment:22 Changed 7 months ago by VZ

(In [76194]) Fix reentrancy issues in wxMSW wxTextCtrl max length handling code.

The changes of r75936 didn't work correctly if the handler of wxEVT_TEXT in
some text control modified a (potentially) different text control, as the same
global variable was reused with disastrous results. Avoid this by keeping a
stack of insertion lengths instead.

Additionally, fix another problem in r75936 which used 0 as a special marker
for the insertion length, which result in redoing each insertion of empty
string (which is another word for Remove()) twice unnecessarily, by using -1
instead.

Closes #15980.

Changed 7 months ago by awi

Patch fixing the issue with loading the text.

comment:23 Changed 7 months ago by awi

  • Resolution fixed deleted
  • Status changed from closed to reopened

Recent changes introduced one small issue which can be observed when the text is loaded into the wxTextCtrl from the file (of size > 30000 B).
Test case:

  • samples/minimal/minimal.cpp

    old new  
    172172    CreateStatusBar(2); 
    173173    SetStatusText("Welcome to wxWidgets!"); 
    174174#endif // wxUSE_STATUSBAR 
     175    wxTextCtrl *tc = new wxTextCtrl(this, wxID_ANY, wxEmptyString, wxPoint(5,5), wxSize(200, 100)); 
     176    tc->LoadFile(wxT("test.txt")); 
    175177} 

In this case application fails with this assertion:

wxWidgets-work\include\wx/vector.h: assert "idx < m_size" failed in wxVector<int>::at().

This happens because inside wxTextCtrl::DoLoadFile() there is called directly wxTextCtrl::AdjustSpaceLimit() and inside this function it is attempted to access uninitialized variable gs_lenOfInsertedText.
According to the current design, AdjustSpaceLimit() shouldn't be called from the outside of EN_MAXTEXT handler but fortunately there is no real need to do so because space adjustment is now done automatically on writing the text into the control.

Patch fixing this issue is attached (attachment:ticket:15980:textctrl-load-file.patch).

comment:24 Changed 6 months ago by VZ

  • Owner set to VZ
  • Resolution set to fixed
  • Status changed from reopened to closed

In 76402:

Don't call AdjustSpaceLimit() explicitly in wxMSW wxTextCtrl any more.

There is no need to do it as this is done by DoWriteText() and
AdjustSpaceLimit() doesn't work correctly if called from outside of it now.
Because of this, also make it private to prevent other accidental calls to it.

Closes #15980.

comment:25 Changed 6 months ago by VZ

In 76403:

Make wxMSW wxTextCtrl::AdjustSpaceLimit() safe to call in all cases.

Allow calling this function not only from inside DoWriteText(): first, because
the existing code could be doing this (although this is only a concern in 3.0
branch as it was made private in the trunk) and second because it could
actually happen if the text limit was exceeded by user typing in the control.

See #15980.

comment:26 Changed 6 months ago by VZ

In 76412:

Don't call AdjustSpaceLimit() explicitly in wxMSW wxTextCtrl any more.

There is no need to do it as this is done by DoWriteText() and
AdjustSpaceLimit() doesn't work correctly if called from outside of it now.
Because of this, also make it private to prevent other accidental calls to it.

Closes #15980.

comment:27 Changed 6 months ago by VZ

In 76413:

Make wxMSW wxTextCtrl::AdjustSpaceLimit() safe to call in all cases.

Allow calling this function not only from inside DoWriteText(): first, because
the existing code could be doing this (although this is only a concern in 3.0
branch as it was made private in the trunk) and second because it could
actually happen if the text limit was exceeded by user typing in the control.

See #15980.

Note: See TracTickets for help on using tickets.