NetSurf Illegal Window Handle

On closing windows in NetSurf choices/configuration I’m seeing the error, ‘An unexpected error occurred: Illegal window handle’. It only happens on alternate config categories and on first look doesn’t appear to do harm.

It’s on today’s (19-12-19) ROM. I don’t know if it’s happened previously – only just noticed. Thought to report here first in case it’s to do with recent changes but if not I know it’ll need to go to the NetSurf list.

I see the same with both NetSurf 3.9 and #4953, but only with today’s (19-12-19) ROM, yesterday’s ROM is OK. NetSurf’s log is of interest.

(9.520000) frontends/riscos/wimp_event.c:1654 ro_gui_wimp_event_get_window: Creating structure for window 0x20481e39
(10.850000) frontends/riscos/dialog.c:378 ro_gui_dialog_close: xwimp_set_caret_position: 0x288: Illegal window handle
(10.850000) frontends/riscos/gui.c:2112 ro_warn_user: WimpError Illegal window handle
(10.850000) frontends/riscos/wimp_event.c:1112 ro_gui_wimp_event_close_window: Close event received for window 0x20481e39

Clipboard caret/task fixes

Hi folks. I can see the error here too.

It’s showing as last night’s build actually reports errors properly – some were lost in previous versions. My logging suggests NetSurf seems to be calling Wimp_SetCaretPosition with a window handle of 0 when a dialogue is closed. The new Wimp is rejecting this as an Illegal window handle (which it is).

Comparison with the previous non-clipboard Wimp though shows that illegal window handles are just ignored and the caret is removed; calls to set the caret in a closed window are faulted though. The official docs say that the window handle should be -1 to remove the caret, but it looks like this might not be happening in the real world. Other apps may possibly hit this too, perhaps.

This is trivial to fix :) , but it depends on what the overall opinion is as to how strict the adherence to the documentation should be. I’ll have a chat with the other developers to see what the preferred outcome is.

Thanks for your patience

Andy

but it depends on what the overall opinion is as to how strict the adherence to the documentation should be.

I think this falls in the same classification as the zero page accesses and our (in)famous discussions on whether it was right to enforce things.

Essentially NetSurf (and possibly other apps) have been doing something wrong but the fault didn’t produce an obvious effect. Now we have visible evidence of the fault and people are questioning whether to hide the evidence or push to get the fault (in NetSurf in this instance) fixed.
Given that there could be knock on effects of the fault in NetSurf I’d say fix NetSurf and leave the OS able to report the fault.

Please don’t break everything again, there’s so few apps left as it is.

1) Fix the behaviour of Wimp_SetCaretPosition to match the last 30 years of compatibility
2) Update the documentation to match reality

Hmmm.

Fix Change the behaviour of Wimp_SetCaretPosition to match the last 30 years of compatibility error invisibility.

I think that fixed it.

Like I said before I think NetSurf was always doing it wrong, it’s just that the system didn’t display the error. Now it does.
When it produces the error message as Fred said above it “doesn’t appear to do harm.” – which is probably why no one noticed it through other means previously.

So should it be fed back to the NetSurf bods and if so would someone more technical than I care to do it to ensure accuracy?

Standard advice – if you spot a bug always report it to the author(s) which as much information as you can gather on the OS version in use, the application version and the method of reproducing the error and any error logs.

Sometimes apparently small errors are having bigger consequences elsewhere.
The fact that this NetSurf error seems relatively benign is a fortunate circumstance.

Edited seriously back spelling/typos…

if you spot a bug always report it to the author(s)

Yes, I get that. I just thought it might be more efficient if reported by someone who understands the detail (I don’t). Otherwise I’ll do it.

Cancelling NetSurf’s toolbar “Scale view” and “Find text’” windows are more good ways of getting the “Illegal window handle” errors.

I have built a NetSurf which which simply traps error 288. It logs it but does not display the desktop error window.

djp@ubuntu1804:~/dev-netsurf/workspace/netsurf/frontends/riscos$ diff dialog-orig.c dialog.c
375c375,377
< 	ro_warn_user("WimpError", error->errmess);
---
> 	if (error->errnum != 0x288) {
>               ro_warn_user("WimpError", error->errmess);
>               }             

To see the context the full file is here

Not a ‘proper’ fix but does simulate pre-CnP-ness, but it will do for now.

Fix the behaviour of Wimp_SetCaretPosition to match the last 30 years of compatibility

Sigh.

The fact that this NetSurf error seems relatively benighn is afortunate circumstance.

This. I’ve just had a look at the NetSurf source for closing dialogues. It looks more like a bug that happens to be benign (for now), rather than a mis-understanding of how the Wimp expects window handles to be formed.

Not a ‘proper’ fix but does simulate pre-CnP-ness, but it will do for now.

Maybe fix it properly? Untested, but I think the issue is that ro_gui_dialog_close (elsewhere in dialogue.c) does

persistent_dialog[i].parent = NULL

before later trying to do

