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

fix: wait for fonts before pdf printing #12175

Merged
merged 1 commit into from
Apr 2, 2024
Merged

fix: wait for fonts before pdf printing #12175

merged 1 commit into from
Apr 2, 2024

Conversation

OrKoN
Copy link
Collaborator

@OrKoN OrKoN commented Mar 29, 2024

This PR adds a command to always wait for the web fonts readiness before generating a PDF.

Closes #3183

@OrKoN OrKoN marked this pull request as ready for review April 2, 2024 08:26
@OrKoN OrKoN enabled auto-merge (squash) April 2, 2024 08:46
@OrKoN OrKoN merged commit 59bffce into main Apr 2, 2024
38 checks passed
@OrKoN OrKoN deleted the orkon/wait-for-fonts branch April 2, 2024 08:52
@release-please release-please bot mentioned this pull request Apr 2, 2024
@whimboo
Copy link
Collaborator

whimboo commented Apr 5, 2024

Hi @OrKoN. I wonder if we should actually get this added to the WebDriver specifications. It sounds like an important thing to do.

@OrKoN
Copy link
Collaborator Author

OrKoN commented Apr 5, 2024

@whimboo perhaps but it is easy to implement on the client side. I decided to enable this by default to learn if there are use cases where waiting for fonts is not desired. It could inform our decision as to if this needs to be optional.

@whimboo
Copy link
Collaborator

whimboo commented Apr 5, 2024

Sounds good. In case it would make things more stable please inform us or as usual file an issue for BiDi yourself. Thanks!

@mariusrak
Copy link

Hi, could you write what is the command exactly, Thanks

@gnuletik
Copy link

gnuletik commented Jun 9, 2024

This change introduced a bug for our use-case. We had intermittent requests that failed with timeout.
I tracked it down to the puppeteer@22.6.3 version (no issue on v22.6.2).

It seems that when reusing pages (with multiple page.goto calls), the document.fonts.ready promise is not always resolved, when interacting with multiple pages concurrently.

While investigating, I tried to debug without headless mode and I realized that the browser would only resolve the document.fonts.ready promise when opening the page in the UI by clicking on the tab in the menu.

My workaround is to close the page after each PDF generation instead of re-using pages.
However, this led to a performance decrease from 10 req/s to 5 req/s.

I'm not sure if this is a bug on Chrome side or on Puppeteer side.
Any ideas where should I open an issue?

Do you think that waiting for document.fonts.ready should be disabled by default and become an option?

@OrKoN
Copy link
Collaborator Author

OrKoN commented Jun 9, 2024

@gnuletik could you provide a minimal executable example that demonstrates the problem? Generally, the browser resolves documents.fonts.ready in the headless mode too and it should not require closing/re-opening the page. It is possible though that if multiple tabs are open, the browser might throttle inactive tabs (Puppeteer disables some throttling so it perhaps you are launching the browser with different args?), so it is recommended to call page.bringToFront() like the user would. In any case, it's best if you could provide a minimal script that demonstrates the problem to make sure the exact cause can be found.

@gnuletik
Copy link

gnuletik commented Jun 9, 2024

@OrKoN Thanks for the feedback!
Here's a reproduction case: https://github.com/gnuletik/puppeteer-repro-12175/blob/main/index.js

You can run it with npm install && npm run start.
As you can see, the tabs get "stuck" (waiting on document.fonts.ready) while trying to generate the PDF with multiple concurrent tabs.

$ npm run start
Server running on port 3000
Generating PDF
Generating PDF
Generating PDF
Generating PDF
Generating PDF
PDF generated
file://project/node_modules/puppeteer-core/lib/esm/puppeteer/common/util.js:250
            throw new TimeoutError(`Timed out after waiting ${ms}ms`, { cause });
                  ^

