react-pdf
react-pdf copied to clipboard
Infinite loop hanging megathread (minPresenceAhead, margin, break)
Description
- There has to be a general flaw in the layout logic resulting in infinite loops that crash browser engines (and whatever the issue might be it is evident that crashes are the worst expression of failure handling)
- (maybe related or not) minPresenceAhead works on the simplest examples but fails to work for me in any real-life scenarios where nested Views (eg inevitable for rendering bullet lists) and page padding (necessary for my case) are used
Note that I use 3.3.8 which afaik should already include https://github.com/diegomura/react-pdf/pull/2400
Preliminary work
Regarding 1)
- https://github.com/diegomura/react-pdf/issues/1384 is related, simply nothing really happened since 2021
- https://github.com/diegomura/react-pdf/issues/2083 was closed (because OP gone rogue) but seems to be related
- https://github.com/diegomura/react-pdf/issues/2303 was closed but seems to be related
- https://github.com/diegomura/react-pdf/issues/2242 might be related
- https://github.com/diegomura/react-pdf/issues/874 might have been related
Regarding 2)
- https://github.com/diegomura/react-pdf/issues/2595 absolutely is related and showcases it pretty well
- https://github.com/diegomura/react-pdf/issues/955 seems it doesn't work for a lot of folks for a very long time
Reproducable cases
(note I'm using @jeetiss REPL for hanging issues since you obviously can't share an url of the official REPL if the process hangs)
1A) Simplest reproducible case REPL: (yes, this is still a thing, nobody confirmed it's fix really)
ReactPDF.render(<Document><Page><Text style={{ paddingBottom: 1000 }}>Text</Text></Page></Document>);
1B) Reducing available space to make it not fit REPL
1D) In my production app I also have a case (it's a larger document and more complex case that I can't 1:1 easily copy over to a REPL) where marking one of the <View>s with break={true} also suddenly introduces the infinite loop hanging. Changing any wrap or minPresenceAhead props don't affect the outcome, yet changing some margins does (although I was not able to find a real pattern yet unfortunately). So I think it's not the implementation of wrap/minPresenceAhead per se but likely a more general flaw in the layout (splitting?) logic. I'll try to spend some more time somehow getting this into a REPL. My gut says that some certain composition of Views with certain paddings/margins (and/or page padding) is getting too large for the Page and then in the splitting logic it somehow falls into the infinite loop hole.
2A) A more contrived Real-life scenario REPL with the minPresenceAhead issue I removed any fancy logic, but kept a lot of styling on purpose so there's something more elaborate to test again when we're confirming a fix.
Thoughts
Interestingly in my hundreds (?) of manual tests I never saw this: https://github.com/diegomura/react-pdf/blob/fab09cc9814326fdb44d2bcb7097ba9960d441d1/packages/layout/src/steps/resolvePagination.js#L44
@carlobeltrame Does https://github.com/diegomura/react-pdf/blob/fab09cc9814326fdb44d2bcb7097ba9960d441d1/packages/layout/src/node/shouldBreak.js#L29 work recursively in a sense that the size and margin/paddings of child Texts of Views of Views are taken into accout? Also – is page size and padding taken into account for the bounding box of the flow? I am asking because of this comment: https://github.com/diegomura/react-pdf/blob/fab09cc9814326fdb44d2bcb7097ba9960d441d1/packages/layout/src/node/shouldBreak.js#L38
What I mean is, if the page has paddingBottom (green) and minPresenceAhead is within the canvas but inside the padding then maybe the following siblings are not taken into account correctly(?):
I understand that this GH issue might actually be multiple issues at once, but I have the feeling that there must be some general connection.
Desktop (please complete the following information):
- OS: MacOS
- Browser: nodejs (electron main thread, but happens in chrome REPL as well)
- React-pdf version 3.3.8
So regarding my main issue 1D) I've dug around for 6 hours without a definite conclusion.
Total weirdness ensures:
- I've patched
asyncComposeto log the name of the async fn that will be executed to find out where it's stuck looping and it consistently printsresolveAssetsas the last line, from which one would like to believe it's stuck there and never reachesresolveSvg(the next step in the chain) - here's where it gets spicy: commenting out the
fontFamilydeclaration on the (very often used) paragraph<View>makes the document render, even though it loads https://github.com/diegomura/react-pdf/blob/fab09cc9814326fdb44d2bcb7097ba9960d441d1/packages/layout/src/steps/resolveAssets.js#L28 the same amount font files (2 font files) and without issues (the fontfiles are fine, the fontkit font loading is fine, the buffers are complete and everything, at the end of the day this works for all other 99% of documents so why would it suddenly not work) - now if you thought I was onto something – hell no: keeping the
fontFamilybut instead commenting out a marginBottom (yes, a margin) of the bullet list wrapper<View>(oddly specific) makes the document render - same goes for setting all
breakproperties to false, renders flawlessly
It really looks like a certain vertical flow position of a certain View or certain Text results in the infinite loop and the margin and different font size or breaking just contribute to that awkward spot.
I'm inclined my logging strategy doesn't work as a marginBottom can't have an effect on resolveAssets and instead it breaks somewhere else but the logs don't reach stdout due to infinite loops preventing the next tick that would print the log. If anyone has an idea how I could go about finding where the infinite loop occurs I'd be grateful.
I'm seeing roughly the same issue as well. We're rendering a list of entry components (elements in a medical record). Certain combinations will cause ReactPDF to go into an infinite loop in the resolvePagination step. In a flame graph, relayout ends up being the function called over and over.
For one of our problem documents:
- Shuffling the the entries fixes the infinite loop
- Removing all bottom margins fixes it
- Removing the
if (shouldBreak) {}block fromsplitNodesfixes it. - When I log each of the
ifblocks insplitNodes, I see that the loop initially goes into theshouldSplitandisOutsideblocks, but eventually gets caught in theshouldBreakblock and then always goes back into that one.
Like @tom2strobl said above, it seems like the margins might be a red herring and the bug is actually caused by a node with specific attributes being split.
I'm continuing to investigate the page splitting logic to see where the loop might be happening. Apologies for the relative lack of details and clarity in my comment 😁
- We're running in Node.js; I'm testing against a local Express server
- I've linked my test setup to a local copy of the
react-pdflibrary so testing against the latest commit in master
Did a bunch of digging and debugging into this.
I believe the root cause is shouldBreak.js is causing nodes to be broken that would fit in the page on their own, but not with their bottom margin included. When there are no sibling nodes present, then the lower value (sometimes -Infinity) will be used, preventing this node from breaking. Otherwise the result of getEndOfMinPresenceAhead is used which is child.box.top + child.box.height + child.box.marginBottom + getMinPresenceAhead(child) (minPresenceAhead = 0 in my case). So if the sibling nodes are smaller than the current node, the bug is prevented, if they are larger then the bug is possible.
We have paddingTop: 35 set on our pages which causes the first node after a relayout to have child.box.top == 35 (represented as 99 in the example below)
Example (in the form of params to shouldBreak):
- Height: 1000
child.box.top: 99child.box.height: 900child.box.marginTop: 0child.box.marginBottom: 10futureElements: Anything with at least one child wherebox.top + box.height > 1009
shouldSplit == false->height < child.box.top + child.box.height->1000 < 99 + 900endOfPresence == 1009-> code herebreakingImprovesPresence == true->child.box.top > child.box.marginTop->99 > 0
!shouldSplit && endOfPresence > height && breakingImprovesPresence->true && 1009 > 1000 && true
These conditions cause shouldBreak to return true. But when this child breaks, resolvePagination.js will push it and all following nodes to the next page (in the splitNodes function), while resetting its top. On the next loop, the first node will have its top reset. In this iteration, nothing on this node has changed to it will again trigger shouldBreak == true, and again gets pushed to the next page. This causes there to always be a nextPage in the paginate function, so while nextPage !== null will keep looping infinitely, as it will never split the problem node, and thus progress through the document.
Workaround
Using paddingBottom instead of marginBottom seems to work ok, but has some tradeoffs, especially if you're using background colors.
Suggested Fix
- Does
getEndOfMinPresenceAheadinshouldBreak.jsreally need to include the bottom margin of the node if we're already at the end of the page anyway? - Should
breakingImprovesPresencehave similar logic for the bottom margin as it does for the top margin, only returning true if the extra bottom space is truly needed?
I'm also running into an infinite loop hanging and ultimately crashing the browser. I don't have much to add except that I tried removing any and all bottom margin in my entire document, and I still got the infinite loop hanging. Was also using React-pdf version 3.3.8 on chrome, and I was not using any minPresenceAhead.
Edit: I solved my problem. It seems that my issue was that I had a Page element whose first child was a View that had break enabled. I was unable to reproduce the issue in REPL though, so it's not as simple as adding a break like that. It's likely related to some of the other stuff I have in my document, but removing the break did fix it for me. Figured I'd share this info.
I'm seeing roughly the same issue as well. We're rendering a list of entry components (elements in a medical record). Certain combinations will cause ReactPDF to go into an infinite loop in the
resolvePaginationstep. In a flame graph,relayoutends up being the function called over and over.For one of our problem documents:
- Shuffling the the entries fixes the infinite loop
- Removing all bottom margins fixes it
- Removing the
if (shouldBreak) {}block fromsplitNodesfixes it.- When I log each of the
ifblocks insplitNodes, I see that the loop initially goes into theshouldSplitandisOutsideblocks, but eventually gets caught in theshouldBreakblock and then always goes back into that one.Like @tom2strobl said above, it seems like the margins might be a red herring and the bug is actually caused by a node with specific attributes being split.
I'm continuing to investigate the page splitting logic to see where the loop might be happening. Apologies for the relative lack of details and clarity in my comment 😁
- We're running in Node.js; I'm testing against a local Express server
- I've linked my test setup to a local copy of the
react-pdflibrary so testing against the latest commit in master
Removing marginBottom on my mapped fields worked!! thank you!