T4 - crash when skipping forward 'blank' minutes

[jacampbell]

T4 with beyonwiz-16.1-beyonwizt4-20170310.

Playing a recording,
press and hold 'fast forward', Skip forward (min) pop-up appears.
pressed the '< PREV' button (instead of 7) and the number of minutes field went blank,
then pressed OK -
then a crash.

Crash report attached.

[MrQuade]

I can reproduce this crash.

Looks like it should be a trivial fix.

FYI, have you explored all of the skip functions in the Wiz interface? You can save yourself a lot of keypresses by using the skip keys rather than typing in the number of minutes to skip.

The arrow buttons are all mapped to different skip durations by default, and you can optionally change the keypad keys such that they also perform skips of different values.....all with one keystroke.

[prl]

I can reproduce this crash.

Looks like it should be a trivial fix.

I can reproduce it, too.

The problem is that the number field is empty when OK is pressed, and that empty string is passed to a function to convert a string to an integer.

There are a few possible trivial fixes. Some that come to mind are that if OK is pressed when the number field is blank, then: OK is ignored, OK does the same action as EXIT or OK does the same action as the default skip time. I'm sure that inventive minds can think of others. What fix do you want?

[MrQuade]

Not that I was asked (but when has that ever stopped me ). But I would vote on OK executing the EXIT function. Reason being, the field is blank, and zero minutes are implied by a blank value, so it should execute a zero minute skip.

[Grumpy_Geoff]

Not that I was asked (but when has that ever stopped me ). But I would vote on OK executing the EXIT function. Reason being, the field is blank, and zero minutes are implied by a blank value, so it should execute a zero minute skip.

Gets my vote too.

[prl]

Bug #604: Crash in "Skip forward/back (min)"

[prl]

Not that I was asked (but when has that ever stopped me ). ...

The "you" was the inclusive one, not just the OP

[prl]

... But I would vote on OK executing the EXIT function. ...

I'll go with that, then, unless there are loud objections. I'll also submit the fix (a one-liner) upstream to OpenViX.

[IanSav]

Hi,

I assume that the code will not be changed to execute the EXIT function but will simply set the jump value to be 0, if it is an empty string, and then effectively make this a no-op.

Regards,
Ian.

[prl]

...
I assume that the code will not be changed to execute the EXIT function but will simply set the jump value to be 0, if it is an empty string, and then effectively make this a no-op.
...

It will have exactly the same effect as the EXIT function, but not by calling MinuteInput.cancel(). Doing it that way would require at least three more lines of code.

My fix is:

Code: Select all

diff --git a/lib/python/Screens/MinuteInput.py b/lib/python/Screens/MinuteInput.py
index 163c257..23f8ae2 100644
--- a/lib/python/Screens/MinuteInput.py
+++ b/lib/python/Screens/MinuteInput.py
@@ -63,7 +63,7 @@ class MinuteInput(Screen):
                self["minutes"].down()
 
        def ok(self):
-               self.close(int(self["minutes"].getText()))
+               self.close(int(self["minutes"].getText() or 0))
 
        def cancel(self):
                self.close(0)

[peteru]

Treating the result as 0 is less than ideal, since it causes a seek. On some media a seek of 0 can cause a fairly large skip to the nearest "access point". An invalid value or a value of zero should really be a no-op.

The one liner fix might catch a blank entry, which is a fix for this particular bug, however, that's just one symptom of insufficient input validation and lack of strong typing. For example, if a user enters another invalid value, such as 99999999, the code will still blow up.

[prl]

Treating the result as 0 is less than ideal, since it causes a seek. On some media a seek of 0 can cause a fairly large skip to the nearest "access point". An invalid value or a value of zero should really be a no-op.

Agreed, but that error lies in InfoBarGenerics.InfoBarSeek.fwdSeekTo()/rwdSeekTo().

The one liner fix might catch a blank entry, which is a fix for this particular bug, however, that's just one symptom of insufficient input validation and lack of strong typing. For example, if a user enters another invalid value, such as 99999999, the code will still blow up.

Agreed. MinuteInput should at least limit the width of its Component.Input component.

[prl]

.., if a user enters another invalid value, such as 99999999, the code will still blow up.

The actual limit where it blows up is ~17000 times larger, 1708031858677.

However, it would make sense to limit the skip to no more than 4-5 digits, since the maximum recording length is 24 hours or 1440 min.

[Grumpy_Geoff]

... 12 hours or 1440 min.

12 hours is 1,440 minutes

[prl]

... 12 hours or 1440 min.

12 hours is 1,440 minutes

No, 24 hours is. Fixed. Thanks.

[peteru]

Don't forget that the skip function is available for media from various sources, not just recordings. It would be uncommon to have a file that spans more than 24 hours, but it's not impossible.

[Grumpy_Geoff]

Don't forget that the skip function is available for media from various sources, not just recordings. It would be uncommon to have a file that spans more than 24 hours, but it's not impossible.

Let 'em eat cake!
They can skip again if they need to jump more than 1440 minutes

[prl]

Don't forget that the skip function is available for media from various sources, not just recordings. It would be uncommon to have a file that spans more than 24 hours, but it's not impossible.

Thats why I said 4-5 digits. 4 digits is nearly 7 days, and 5 digits is nearly 70 days.

[jacampbell]

Instead of making some guess about what to do, surely the correct response would be to display an error message "Non numeric minute value" or "Excessive minute value - maximum is 9999 minutes" or <whatever>.

[prl]

I'd rather fix the bugs in fixed-width numeric input in Components.Input (and it seems to be quite flaky). Fixing the width would make the width restriction feedback direct without error popups.

[prl]

Bug #604: Crash in "Skip forward/back (min)" on empty skip field
Bug #605: Illegal number format entry allowed in Components.Input
Bug #606: Skip forward/back (min) skips even if cancelled or 0 min
Bug #607: Crash in "Skip forward/back (min)" on very large skips