nbdime icon indicating copy to clipboard operation
nbdime copied to clipboard

SVG diffs are shown as code crashing browser

Open krassowski opened this issue 2 years ago • 1 comments

When diffing a notebook with a few SVG images, my browser often freezes and becomes unusable. There is no such a problem with PNGs. The difference is in the PNG blobs being shown as images, while SVG is shown as syntax-highlighted code. This leads to a very large memory consumption when a few mildly complex SVG images are present and a crash after an SIGILL:

Screenshot from 2021-11-25 11-19-22

I'm willing to work on this ASAP as it prevents me from diffing some notebooks for my thesis, but I would appreciate hints on the codebase and any remarks on whether it should be configurable, or maybe only kick-in for SVGs of certain size, etc.

krassowski avatar Nov 25 '21 11:11 krassowski

Sorry that you're experiencing this issue! As a potential short-term workaround: It might also be worth checking the "trust outputs" checkbox, as that will likely cause the SVGs to render (as the SVG mimerender is declared as unsafe). I don't think there is a way to declare trust before the diff is already rendered though :-/

If you want to make some changes to the logic, the code for output rendering should be here:

https://github.com/jupyter/nbdime/tree/master/packages/nbdime/src/diff/model https://github.com/jupyter/nbdime/tree/master/packages/nbdime/src/diff/widget

Unfortunately, that part of the code base was always a little complex (inheritance and naming-wise). The best effort suggestion I can make is:

  1. Add a CLI option to set the default trust state. Ensure this gets used when we create new output diff models. Maybe a static property on OutputDiffModel ?
  2. For cases that (1) are not renderable, or for other reasons prefer a text render, (2) the string data is prohibitively large for a diff comparison, insert a placeholder display, e.g. "Diff too large to show. Attempt anyway (clickable link to ignore safety for this output only). Implementation wise, probably I would add one or more props on OutputModel to track the safety opt-out, which the OutputPanel.createOutputTextView could then use and listen to changes to (via a new signal)?

We can probably do both or just either of these. Any contribution is welcome!

vidartf avatar Dec 20 '21 17:12 vidartf