[$250] Web - Attachment - "Worker was terminated" console error when closing a PDF

[IuliiaHerets]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 9.0.23-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4880362&group_by=cases:section_id&group_id=292107&group_order=asc
Issue reported by:

Action Performed:

1. Navigate to https://staging.new.expensify.com/

2. Log in with a new Gmail account

3. Navigate to FAB - Start chat

4. Start a 1:1 DM with any account

5. Upload a small PDF (included)

6. Upload a large PDF (included)

7. Open the large PDF

8. Close the large PDF

9. Open the small PDF

10. Close the small PDF

Expected Result:

I should be able to close the PDF without a console error.

Actual Result:

"Worker was terminated" console error when closing a PDF after opening and closing a different, larger PDF. Sometimes I get the console error when closing an image attachment too.

Workaround:

Unknown

Platforms:

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6578472_1724261778978.bandicam_2024-08-21_19-25-00-639.mp4

Bug6578472_1724261778945!Small_PDF.pdf
Bug6578472_1724261778936!Large_PDF.pdf

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a5b10381ea8600ab
• Upwork Job ID: 1827398804771215994
• Last Price Increase: 2024-08-24
• Automatic offers:
• paultsimura | Reviewer | 103709560

Issue Owner

Current Issue Owner: @paultsimura

[melvin-bot]

Triggered auto assignment to @kevinksullivan (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[IuliiaHerets]

@kevinksullivan FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[CyberAndrii]

Proposal

Please re-state the problem that we are trying to solve in this issue.

"Worker was terminated" console error shows up when closing a PDF view while it is still loading.

What is the root cause of that problem?

This error occurs because loadingTask.destroy() is not working correctly, as explained in mozilla/pdf.js#11595.

We previously fixed this issue in #31209, but it has reappeared since. The reason is that the previous fix patches the node_modules/react-pdf/dist/esm/Document.js file, but the app now uses node_modules/react-pdf/dist/cjs/Document.js.

What changes do you think we should make in order to solve the problem?

In addition to the esm/Document.js file, we need to apply the same fix to cjs/Document.js.

Updated react-pdf+7.7.3.patch file will be:

diff --git a/node_modules/react-pdf/dist/cjs/Document.js b/node_modules/react-pdf/dist/cjs/Document.js
index 9bb0398..032d898 100644
--- a/node_modules/react-pdf/dist/cjs/Document.js
+++ b/node_modules/react-pdf/dist/cjs/Document.js
@@ -289,6 +289,7 @@ const Document = (0, react_1.forwardRef)(function Document(_a, ref) {
             pdfDispatch({ type: 'REJECT', error });
         });
         return () => {
+            loadingTask._worker.destroy();
             loadingTask.destroy();
         };
     }
diff --git a/node_modules/react-pdf/dist/esm/Document.js b/node_modules/react-pdf/dist/esm/Document.js
index b1c5a81..569769e 100644
--- a/node_modules/react-pdf/dist/esm/Document.js
+++ b/node_modules/react-pdf/dist/esm/Document.js
@@ -261,6 +261,7 @@ const Document = forwardRef(function Document(_a, ref) {
             pdfDispatch({ type: 'REJECT', error });
         });
         return () => {
+            loadingTask._worker.destroy();
             loadingTask.destroy();
         };
     }

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01a5b10381ea8600ab

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @paultsimura (External)

[paultsimura]

The proposal by @CyberAndrii looks good to me.

🎀👀🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @thienlnam, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[thienlnam]

Ideally we can upgrade this version so we can remove the amount of patches we use. In the previous issue, it looks like we considered that but didn't due to a re-render issue in v7. Has that been solved since then? It's been a year #31209 (comment)

[CyberAndrii]

@thienlnam We're currently using v7.7.3, so most likely we already have a workaround for the re-rendering issue. I agree it would be a good idea to update to the latest version (v9), but it still doesn't have a fix for the 'Worker was terminated' error. I can probably submit a PR for them, and we would be able to upgrade after it's released.

[paultsimura]

@thienlnam bumping the version will not only not solve the issue (as @CyberAndrii already mentioned), but it will require additional patches as mentioned by the package author in the migration guide. Therefore, I think we should go with the suggested patch.

[thienlnam]

Thanks for the context - agree with patching for this issue since it's not resolved in the latest yet.

However, I'm going to create another issue to remove the patches so we reduce the technical debt when we upgrade RN versions - if you'd like to submit an upstream PR for that issue that would be appreciated!

[melvin-bot]

📣 @paultsimura 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @CyberAndrii You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

[thienlnam]

@CyberAndrii Here's the issue if you're interested in doing that #48124

[CyberAndrii]

Thanks! I'll submit a PR for this issue tomorrow and then look into fixing it in the upstream repo.

[paultsimura]

Deployed to prod #48196 (comment)
Payment is due 06.09.2024

[paultsimura]

• The PR that introduced the bug has been identified. Link to the PR:
  It's quite challenging to find which version change led to using cjs/Document.js instead of esm/Document.js, but I would bet on fix: upgrade to newer react-pdf and pdf.js to render high-resolution images properly on the web #33083 when the major version was bumped from 6 to 7.
• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A
• A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
• Determine if we should create a regression test for this bug: Yes
• If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

1. Start a 1:1 DM with any account.
2. Upload 2 PDF files
3. Wait until the files are uploaded
4. Open one PDF attachment
5. Close it while it's still loading
6. Verify that no "Worker was terminated" error is logged to the console
7. Open another PDF file
8. Close it while it's still loading
9. Verify that no "Worker was terminated" error is logged to the console

Do we agree 👍 or 👎

[paultsimura]

FYI @kevinksullivan – the payment is due tomorrow.

[kevinksullivan]

Paid @paultsimura , offer sent to @CyberAndrii . Let me know when you accept!

[CyberAndrii]

@kevinksullivan Accepted the offer

[kevinksullivan]

all set