search

mozilla / pdf.js

PDF Reader in JavaScript
https://mozilla.github.io/pdf.js/
Apache License 2.0
48.14k stars 9.95k forks source link

[Meta] Fix log issues from the `dumpio: true` Puppeteer option #18281

Open timvandermeij opened 3 months ago

timvandermeij commented 3 months ago

In PR #18260 we enabled dumpio: true for Puppeteer. This option forwards the browser logs to Node's log stream so that we can see any warnings/errors that are logged in e.g. the web console. This surfaced a few interesting issues that we should fix, not only to reduce the amount of logs but mainly because they might point at actual issues in our code.

This issue is a meta issue for this effort. We should go over the logs and file individual issues for each interesting issues. The logs from the last run in the PR are:

This meta issue already contains following issues:

timvandermeij commented 2 months ago

Having looked through the logs of the last few weeks, which are fortunately pretty clean after all the fixed above, I think we have two main categories of tracebacks left:

  1. dispatchEventInSandbox-related logs such as in http://54.241.84.105:8877/c51fe1b82f42d3b/output.txt. In #18293 this was attempted to be solved, but it looks like the problem lies elsewhere.
  2. bindListeners-related logs such as in http://54.193.163.58:8877/5f4c377dce34650/output.txt. This one seems to be because in at least the regular and secondary toolbar we only register event listeners but don't unregister them, and that should hopefully be easy to fix with e.g. abort controllers.
Snuffleupagus commented 2 months ago

2. bindListeners-related logs such as in http://54.193.163.58:8877/5f4c377dce34650/output.txt. This one seems to be because in at least the regular and secondary toolbar we only register event listeners but don't unregister them, and that should hopefully be easy to fix with e.g. abort controllers.

What's triggering those event listeners though, since the Toolbar/SecondaryToolbar have no async code and the errors appear to suggest that the tests somehow click on toolbar-buttons after we've invoked testingClose in the viewer?

timvandermeij commented 2 months ago

Good question, and I'm not really sure. However, I just found http://54.193.163.58:8877/7a0c5f232507dff/output.txt and http://54.193.163.58:8877/c0e29236d621ca3/output.txt which both also have the bindListener traceback at the same position in the logs, so that might be an indication that the must check the keyboard event is limited to the input integration test does something to trigger it (it interacts with the page number field in the toolbar).

calixteman commented 2 months ago

We're deleting the page number here: https://github.com/mozilla/pdf.js/blob/145951df88570909c9e92481e04f712c7d65b144/test/integration/freetext_editor_spec.mjs#L2922-L2931 and I guess the callback defined here: https://github.com/mozilla/pdf.js/blob/145951df88570909c9e92481e04f712c7d65b144/web/toolbar.js#L212-L217 is triggered just before we close. So I think we should add the abort controller stuff in the toolbar to remove every listener we've here.

timvandermeij commented 2 months ago

It looks like there's a third one in addition to the two from https://github.com/mozilla/pdf.js/issues/18281#issuecomment-2211394260. For example in http://54.241.84.105:8877/00b7f8245c22a11/output.txt the #colorSelect one appears; this one happens less often but I have seen it a couple of times now. I don't recall having seen any other types of tracebacks, so for now I think it's limited to these three.