Do not run cleanup while printing is ongoing. #5001
Conversation
|
@yurydelendik @Snuffleupagus as discussed in irc. |
| PDFView.idleTimeout = setTimeout(function () { | ||
| PDFView.cleanup(); | ||
| }, CLEANUP_TIMEOUT); | ||
| }, | ||
|
|
||
| cleanup: function pdfViewCleanup() { | ||
| PDFView.idleTimeout = null; |
There was a problem hiding this comment.
Remove this line, otherwise timer will not be canceled by renderHighestPriority()
|
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/8eab0ecb7bc84ca/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/8eab0ecb7bc84ca/output.txt Total script time: 0.72 mins Published
|
| @@ -1570,6 +1576,8 @@ var PDFView = { | |||
| for (i = 0, ii = this.pages.length; i < ii; ++i) { | |||
| this.pages[i].beforePrint(); | |||
| } | |||
| this.printing = true; | |||
| this.renderHighestPriority(); | |||
There was a problem hiding this comment.
Should these lines perhaps be placed before the loop, to make absolutely sure that the cleanup doesn't run before we have called beforePrint for all pages?
There was a problem hiding this comment.
I do not know, if you think thats better I can move it, then I will also move the code in after print down to have symmetry.
There was a problem hiding this comment.
The reason that I asked is that I'm not sure exactly how the event loop in JavaScript handles this case. I'm wondering if it's possible that a previously set idleTimeout could be allowed to run before the entire loop has completed (thus causing issues)?
@yurydelendik Is this something that we should be concerned about here?
There was a problem hiding this comment.
well widening the lock should not be a problem. if we have any doubt we should do it. ill update the pr
Do not run cleanup while printing is ongoing.
|
Looks good. Thank you for looking into that. I think it will fix some of the printing bugs at bugzilla as well |
During investigation of #4994 it became clear that one possible problem with fonts in print is that while printing is ongoing a font is removed and reloaded. If cleanup was set to a low number the problem disappeared.
the cleanup was scheduled when no page or thumbnail was rendered. Because printing uses a different mechanism, t could happen that printing occurs at the same time as cleanup. This is consistent with the observation that the error was reproducible after a few tries (most likely just hitting the 30 second mark).
The solution is to not run cleanup while printing is ongoing. Therefor a flag is set and renderHighestPriority() invoked before and after printing.