IDLE crash on startup with typo in config-keys.cfg

[GeorgeDhoore]

BPO	11437
Nosy	@terryjreedy, @ned-deily, @serwy, @asvetlov
Files	config-keys.cfg: File that crashes IDLE in interpreter mode issue11437.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/terryjreedy'
closed_at = None
created_at = <Date 2011-03-08.00:13:57.851>
labels = ['3.7', 'expert-IDLE', 'type-bug', 'expert-tkinter']
title = 'IDLE crash on startup with typo in config-keys.cfg'
updated_at = <Date 2017-09-28.06:58:52.142>
user = 'https://bugs.python.org/GeorgeDhoore'

bugs.python.org fields:

activity = <Date 2017-09-28.06:58:52.142>
actor = 'taleinat'
assignee = 'terry.reedy'
closed = False
closed_date = None
closer = None
components = ['IDLE', 'Tkinter']
creation = <Date 2011-03-08.00:13:57.851>
creator = 'George.Dhoore'
dependencies = []
files = ['21039', '24351']
hgrepos = []
issue_num = 11437
keywords = ['patch']
message_count = 18.0
messages = ['130301', '130387', '151993', '151994', '152187', '157095', '157098', '157206', '157208', '157211', '157213', '157215', '220093', '220107', '220391', '226890', '280685', '297253']
nosy_count = 6.0
nosy_names = ['terry.reedy', 'ned.deily', 'roger.serwy', 'asvetlov', 'George.Dhoore', 'Saimadhav.Heblikar']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue11437'
versions = ['Python 3.6', 'Python 3.7']

[GeorgeDhoore]

If the user makes a typo when setting a custom keybind (in this case "<Alt-Key-up>" instead of "<Alt-Key-Up>") IDLE will silently crash. From the command-line the error shows as:

Traceback (most recent call last):
  File "C:\Python32\Lib\idlelib\idle.py", line 11, in <module>
    idlelib.PyShell.main()
  File "C:\Python32\Lib\idlelib\PyShell.py", line 1388, in main
    shell = flist.open_shell()
  File "C:\Python32\Lib\idlelib\PyShell.py", line 277, in open_shell
    self.pyshell = PyShell(self)
  File "C:\Python32\Lib\idlelib\PyShell.py", line 856, in __init__
    self.history = self.History(self.text)
  File "C:\Python32\Lib\idlelib\IdleHistory.py", line 12, in __init__
    text.bind("<<history-previous>>", self.history_prev)
  File "C:\Python32\Lib\idlelib\MultiCall.py", line 332, in bind
    self.__binders[triplet[1]].bind(triplet, func)
  File "C:\Python32\Lib\idlelib\MultiCall.py", line 213, in bind
    seq, handler)))
  File "C:\Python32\Lib\tkinter\__init__.py", line 977, in bind
    return self._bind(('bind', self._w), sequence, func, add)
  File "C:\Python32\Lib\tkinter\__init__.py", line 932, in _bind
    self.tk.call(what + (sequence, cmd))
_tkinter.TclError: bad event type or keysym "up"

This is doubly confusing as case sensitivity seems to be applied at random in config-keys.cfg.

Expected behavior:
Ideally case sensitivity shouldn't matter in config-keys.cfg and if there is an error in the config syntax, that particular line should be ignored and the rest of the file tried. If IDLE is still able to start, some user friendly error should be displayed indicating the problem line.

[terryjreedy]

I presume 'silently' means when starting with an icon or shortcut, so that there is no window left to contain the traceback.

The standard key customization is by clicking, which makes errors impossible. How did you get the error? By using the "Advanced Key Binding Entry" button, which warns "These bindings will not be checked for validity!"? Or by directly editing the a file in its undocumented location (which is unknown to me)? [Question: why is either alternative
needed?]

Case sensitivity is a feature of string comparison. In this case, it is done by tcl and beyond our control. Adding validity checking where now disclaimed would be a possible feature request.

Assuming the binding is done within a loop, it would be reasonable to wrap the failing call with try...except TclError: display message "When binding key '...' to action '...', tcl reports error <emsg>".

Can you submit a patch?

[terryjreedy]

bpo-13864 is a duplicate, where I mentioned the remedy of deleting the bad file.

This is similar to bpo-5707, but I am not sure if exactly the same.
The patch there fixed one problem but not the bad key binding problem.

[terryjreedy]

bpo-13071 is another duplicate closed in favor of this.

[serwy]