xwimp_set_caret_position(persistent_dialog[i].parent, ...

with the expected outcome. Ideally, one would want to take a copy of that parent handle, then NULL it, then use the copy for any subsequent repatriation of the caret. The side effect is that the caret seems unlikely to actually be put back where it came from by the current code – changes to the Wimp or not.

It’s been a while since I’ve had the tools to build NetSurf, however.

Sent report to NetSurf list

Not a ‘proper’ fix but does simulate pre-CnP-ness, but it will do for now.

Maybe fix it properly?

Any real depth in C eludes me, but yes ‘properly’ would be nice. The catch is that there is no RISC OS developer at the moment to do a proper job for us.

1) Fix the behaviour of Wimp_SetCaretPosition to match the last 30 years of compatibility

I would agree with this if multiple programs were involved.
So far it seems to be only NetSurf and it looks like a use-after-null bug.

Rationale: software crashing today that worked yesterday…not good. At least NetSurf is still being maintained, not all software is.

2) Update the documentation to match reality

Never. If this was to happen, then 0 should be silently accepted as -1 for the purposes of keeping bugged software operating.
But 0 is not valid, and should never be implied to be valid. Don’t document it, that gives it legitimacy.

However, as it appears thus far to be only one program with an identifed cause, leave the OS and fix the browser. :-)

If this was to happen, then 0 should be silently accepted as -1 for the purposes of keeping bugged software operating.
But 0 is not valid, and should never be implied to be valid. Don’t document it, that gives it legitimacy.

Just out of interest, if the documentation was updated to reflect the actual behaviour prior to the latest update (ie. 0 and -1 mean the same in this context), what would actually be broken by it? If 0 was treated the same as -1, would some other software cease to function due to other side effects of the 0?
I can see the case for “the source code is the documentation, anything else stated elsewhere is irrelevant”, and also “let’s not break existing software for the sake of ‘purity’”. I’m not taking any particular side on this (and for the umpteenth time, this is posted in my own name, not that of my employer before anyone jumps in). If NetSurf is definitely the only affected software, and can be updated to the new behaviour by some competent developer, great, but also, if it’s going to affect other, no-longer developed software, as Peter suggests it will reduce the pool of available RISC OS software. Swings and roundabouts, maybe not everything is absolutely black or white?

let’s not break existing software for the sake of ‘purity’”.

Lets be clear about this, before (last x years) the error could occur invisibly with unknown consequences.
Now it’s visible.

Nothing has been broken by the change, but a coding error in NetSurf has been revealed.

If NetSurf isn’t fixed to not produce the visible error then people can continue to use it and just wonder why no one fixed the visible, but possibly benign, error.

If you remove a panel in your downstairs toilet and see you have a water pipe leaking behind the panel that drips into a soak away under the floor, do you fix the pipe or put the panel back to hide the leak and pretend it isn’t leaking?

If you remove a panel in your downstairs toilet and see you have a water pipe leaking behind the panel that drips into a soak away under the floor, do you fix the pipe or put the panel back to hide the leak and pretend it isn’t leaking?

This is what I was asking. If using 0 instead of -1 definitely has negative effects (eg. a leaking pipe), obviously it needs fixing. Now you’ve confirmed that it definitely causes other clear problems in the OS I can see of course you fix it.

I’ve reported the NetSurf issue as Mantis bug 2728.

Any real depth in C eludes me, but yes ‘properly’ would be nice. The catch is that there is no RISC OS developer at the moment to do a proper job for us.

The fix doesn’t seem much more complex than your hack to another part of the same file, TBH.

I may see if I can do something, but after the NetSurf toolchains changed, I never managed to get it all to co-exist nicely with my own cross-compiler setup and I don’t fancy committing an untested change to the code.

If using 0 instead of -1 definitely has negative effects (eg. a leaking pipe), obviously it needs fixing. Now you’ve confirmed that it definitely causes other clear problems in the OS I can see of course you fix it.

It’s not in the OS. What this has shown up is that NetSurf contains a chunk of code to return the caret to where it came from when a dialogue is closed, which simply doesn’t work because it loses the return window handle before trying to call Wimp_SetCaretPosition. That’s a bug in NetSurf, which probably needs fixing (or the chunk of code removing).

ETA. For the avoidance of any doubt, the whole point here is that from the code, NetSurf is not trying to pass -1, or even 0, as a window handle. The code only makes sense if the intent was to pass a valid window handle to Wimp_SetCaretPosition. The handle of the window which had contained the caret prior to the dialogue opening, in fact, so that the caret would go back when the dialogue closes.

It falls over because, before using the variable in the call to Wimp_SetCaretPosition, NetSurf sets its value to NULL – as part of the process of tidying up and recording that the dialogue no longer has a parent window.

I’ve reported the NetSurf issue as Mantis bug 2728

Gwych!

I’ve reported the NetSurf issue as Mantis bug 2728.

It looks as if John-Mark Bell has committed a fix to the code. Thanks to the NetSurf team for another prompt response!

Thanks to the NetSurf team for another prompt response!

I think there’s also a vote of thanks due to the various people who investigated the NetSurf code to find the bug.
Nice one guys.

OK, all you people who saw the problem: has the new CI build of NetSurf fixed it?

Yes, it has. Yes, it has.