Fix Chrome data store directory is not cleaned up

[ankur22]

This fix resolves #278 in majority of times the extension is ran. As outlined in the issue itself, the problem is:

1. The cleanup of the temp dir is done too early, but it does work;
2. Another process (chrome ran with exec) creates the temp dir if it's not present -- the cleanup occurs prior to the exec being called.

The fix is:

1. To move the cleanup call from BrowserType to the Browser downstream process and call it from Close.
2. To try to call the cleanupFunc when the context is closed or the browser sub process exits.

There is an edge case that remains which is that the cleanup is not guaranteed because k6 can shut down at any point during runtime, and currently there is no way for k6 to wait for extensions to terminate cleanly. This issue is being tracked in #403.

[inancgumus]

Nice work, I haven't finished my review yet :) I'll do so today.

By the way, you can squash this commit.

[imiric]

Nice work! Initially I liked the approach of doing this in Browser.close(), but after thinking about it some more, I think we need to ensure that the directory is cleaned up regardless if the script calls close() or not. It might be because of a script error or a browser crash, but we should ensure the directory is removed in all of these cases if we created it initially. This is because it could potentially have sensitive information we shouldn't leave lying around, especially when run in the cloud.

The only cases where it should remain is if the k6 process itself crashes, in which case there's nothing we can do outside of an external cleanup process, or if the user sets a new option or env var to keep it.

[ankur22]

Nice work! Initially I liked the approach of doing this in Browser.close(), but after thinking about it some more, I think we need to ensure that the directory is cleaned up regardless if the script calls close() or not. It might be because of a script error or a browser crash, but we should ensure the directory is removed in all of these cases if we created it initially. This is because it could potentially have sensitive information we shouldn't leave lying around, especially when run in the cloud.

The only cases where it should remain is if the k6 process itself crashes, in which case there's nothing we can do outside of an external cleanup process, or if the user sets a new option or env var to keep it.

Thanks for the feedback, I'll take a look 👍

[inancgumus]

Nice! I like to see ditching more bugs 🐞!

A little nitpick: Could you do refactorings in one commit, and fixes in another one? As it was kind of difficult for me to review the PR :) There were others, but, as an example, extracting to createTempDir was refactoring and could be done in its own commit.

[ankur22]

Nice! I like to see ditching more bugs 🐞!

A little nitpick: Could you do refactorings in one commit, and fixes in another one? As it was kind of difficult for me to review the PR :) There were others, but, as an example, extracting to createTempDir was refactoring and could be done in its own commit.

Yep, that makes sense. Please just keep pointing this out when i miss it 😅 I'll learn eventually #BadHabits

[imiric]

Nice job with this! 👏 And sorry again it was more complicated than we originally thought, but hopefully it was a good learning experience for you. 😅

Unfortunately, as you explained in that comment, I don't think we can do better than this until grafana/k6#2432 is implemented. From my local tests it does get rid of the temp directory consistently, even without calling Browser.close(), but of course, we can't guarantee this will happen all the time. I'm fine with having both solutions in place until we can add a single one after 2432 is implemented. And for Cloud tests, we'll still have to add an external cleanup to make sure it's removed even if the process crashes for some reason.

Can you please squash and rebase this on main, and address the minor comments I had? I'll take another look when that's done.