Attached is a patch against 3.3a0 to validate the active key set in the configuration handler. It validates each key binding and then presents a dialog with a list of incorrect keys if there are any.

In developing this patch I found that IDLE calls GetCurrentKeySet multiple times during initialization of an EditorWindow or a PyShell, once for each extension loaded. This inefficiency leads to long start times for IDLE.

[asvetlov]

Roger, can you help me?

What steps should I do to reproduce the issue? Pushing config from George.Dhoore into ~/.idlerc does nothing. IDLE starts fine as usual.

[serwy]

Andrew, after placing config-keys.cfg into .idlerc, launch IDLE and change the key map to Xip's keyset. You'll get a traceback on the terminal when you click ok.

[asvetlov]

I reproduced the bug.

Roger Serwy, your patch looks working, but can you describe:

1. Why do you need self.known_invalid list?
2. Why do you always extend that list (known_invalid variable is
   just shared alias for self.known_invalid member, not list copy).

I'm a bit confusing with that logic.

[serwy]

Without the known_invalid list, each call to GetCurrentKeySet would cause an error message to appear. The effect of keeping track of these bad key bindings is that the user receives a notification only once about a particular bad key binding. (The present architecture in IDLE calls GetCurrentKeySet too many times during initialization.)

The known_invalid list gets extended with any newly found invalid key bindings. Also, these newly found invalid keys generates an error message. This can occur again if the user modifies the key map with the configuration dialog.

Perhaps using a set instead of a list would make the code clearer.

[asvetlov]

Ok

[serwy]

I just realized that if the user, for whatever reason, fixes a bad key binding and then rebinds it to the previously bad one, no error message would appear. This can occur if the user is switching between key sets where one key set contains invalid bindings.

[asvetlov]

You right. That was my concern.

[BreamoreBoy]

@Roger, Andrew could you pick this up again?

[terryjreedy]

Mark, they are both inactive at the moment.

There are a number of issue around configuration, especially key bindings. I think the whole process needs review and redoing. For instance, I understand Roger as saying that the problematical known-invalid list (set) is needed because GetCurrentKeySet is called repeatedly, once for each extension. I want to look into fixing the repeated calls, which will increase with more extensions, and possibly eliminate the need for known-invalid.

Another redesign: I think Control-Key-x should automatically mean Control-Key-X, and ditto for Alt-Key-x, and that we should not need both in config-keys even if tk requires separate bind calls.

Saimadhav, we should try to do some validation of the key bindings in the unittest for configurations. The pattern seems to be
optional Control-, Alt-, or Meta-
optional Shift-
required Key-
required ascii key, bracketleft/right, or Uppercase-key-name,

[taleinat]

I don't think the patch should currently be committed.

I agree with Terry: we should first fix the issue whereby the key config is read repeatedly. Given such a fix, the problematic "known_invalid" workaround in the patch would no longer be necessary.

As a side note, I think the method used in the patch to check if a binding is good. Ideally it would be the final check done after a simpler syntax check, since a syntax check could give more informative error messages.

Also, there are two other more technical issues with the patch; I mentioned them in the patch review system.

[terryjreedy]

https://stackoverflow.com/questions/25750477/python-2-7-8-idle-will-not-open-on-mavericks -- because one of the two files has 'shift' instead of Shift. When I make a change in either file (with custom keys being used), the traceback ends slightly differently until the final message.

File "C:\Programs\Python34\lib\idlelib\EditorWindow.py", line 1100, in apply_bindings
text.event_add(event, *keylist)
File "C:\Programs\Python34\lib\idlelib\MultiCall.py", line 379, in event_add
widget.event_add(self, virtual, seq)
File "C:\Programs\Python34\lib\tkinter\init.py", line 1441, in event_add
self.tk.call(args)

bpo-21519 is about checking bindings entered in the dialog with either the basic or advanced methods. Bindings also need to be checked when read so we know which file is bad. Exception tracebacks should be replaced by more helpful messages that include fix-up instructions and which do not disappear into the void when running on pythonw.

[terryjreedy]

I closed bpo-25662 in favor of this.

[terryjreedy]

Tal's comments from Rietveld review (see msg220391);

529: Does this have to be a separate, new Tk instance? Wouldn't a temporary widget under the current Tk be just as good? Creating a new Tk instance could take a relatively large amount of time time and perhaps also have visual side-effects.

me> better to use original if possible.

545: Here keylist is mutated inside the for loop which iterates over it. This looks like a bug, and in any case should be avoided.

me> looping over invalid. But this is part of repeated check which we should avoid.