Ngx-scanner: Undefined error in prepareCaptureCanvas method, if scanner is closed within scanDelay time after scanning a QR code successfully

[xtj7]

Bug, feature request, or proposal:

What am I trying to do: I open an overlay to scan the QR code and when it was scanned successfully, I want to immediately close it. If I do that through the scanSuccess callback, it will throw an error that "this.imageElement.naturalWidth" is undefined

What is the expected behavior?

After a successful scan, I want to be able to immediately stop scanning and/or hide the entire scanner without any errors being thrown.

What is the current behavior?

After a successful scan, if I stop scanning immediately, it will throw an error "Cannot read property 'naturalWidth' of undefined" in the prepareCaptureCanvas method on the imageElement. This is because of

What are the steps to reproduce?

Here you can test both scenarios, once with the delay and once without, to see that the same code works as long as I wait after scanning and it does not, once I immediately stop scanning.
https://zxing-ngx-scanner-n5hoke.stackblitz.io/

Which versions of Angular, ngx-scanner, OS, TypeScript, browsers are affected?

Tested with Angular 4 and Angular 5, ngx-scanner 1.0.5
OS, TS version and browser are no factor.

Is there anything else we should know?

A suggestion on how to "fix" it, is to let it silently fail in the beginning of the decode method, if neither videoElement nor imageElement are found, e.g.:
if (undefined === this.videoElement && undefined === this.imageElement) return;

[odahcam]

Nice find!

I think the proper solution would be to clearTimeout(this.timeoutHandler) on stop, this way the scan will be really stopped.

[xtj7]

Thanks :)
But this is already happening, as far as I see. On change of scannerEnabled, the resetScan method of the scanner component is called, which in turn calls the reset method of the codeReader. The codeReader's reset method calls the stop method, which already clears the timeout of the timeoutHandler, if it is set.

[xtj7]

Actually after a little investigation, it seems that the zone task is still running after the timeout was already cleared. This may be a bit trickier than it initially seemed.
scheduleTask in zone is called, clearTask is not.

I was not able to find much information on that topic and I don't have in-depth zone knowledge. So I can't really say why clearTask is not called.

We could check if the timeoutHandler is null in decode, and if it is, stop the operation. But that would couple the decode method to the timeout directly, which does not seem very clean.

My proposed PR is probably the cleanest workaround for this particular problem, but it is still a workaround. I would of course prefer a solution :)

[odahcam]

Maybe we should open a issue on their repository and provide a demo for they to investigate with us. 🤔
Do you think it really could be a issue with zone?

[xtj7]

I don't think so. I suspect it could be related to the setTimeout function here: https://github.com/zxing-js/ngx-scanner/blob/f289b4f815bdc5ef002c550ee7d2c471290aac6a/src/app/modules/zxing-scanner/browser-code-reader.ts#L192

As this one does not set its return value to the timeoutHandler, so it may continue to run anyways, even if timeoutHandler is cancelled. Maybe the fix would be to just replace this with this.decodeWithDelay(callbackFn) instead.

Since basically that is just doubling the timeout and seems strange to begin with.

[xtj7]

Yes, I could confirm that is the issue. It work as intended if replaced as per my proposal. I have updated my PR to reflect this. Using that patch I can no longer reproduce the error.
Maybe you can check if this could have any other side effects and was an intentional deviation, but I could not see any.

[odahcam]

Wow! This looks soo wrong. 😨

@werthdavid can ya take a look at this? I think the only "why" this timeout is there could be to wait for the decode to end it's first attempt.

Even if it should be there, I can refactor the two functions to work nicely together. I will to that in the future anyway. 😅

[xtj7]

That is possible, I have not tested it across many devices but in my initial tests that did not seem to be an issue. With that fix I could start/stop it at any time, scan multiple times and it would not cause any issues 😄

Probably this would be an edge case where you try to stop the scanner manually while the decoding is in progress at the exact same time. Then it may not stop but continue, as decodeWithDelay is called after that.
Instead of adding another delay on top of it, I would rather have an attribute "isEnabled" on the browser-code-reader that will be toggled as well by startDecodeFromStream/stop methods accordingly. Then you can check this in the decodeWithDelay method and just abort there.

Also I appreciate your change. I was wondering already what the point of that wrapper method was, when you call the reader in a single place only anyways 👍

[odahcam]

First, thanks. I think that delay was not necessary, because the reader.decode seems to be a sync function, so it already have to wait for that to end. 🤔

[xtj7]

Once the scanner started, it will always be triggered through the timeout, so while the executed code within the decode method itself is synchronous, the entire decode method runs asynchronous in relation to the stop method. So it is still susceptible to race conditions.

If you run stop while decode has started, it will try to remove the timeout stored in the timeoutHandler (which has expired by that point), decode does not care about that and will - once it is done with the execution - call decodeWithDelay again and the scanner will keep on running in the background. The component gets removed and we get the same issue that we have right now. That is of course limited to this particular edge case, where the stop method needs to be called after decode was called, but before decode calls the decodeWithDelay method. Otherwise it works as intended.

It is purely theoretical at this point though. I was not able to force it to behave like that and writing a test for it may prove to be very tricky.

[werthdavid]

Sorry guys, little time atm..
I don't exactly remember what I thought when I initially implemented the timeout but if I remember correctly my intention was to prevent the scanner to pass a result to the output multiple times after an successful scan e.g. the user points the camera to the code for 1s and the result is outputted 3 times (that was the case with my Android App that uses ZXing Java).

[odahcam]

Too much performatic, let's make it slower. 😄

That's ok David, I think we can deal with that now, also by improving the method implementation we can do some new tricks, like some 1d code reading. 🙂

I've merged #72 to develop just now. Thanks @xtj7.

[xtj7]

Thanks a lot guys! And also: great work on the ngx-scanner :)

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.