Repaint lockup in ctlSQLBox::OnPositionStc on windows (Was: SQL-panel causes 100% CPU and lockup on windows) - Mailing list pgadmin-hackers

Hi all,

ok, it is broken since this commit:

http://git.postgresql.org/gitweb/?p=pgadmin3.git;a=patch;h=b0ecbbca7f77c0f07cff67bba3d2bca28954a1e2

-   EVT_STC_UPDATEUI(-1,  ctlSQLBox::OnPositionStc)
+   EVT_STC_PAINTED(-1,  ctlSQLBox::OnPositionStc)

It appears that in previous version of scintilla, there existed a
SCN_POSCHANGED event, which then got obsolete and replaced by
SCN_UPDATEUI, and probably that is why the name 'OnPositionStc' was
chosen for the handler. So linking OnPositionStc() to STC_UPDATEUI was
correct and in accordance with scintilla's manual.
However the change from EVT_STC_UPDATEUI to EVT_STC_PAINTED was
incorrect. These events are not quite equivalent. EVT_STC_PAINTED has a
somewhat different purpose.

I've checked, reverting back to EVT_STC_UPDATEUI indeed fixed CPU load
and lockup issues on windows. See also
http://www.scintilla.org/ScintillaDoc.html#SCN_PAINTED
http://www.scintilla.org/ScintillaDoc.html#SCN_UPDATEUI
http://www.scintilla.org/ScintillaHistory.html
http://web.mit.edu/jhawk/mnt/spo/sandbox/punya/anjuta-1.2.3/doc/ScintillaDoc.html
  (-- this is some older document with a more detailed explanation of
SCN_UPDATEUI)

Some relevant excerptions:

"SCN_PAINTED: is to update some _other_ widgets based on a change."

"SCN_POSCHANGED: notification is deprecated as it was causing confusion.
Use SCN_UPDATEUI instead."

"SCN_UPDATEUI, SCN_CHECKBRACE:
Either the text or styling [...] common use is to check whether the
caret is next to a brace and set highlights on this brace and its
corresponding matching brace."

So for me this all sounds clear sufficiently. I'll stick with
EVT_STC_UPDATEUI and I can rebuild it myself if I have to. Whether fix
git respectively or not is up to maintainers now.


Thank you,
Nikolai

03.10.2015 21:56, I wrote:
> Hi all,
>
> Ok, I've verified that ctlSQLBox::OnPositionStc() is broken.
>
> If I comment the code for highlighting in it (that is, almost all of its
> body), the problem goes away.
>
> I've found quite some problems observed with it and attempted to fix
> previously, like e.g. the one from 2011:
> ---------------
> + // Ensure we don't recurse through any paint handlers
> + Freeze();
> UpdateLineNumber();
> + Thaw();
>
> // Clear all highlighting
> ---------------
> That was most probably only a partial fix, and it was subsequently
> reverted.
>
> I'd like to also note that 'OnPositionStc' name is pretty much
> misleading. If it was named 'OnPaintedStc' it would be more consistent
> and more hinting where the problem is. I'm aware that renaming alone
> don't usually fix bugs, but it might help humans better catch ones.
>
> Ok, anyway, I've got no real fix for now, but I'm going on.
>
>
> Thank you,
> Nikolai
>
>
> 29.09.2015 21:40, I wrote:
>> Hi,
>> 29.09.2015 1:17, I wrote:
>> [...]
>>> Here is how to reproduce. (Procedure valid at least for win xp and 7)
>>> In under 1 minute!
>>
>> Some more observations. Same problem happens also in function properties
>> window, in the 'Code' tab:
>>
>> - choose some simple (lets say empty) function, open its properties;
>> - click on the 'Code' tab;
>> - place this properties window so that its righthand margin is somewhat
>> beyond the right margin of the monitor, making some part of SQL editor
>> invisible (out of the real screen space);
>> - click to place cursor into the function body and type 'select now()';
>> - watch the CPU load.
>>
>> So it looks like ctlSQLBox instances are affected, although ctlSQLBox
>> itself seems quite harmless (looking at the code), except for maybe
>> ctlSQLBox::OnPositionStc() about which I've just no idea. And since
>> ctlSQLBox is a direct descendant of a wx class, it looks like maybe it
>> is actually wxwin to blame.
>> So my theory is:
>> -- either ctlSQLBox::OnPositionStc() itself,
>> -- or the underlying wx class (classes).
>>
>> I haven't arranged a development environment yet, but am going to.
>>
>>
>> Thank you,
>> Nikolai
>>
>>> - Aero have to be disabled (on versions that have it). If you can not or
>>> do not want to disable aero for some reason, you might try connecting
>>> via remote desktop instead, it should basically behave very similar,
>>> I've tested it as well.
>>> - start windows task manager and minimize it so it just shows CPU load
>>> within a tray icon. (taskmgr.exe)
>>> - start pgamin as usual, connect to some server, then click on some
>>> database, so that SQL-panel button becomes available.
>>> - open SQL-panel (click the above mentioned button), then move SQL-panel
>>> window in the righthand direction till some part of SQL-editor window
>>> rectangle goes beyond the right border of the screen. Let is stay like
>>> that (partly invisible). Note that I mean exactly SQL-editor window
>>> itself, not additional notepad window that might happen to be located to
>>> the right of SQL-editor. (Or you can just close this notepad to be sure)
>>> - now click into SQL-editor so that you can start typing.
>>> - type just exactly this: 'select now()' then stop, do not touch
>>> anything and watch the CPU load. it should go 100% (50% on dual core,
>>> 25% on quad core) and stick there.
>>> - note: it is critical to put brackets there, otherwise bug is not
>>> triggered, so I'd guess this is related to code highlighting.
>>> - now go to SQL-panel's main menu - select file - select exit. instead
>>> of closing, SQL-panel goes completely unresponsive here.
>>>
>>> I'd be glad to hear some feedback (from windows users) :)
>>>
>>>
>>> Thank you,
>>> Nikolai
>>>
>>>>
>>>>
>>>> Thank you,
>>>> Nikolai
>>>
>>>
>>>
>>
>>
>>
>
>
>