mozilla / pdf.js

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

Fix `dumpio: true` logs about `#createBundleFallback` #18264

Closed timvandermeij closed 2 weeks ago

timvandermeij commented 2 weeks 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 before we enable that further due to the spammy nature of said logs.

This issue tracks the following often repeated log from http://54.241.84.105:8877/34fcd127eefeaf9/output.txt:

JavaScript error: http://127.0.0.1:41223/build/generic/web/viewer.mjs, line 3023: Error: Not implemented: #createBundleFallback

I have tracked the problem down to https://github.com/mozilla/pdf.js/blob/06800cd966111f7262d3334dfb68c3765bc92740/web/genericl10n.js#L79 being triggered, which in turn triggers https://github.com/mozilla/pdf.js/blob/06800cd966111f7262d3334dfb68c3765bc92740/web/genericl10n.js#L110-L112 and because we're in testing mode the exception is thrown.

Both code paths were introduced in https://github.com/mozilla/pdf.js/commit/97c2ce9da02dfc3ee3a563342e48540c07c18a6f but unfortunately I don't know enough about the L10n bundle handling to be able to tell for sure what the issue is. However, in debugging this I found that we in fact have a valid bundle for en-us just before at https://github.com/mozilla/pdf.js/blob/06800cd966111f7262d3334dfb68c3765bc92740/web/genericl10n.js#L76 which made me wonder why we also try to create a fallback bundle for this language, and thought: are we perhaps missing a continue after the yield here? If I look at the situation before the patch, see https://github.com/mozilla/pdf.js/commit/97c2ce9da02dfc3ee3a563342e48540c07c18a6f#diff-28e788fc43e67c197d60ffb3054ffb599f083341963c64d156e7a87a0ced98ceR78, we would always continue implicitly after the yield, but now it basically falls through to the fallback bundle creation.

This might very well be intended though, and this is the part I don't know. I'm also not sure why there is a dedicated testing path in this code, because if we fall through we'll always get here in testing mode too. @Snuffleupagus Do you perhaps have ideas for this, since you probably know this code a lot better than I do?