fix(browser-runner): reject promise on error

[andresrondon]

This patch adds a reject callback to the _processClosing promise and executes it if it catches an error on removeFolderAsync(...).

Worth noting that I found this while experiencing this issue: original, fairly-recent duplicate.

This change DOES NOT attempt to fix the actual bug that prevents from removing CrashpadMetrics-active.pma temp file, but it solves the most annoying part which is letting the process hanging. This change should allow the client to catch the error in their own implementation and handle it adequately based on their needs.

[mathiasbynens]

Could you please

• add a test so that we don’t regress this behavior?
• update the documentation

[andresrondon]

@mathiasbynens for sure. I will add that as soon as I have some free time. What part specifically from the doc should I update? Thanks.

[mathiasbynens]

What part specifically from the doc should I update?

docs/api.md. Just a brief note on how you might observe this new behavior as a user would be great 👍

[badluck13]

I would really love to see this in the next version!

[andresrondon]

Sorry for the delay. Been very busy (with a project using puppeteer hah). Will get on this this evening hopefully.

[andresrondon]

Hi folks! Firstly, I added the required documentation in docs/api.md.

On the unit tests, after spending some hours, I think there's no practical way to unit test this without doing some other changes. Here's what happens:

This can only be reproduced if for some reason the temp folder cannot be deleted (for example: a different process is blocking a file in it, the folder no longer exists or insufficient permissions). Knowing this, the only practical way I can think of is changing the value of the _tempDirectory property used in line 99 of BrowserRunner.ts, which I can't do since it's private. I also thought of removing the folder before calling browser.close(), but again, I cannot do that because I can't know the randomly-generated name of the temp folder, and no way to access that private property I mentioned. Finally, I even thought of the following:

      it('should reject promise on error with deleting temp folder', async () => {
        const { puppeteer, defaultBrowserOptions } = getTestState();

        const browser = await puppeteer.launch(defaultBrowserOptions); // the temp folder gets created
        await browser.close(); // the temp folder gets removed and connection is disposed

        let error = undefined;

        try {
          await browser.close(); // will attempt to delete non-existent temp folder
        } catch (err) {
          error = err
        }

        expect(error).toBeDefined();
      });

but this wouldn't work either because the attempt to delete the folder occurs in a once event, meaning that the second time I call browser.close() it won't execute the callback.

If you can think of a way to unit test this change or if you agree to make the value of [browser-runner]._tempDirectory somehow accessible from a browser object, please let me know and I would be happy to make the necessary changes. Thanks.

@mathiasbynens

[alamothe]

I would like to report that, while this fix works (I prefer resolved vs rejected) and doesn't hang the browser, there is still a lot of instability caused by trying to remove the temporary directory. Our production service on Windows crashes ever so often due to that. Right now the way we patched this is to schedule (setTimeout) deleting temp folder after a minute instead of immediately during close. Will report if this fully resolves the problem.