search

mozilla / pdf.js

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

Fix integration test "Stamp Editor Copy/paste from a tab to an other" #18318

Closed timvandermeij closed 1 week ago

timvandermeij commented 1 week ago

The integration test runs currently contain the following traceback in the logs:

Suite error: Stamp Editor Copy/paste from a tab to an other
  Message:
    ProtocolError: Runtime.callFunctionOn timed out. Increase the 'protocolTimeout' setting in launch/connect calls for a higher timeout if needed.
  Stack:
        at <instance_members_initializer> (file:///home/ubuntu/pdfjs/botio-files-pdfjs/private/ae7a2979558207a/node_modules/puppeteer-core/lib/esm/puppeteer/common/CallbackRegistry.js:89:14)
        at new Callback (file:///home/ubuntu/pdfjs/botio-files-pdfjs/private/ae7a2979558207a/node_modules/puppeteer-core/lib/esm/puppeteer/common/CallbackRegistry.js:93:16)
        at CallbackRegistry.create (file:///home/ubuntu/pdfjs/botio-files-pdfjs/private/ae7a2979558207a/node_modules/puppeteer-core/lib/esm/puppeteer/common/CallbackRegistry.js:19:26)
        at Connection._rawSend (file:///home/ubuntu/pdfjs/botio-files-pdfjs/private/ae7a2979558207a/node_modules/puppeteer-core/lib/esm/puppeteer/cdp/Connection.js:86:26)
        at CdpCDPSession.send (file:///home/ubuntu/pdfjs/botio-files-pdfjs/private/ae7a2979558207a/node_modules/puppeteer-core/lib/esm/puppeteer/cdp/CDPSession.js:63:33)
        at #evaluate (file:///home/ubuntu/pdfjs/botio-files-pdfjs/private/ae7a2979558207a/node_modules/puppeteer-core/lib/esm/puppeteer/cdp/ExecutionContext.js:353:50)
        at ExecutionContext.evaluate (file:///home/ubuntu/pdfjs/botio-files-pdfjs/private/ae7a2979558207a/node_modules/puppeteer-core/lib/esm/puppeteer/cdp/ExecutionContext.js:268:36)
        at IsolatedWorld.evaluate (file:///home/ubuntu/pdfjs/botio-files-pdfjs/private/ae7a2979558207a/node_modules/puppeteer-core/lib/esm/puppeteer/cdp/IsolatedWorld.js:96:30)
        at CdpFrame.evaluate (file:///home/ubuntu/pdfjs/botio-files-pdfjs/private/ae7a2979558207a/node_modules/puppeteer-core/lib/esm/puppeteer/api/Frame.js:335:43)
        at CdpFrame.<anonymous> (file:///home/ubuntu/pdfjs/botio-files-pdfjs/private/ae7a2979558207a/node_modules/puppeteer-core/lib/esm/puppeteer/util/decorators.js:94:27)

I have bisected this to #18313, and only now noticed that this was actually already present in the integration test run logs at http://54.241.84.105:8877/ae7a2979558207a/output.txt and http://54.193.163.58:8877/5ae7c79b8bea53b/output.txt in that PR. However, sadly this didn't actually make the integration tests fail, and this bug is filed separately in #18319.

The problem is that this test opens two pages, whereas the other tests always open just one. The test copies the image from page 1 to page 2 and then stops. This means that at the end of the test page 2 has focus, but in the afterAll handler we close page 1 before page 2, in other words: we attempt to close the page first that doesn't have focus.

Before this PR that wouldn't have been an issue, but now that the L10n logic waits for requestAnimationFrame that hangs page 1's tab because, until there is actually focus, no new request animation frame will come. This in turn causes the test to hang in the afterAll handler until the timeout happens, or if the user manually clicks the first tab in the browser.

One could argue that this is an issue in the PR, but on the other hand it was done like that for good reason and I also don't really see a better way. It seems easier to just change the test, since there is only one that actually suffers from this, to close the pages in reverse order so that we always close the tab with focus first, after which the other tab will automatically get focus and close too.

Snuffleupagus commented 1 week ago

Hopefully Fluent itself can be updated to address what at least I consider a bug, as discussed in https://github.com/mozilla/pdf.js/pull/18313#discussion_r1649732857, by changing https://github.com/projectfluent/fluent.js/blob/fee2242049010f314345a7c3b7b91600df3a05fe/fluent-dom/src/dom_localization.js#L171-L172 into e.g.

this.windowElement?.cancelAnimationFrame(this.pendingrAF);
this.windowElement = null;
this.pendingrAF = null;

since that'd allow us to remove our requestAnimationFrame when destroying the l10n-class.

timvandermeij commented 1 week ago

Hopefully Fluent itself can be updated to address what at least I consider a bug

Yes, I agree. For now we can work around this (fortunately in an easy manner; see the PR above) until Fluent implements this functionality, but it's indeed extra reason to implement this upstream in Fluent and not the projects using it.