Avoid pathological QT text/markdown behavior… #886

pull davidgumberg wants to merge 1 commits into bitcoin-core:master from davidgumberg:2025-09-03-textedit-oom-fix changing 2 files +24 −1
  1. davidgumberg commented at 11:30 pm on September 3, 2025: contributor

    …during text selection by only setting plaintext mime data.

    Fixes the OOM described in #887.

    The issue is related to the construction of the text/markdown MIME data for the selection. Using the heaptrack utility, I observed that nearly all of the allocations when reproducing happen in QTextMarkdownWriter::writeFrame. I am not 100% sure what is causing this issue in QT’s conversion of our HTML to markdown; I have tried changing the HTML tags (e.g. using <p></p> and <ul><li></li></ul> in place of tables) used in our rpcconsole messages, but the issue recurs.

    The solution applied here is to override createMimeDataFromSelection() to avoid construction of the (likely never-used anyways) text/markdown mime data, and only set plaintext mime data in the clipboard.

  2. DrahtBot commented at 11:30 pm on September 3, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK hebasto
    Concept ACK w0xlt
    Approach ACK katesalazar

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    LLM Linter (✨ experimental)

    Possible typos and grammar issues:

    • QT’s -> Qt’s [proper capitalization of the Qt framework name]
    • mime -> MIME [acronym should be uppercase for clarity]
    • , this avoids -> . This avoids [comma splice; split into two sentences for grammatical correctness]

    drahtbot_id_5_m

  3. w0xlt commented at 11:33 pm on September 3, 2025: contributor
    Concept ACK
  4. maflcko commented at 6:14 am on September 4, 2025: contributor
    (ci failure can be ignored, see https://github.com/bitcoin/bitcoin/issues/33293)
  5. katesalazar commented at 4:35 pm on September 4, 2025: none

    […] I am not 100% sure what is causing this issue in QT’s conversion of our HTML to markdown […]

    You implying that an upstream bug must be searched in Qt?

  6. katesalazar commented at 4:39 pm on September 4, 2025: none

    Thank you

    Approach ACK

    possible nit:

    0PlainCopyQTextEdit
1         ^
  7. katesalazar commented at 4:46 pm on September 4, 2025: none
    nit: splash post in two paragraphs (none tiny) then the commit message is a one liner
  8. davidgumberg commented at 6:55 pm on September 4, 2025: contributor

    You implying that an upstream bug must be searched in Qt?

    “Bug” might be too strong, having one HTML element be so large is probably pretty unsually behavior for users of QTextEdit, but I can’t say for sure what is going wrong in QTextMarkdownWriter::writeFrame.

    possible nit:

    0PlainCopyQTextEdit
1         ^

    I don’t really agree, I figure that it’s best not to use Q when overriding a QT base class since this makes it obvious to users of PlainCopyTextEdit that it’s custom and not from upstream.

    nit: splash post in two paragraphs (none tiny) then the commit message is a one liner

    I’m not sure I follow what you’re suggesting.

  9. hebasto renamed this:
    gui: Avoid pathological QT text/markdown behavior...
    Avoid pathological QT text/markdown behavior...
    on Sep 5, 2025
  10. hebasto commented at 11:16 pm on September 5, 2025: member
    Concept ACK.
  11. hebasto added this to the milestone 30.0 on Sep 5, 2025
  12. hebasto commented at 2:16 pm on September 6, 2025: member

    Tested 5cb39181019dd1d132da9e87e88921e477facbae on Ubuntu 24.04 (Qt 6.4.2).

    The patch indeed fixes the pathological memory usage. However, it modifies the clipboard content. The copy-pasted prompt appears as follow:

    • on the master branch:
    0Welcome to the Bitcoin Core RPC console.
1Use up and down arrows to navigate history, and Ctrl+L to clear screen.
2Use Ctrl++ and Ctrl+- to increase or decrease the font size.
3Type help for an overview of available commands.
4For more information on using this console, type help-console.
    • with this PR:
    0Welcome to the Bitcoin Core RPC console.<0x2029>
Use up and down arrows to navigate history, and Ctrl+L to clear screen.
<0x2029>Use Ctrl++ and Ctrl+- to increase or decrease the font size.
<0x2029>Type help for an overview of available commands.
<0x2029>For more information on using this console, type help-console.

    (<0x2029> characters have been visualised manually)

  13. hebasto removed this from the milestone 30.0 on Sep 6, 2025
  14. Ari4ka approved
  15. gui: Avoid pathological QT text/markdown behavior... 
    during text selection by only setting plaintext mime data.
    6a371b70c8
  16. davidgumberg commented at 6:22 pm on September 9, 2025: contributor

    The patch indeed fixes the pathological memory usage. However, it modifies the clipboard content. The copy-pasted prompt appears as follow:

    […]

    Thanks for catching this, I should have checked that clipboard contents actually were reasonable with my change, my mistake.

    I’ve updated the branch to fix both the newline issue, and the issues that existed with copying icon’s and some of the other rich text stuff in the console. I use the same function as upstream to convert the selected QTextDocumentFragment to plaintext, so clipboard contents should be identical to master, I’ve checked a few simple cases and they look right to me.

  17. davidgumberg force-pushed on Sep 9, 2025
  18. w0xlt commented at 6:43 pm on September 9, 2025: contributor
    I wasn’t able to detect any memory increase on macOS when running getblocktemplate ‘{“rules”: [“segwit”]}’ in the GUI. Is this issue specific to Linux?
  19. davidgumberg commented at 6:50 pm on September 9, 2025: contributor

    I wasn’t able to detect any memory increase on macOS when running getblocktemplate ‘{“rules”: [“segwit”]}’ in the GUI. Is this issue specific to Linux?

    Are you running from a release or a depends build? This will only occur when linking against a QT with the textmarkdownwriter feature enabled, which we disable in our depends build of QT. See discussion here: #887 (comment)

  20. davidgumberg commented at 9:08 pm on September 9, 2025: contributor
    CI Failure is unrelated to this PR: https://github.com/bitcoin/bitcoin/issues/33345
  21. hebasto approved
  22. hebasto commented at 11:22 am on September 10, 2025: member
    ACK 6a371b70c87ad6b763c89384562fce8549f37434.
  23. hebasto merged this on Sep 10, 2025
  24. hebasto closed this on Sep 10, 2025
  25. hebasto commented at 11:28 am on September 10, 2025: member

    @fanquake

    Backport to 30.x?

  26. fanquake referenced this in commit 6b19ede1a5 on Sep 10, 2025
  27. fanquake commented at 11:44 am on September 10, 2025: member
    Backported to 30.x in https://github.com/bitcoin/bitcoin/pull/33356.
  28. alexanderwiederin referenced this in commit 49e068b15b on Sep 16, 2025
  29. alexanderwiederin referenced this in commit 4b0c2f2a8f on Sep 17, 2025
  30. alexanderwiederin referenced this in commit 2edb618ffe on Sep 17, 2025
  31. glozow referenced this in commit b7a722724d on Sep 17, 2025
  32. stringintech referenced this in commit fb8510ba20 on Sep 17, 2025
  33. bug-castercv502 referenced this in commit 9f5d12d027 on Sep 28, 2025
  34. yuvicc referenced this in commit d89c6d0002 on Sep 28, 2025


davidgumberg DrahtBot w0xlt maflcko katesalazar hebasto Ari4ka fanquake

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-11-07 18:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me