TimeoutError: Timed out after waiting 30000ms
    at file://project/node_modules/puppeteer-core/lib/esm/puppeteer/common/util.js:250:19
    at file://project/node_modules/puppeteer-core/lib/esm/third_party/rxjs/rxjs.js:1936:31
    at OperatorSubscriber2._this._next (file://project/node_modules/puppeteer-core/lib/esm/third_party/rxjs/rxjs.js:993:9)
    at Subscriber2.next (file://project/node_modules/puppeteer-core/lib/esm/third_party/rxjs/rxjs.js:696:12)
    at AsyncAction2.<anonymous> (file://project/node_modules/puppeteer-core/lib/esm/third_party/rxjs/rxjs.js:2280:20)
    at AsyncAction2._execute (file://project/node_modules/puppeteer-core/lib/esm/third_party/rxjs/rxjs.js:1360:12)
    at AsyncAction2.execute (file://project/node_modules/puppeteer-core/lib/esm/third_party/rxjs/rxjs.js:1349:22)
    at AsyncScheduler2.flush (file://project/node_modules/puppeteer-core/lib/esm/third_party/rxjs/rxjs.js:1427:26)
    at listOnTimeout (node:internal/timers:573:17)
    at process.processTimers (node:internal/timers:514:7) {
  [cause]: undefined
}

Node.js v22.2.0

You can switch the lines in comment in order to close / reopen the tabs, which makes the script complete:
https://github.com/gnuletik/puppeteer-repro-12175/blob/345e54eb3bce764392d64a7ede687da015a19074/index.js#L30-L33

While building it, I realized that the issue only occurs when rendering a page with:

  • custom fonts
  • an SVG file used as a background in CSS

as you can see here: https://github.com/gnuletik/puppeteer-repro-12175/blob/main/public/index.html

I also gave [page.bringToFront()](https://pptr.dev/api/puppeteer.page.bringtofront/) a try:

  • Using it after the goTo doesn't fix it
  • When calling it after a timeout, it seems to reduce the number of occurrences of the issue but when increasing the number of concurrent calls, the issue remains.

Thanks!

@OrKoN
Copy link
Collaborator Author

OrKoN commented Jun 10, 2024

@gnuletik thanks, I see what is happening, so indeed it looks like tabs are being throttled because all PDFs are being requested at the same time and only one page can active at the same time. Would you mind filing a new issue for this? I also wonder if the fonts are actually loaded in the previous versions of Puppeteer? i.e., is it only the fonts.ready timing out or if the fonts are altogether not loading.

@OrKoN
Copy link
Collaborator Author

OrKoN commented Jun 10, 2024

A few workarounds:

  1. Use headless: 'shell' for printing. This mode does not behave as the regular Chrome so throttling there should not happen.
  2. Use a lock to bring a page to front before capturing the PDF.

We could roll back the behavior of waiting for fonts or make it optional but I think the issue is probably that with this usage pattern some PDFs would not have the fonts loaded (although the timeouts would go away).

@OrKoN
Copy link
Collaborator Author

OrKoN commented Jun 10, 2024

Having a mutex would make sure the page is active when PDF is being generated:

import Mutex from 'p-mutex';
const activePageMutex = new Mutex();

async function run () {
  const page = await pagePool.acquire()
  try {
    await activePageMutex.withLock(async () => {
      await page.bringToFront();
      await generatePDF(page)
    });
  } finally {
    // Release the page back to the pool leads to `document.fonts.ready` blocking
    await pagePool.release(page)
    // Destroying the page is a workaround
    // await pagePool.destroy(page)
  }
}

Obviously, this cannot run in parallel. So the other workaround for using headless shell would be recommended to achieve parallelism.

@OrKoN
Copy link
Collaborator Author

OrKoN commented Jun 10, 2024

Shorter example to reproduce:

import express from 'express'
import puppeteer from 'puppeteer'

const app = express()
app.use(express.static('public'))
const server = app.listen(3000, () => console.log('Server running on port 3000'))

const browser = await puppeteer.launch()
const page = await browser.newPage();
await browser.newPage();
await page.goto("http://localhost:3000/");
console.log('Generating PDF')
const pdf = await page.pdf()
console.log('PDF generated')
await browser.close()
server.close()

@OrKoN
Copy link
Collaborator Author

OrKoN commented Jun 10, 2024

@gnuletik interesting is that I am not able to reproduce on Linux but on Mac it happens consistently.

Edit: the full example also reproduces on Linux.

@gnuletik
Copy link

Thanks for digging into the issue @OrKoN!

Yes the full example happens consistently on Linux on my side.
Having a mutex is a workaround but that would be a huge performance decrease from the previous version.

I did not see a visual issue with the previous version, but that's hard to prove that it can never happen.

I'd be happy to create an issue but the Issue template is expecting a single JS file without external dependencies. So I guess that if I don't respect it, the issue will get closed.

@OrKoN
Copy link
Collaborator Author

OrKoN commented Jun 10, 2024

@OrKoN I have digged a bit deeper in the code and basically the ready promise is resolved when layout happens in Chrome and since the page is inactive there is no layout/animation frames etc. I am not sure yet how it is all connected with the svg document. I am not sure if we can go to the previous behavior since other users have the issue of fonts not being loaded for PDF and at the same time I am not sure if it is possible to change the browser behavior (although fonts.ready seems to be under-specified w3c/csswg-drafts#4248). I think the recommendation would be to activate the page before interacting with it (like a user would do) or use the headless: 'shell' where replicating the behavior of the browser w.r.t to the activation/focus/page visibility is not desired.

@gnuletik
Copy link

Cool finding for the ready promise trigger condition!

I think the recommendation would be to activate the page before interacting with it (like a user would do)

Do you mean using page.bringToFront()?

headless: shell can be a workaround too but I faced other issues while using it at some point.

@OrKoN
Copy link
Collaborator Author

OrKoN commented Jun 10, 2024

yes, page.brintToFront()

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

Successfully merging this pull request may close these issues.

Custom Fonts not loaded when pdf generated
5 participants