Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Meta] Fix intermittent issues in the integration tests #18396

Open
timvandermeij opened this issue Jul 5, 2024 · 4 comments
Open

[Meta] Fix intermittent issues in the integration tests #18396

timvandermeij opened this issue Jul 5, 2024 · 4 comments

Comments

@timvandermeij
Copy link
Contributor

timvandermeij commented Jul 5, 2024

Despite having fixed the most occurring intermittent failures in the integration tests (see the closed issues with the intermittent label) we unfortunately still have a few of them left that trigger from time to time. We should list and fix them. This issue is a meta issue for this effort.

The following intermittents have been found on the bots as of July 4th, 2024, which is after landing all previously identified improvements:

Name OS Logs Fixed?
FreeText Editor Update a freetext and scroll must check that a freetext is still there after having updated it and scroll the doc (assertion failure) Windows http://54.193.163.58:8877/f1e92ac29cc3a65/output.txt -
FreeText Editor Update a freetext and scroll must check that a freetext is still there after having updated it and scroll the doc (timeout) Windows http://54.193.163.58:8877/481c711563dfae6/output.txt #18418
Interaction in autoprint.pdf must check if printing is triggered when the document is open Windows http://54.193.163.58:8877/b48df120bbc7f5c/output.txt #18617
Interaction in bug1782564.pdf must check that charLimit is correctly set Windows http://54.193.163.58:8877/a58317de0521842/output.txt #18399
Highlight Editor Highlight and caret browsing must check that the caret can move a highlighted text (and others tracked in #18187 due to the additional context) Windows http://54.193.163.58:8877/60a667d8d66f476/output.txt #18574
Highlight Editor Highlight with the keyboard must check that some text has been highlighted Linux http://54.241.84.105:8877/8891dbe9fe88686/output.txt -
FreeText Editor FreeText on several pages must check that first annotation is selected without errors Windows http://54.193.163.58:8877/2336ef073f32ba6/output.txt -
Text layer Text selection using mouse when selecting over a link allows clicking the link after selecting Windows http://54.193.163.58:8877/08a1619a072581d/output.txt #18555
Stamp Editor New alt-text flow must check the new alt text flow (part 1) Windows http://54.193.163.58:8877/10dc1652d064ee2/output.txt -
Stamp Editor New alt-text flow must check the new alt text flow (part 2) Windows http://54.193.163.58:8877/bdd22b640958b26/output.txt -
Interaction in issue15092.pdf must check that a values is correctly updated on a field and its siblings Linux / Windows http://54.241.84.105:8877/2277869b1c75180/output.txt / http://54.193.163.58:8877/4ced5711a1d31ee/output.txt #18624
Stamp Editor Basic operations must load a SVG, delete it and undo Windows http://54.193.163.58:8877/0cbe9c22f3b1f86/output.txt #19069
Interaction in issue14862.pdf must convert input to uppercase Linux http://54.241.84.105:8877/30bf36234d8301b/output.txt #19001
@timvandermeij
Copy link
Contributor Author

timvandermeij commented Jul 5, 2024

Note to self, the following is useful for reproducing scripting-related intermittents locally:

diff --git a/web/pdf_scripting_manager.js b/web/pdf_scripting_manager.js
index 63d08c604..68d1b8a21 100644
--- a/web/pdf_scripting_manager.js
+++ b/web/pdf_scripting_manager.js
@@ -177,6 +177,7 @@ class PDFScriptingManager {
         return; // The document was closed while the properties resolved.
       }
 
+      await new Promise(r => setTimeout(r, 2000));
       await this.#scripting.createSandbox({
         objects,
         calculationOrder,
diff --git a/src/pdf.sandbox.external.js b/src/pdf.sandbox.external.js
index 365d94a60..af646c29f 100644
--- a/src/pdf.sandbox.external.js
+++ b/src/pdf.sandbox.external.js
@@ -67,7 +67,7 @@ export class SandboxSupportBase {
     }
     try {
       args = this.exportValueToSandbox(args);
-      this.commFun(name, args);
+      this.win.setTimeout(this.commFun.bind(this, name, args), 1000);
     } catch (e) {
       this.win.console.error(e);  
  1. Run the tests in a random order.

@timvandermeij
Copy link
Contributor Author

timvandermeij commented Jul 9, 2024

For the FreeText Editor Update a freetext and scroll must check that a freetext is still there after having updated it and scroll the doc failure we collected interesting findings in #18415. Note in particular #18415 (comment) and the failed run at http://54.193.163.58:8877/0507814a8df56c1/output.txt where the base64-encoded PNGs are correct. If we comment out https://github.com/mozilla/pdf.js/blob/master/test/integration/freetext_editor_spec.mjs#L3647 we can reproduce the empty PNG issue locally.

timvandermeij added a commit to timvandermeij/pdf.js that referenced this issue Aug 4, 2024
…tests

The `waitForClick` helper function is functionality-wise mostly a
reduced copy of the more generic `waitForEvent` helper function that
we use for other integration tests, so we can safely replace it to
reduce the amount of code.

Moreover, the `waitForClick` code is prone to intermittent failures
given recent assertion failures we have seen on the bots (one of them
is linked in mozilla#18396) while `waitForEvent` has recently been fixed to
avoid such intermittent failures, so this should also remove the
flakiness for these integration tests.
timvandermeij added a commit to timvandermeij/pdf.js that referenced this issue Aug 4, 2024
…tests

The `waitForClick` helper function is functionality-wise mostly a
reduced copy of the more generic `waitForEvent` helper function that
we use for other integration tests, so we can safely replace it to
reduce the amount of code.

Moreover, the `waitForClick` code is prone to intermittent failures
given recent assertion failures we have seen on the bots (one of them
is linked in mozilla#18396) while `waitForEvent` has recently been fixed to
avoid intermittent failures, so usiong it should also get rid of the
flakiness for these integration tests.
@timvandermeij
Copy link
Contributor Author

timvandermeij commented Nov 3, 2024

I have debugged the must load a SVG, delete it and undo integration test. It's reproducible almost 100% of the time on Linux too if we update Puppeteer to version 23.4.0+ (see #18773), but it only fails in Chrome. I found the following:

  1. The timeout occurs at https://github.com/mozilla/pdf.js/blob/master/test/integration/stamp_editor_spec.mjs#L206.
  2. Just before that the deleted annotation is undone, causing a rebuild of the editor.
  3. During editor rebuild the waitForSerialized helper function gets the serializable annotation storage at https://github.com/mozilla/pdf.js/blob/master/test/integration/test_utils.mjs#L329.
  4. This immediately calls serialize on all available editors at https://github.com/mozilla/pdf.js/blob/master/src/display/annotation_storage.js#L190-L193.
  5. The editor's serialize() method calls #serializeBitmap, but apparently too soon because in the browser console we find TypeError: Cannot read properties of null (reading 'width'):
pdf.mjs:19593 Uncaught (in promise) TypeError: Cannot read properties of null (reading 'width')
    at #serializeBitmap (pdf.mjs:19593:54)
    at StampEditor.serialize (pdf.mjs:19738:48)
    at get serializable (pdf.mjs:4859:64)

My suspicion is that the fact that this throws during serialization breaks the annotation storage's map and causes the next waitForSerialized call to hang forever.

However, I don't know enough about the stamp editor implementation to pinpoint exactly what's wrong. It looks like a race condition where the stamp editor may not expect its serialize() method to be called during rebuilds, because I noticed that if I introduce a call to waitForTimeout just after the kbUndo call that the test passes. One idea I had was that this.#bitmapPromise may be related to this because we don't really seem to await it in the code; if something else calls serialize() in the middle of the promise execution I'm not sure if the code was made with that in mind.

@calixteman Does this ring a bell for you, since you're much more familiar with the stamp editor than I am?

@calixteman
Copy link
Contributor

Could you check if waiting for the canvas being visible is enough ?
It could be something like:

await page.waitForSelector(`${editorSelector} canvas`, { visible: true });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants