CollaboraOnline / online

Collabora Online is a collaborative online office suite based on LibreOffice technology. This is also the source for the Collabora Office apps for iOS and Android.
https://collaboraonline.com
Other
1.85k stars 704 forks source link

Unhandled "Maximum call stack size exceeded" error by large "jsdialog:" response #6954

Open Traktormaster opened 1 year ago

Traktormaster commented 1 year ago

Describe the bug It is possible that the WSD sends a "jsdialog:" payload so big, that "Maximum call stack size exceeded" error will be raised and not handled properly.

To Reproduce

  1. Open a writer, draw or presentation file and add a drawing object.
  2. When the object is selected the sidebar's jsdialog data payload will fail to process with:
    Socket.js:411 Uncaught RangeError: Maximum call stack size exceeded
    at NewClass._extractCopyObject (Socket.js:411:35)
    at NewClass._extractTextImg (Socket.js:473:9)
    at NewClass._slurpMessage (Socket.js:393:8)
    ...

The culprit line is: https://github.com/CollaboraOnline/online/blob/master/browser/src/core/Socket.js#L412

If I replace that with one of the following, the error does not happen and the sidebar loads:

Desktop (please complete the following information)

Additional context

eszkadev commented 1 year ago

I can't reproduce. What do you mean by "and add a drawing object", inserting a shape?

Traktormaster commented 1 year ago

Here is a draw file I can reproduce it with, with a screenshot of the issue. Selecting any of the shapes will trigger an error in the console and the sidebar on the right will not change to show the shape-editing controls. stack-error draw.odg

eszkadev commented 1 year ago

I can't reproduce with provided file. Local build, master:

COOLWSD version: 23.05.2.1 git hash: bfdac84d;)

Ezinnem commented 1 year ago

@Traktormaster Please can you test again using an updated Collabora Online version?

Traktormaster commented 1 year ago

I won't be able to do a local build and check soon.

However, even if the new version would not produce this error, I still argue that the bug has not been addressed appropriately, because the line of code that produced the error is still faulty.

The String.fromCharCode function is called with as many arguments as many items are in the input array. This means that, at some point it will fail on all platforms, because there is a limited amount of stack space for the function call to be made. This is a limitation on the Javascript VM that must be avoided.

The second solution I mention is a method a few lines below the linked line, that is explicitly made to work-around this issue. There is even a comment explaining it: // convert to string of bytes without blowing the stack if data is large.

The issue will present itself based on the available stack of the browser client and the size of the WSD response payload. These can vary, so it seems not everyone will reproduce the issue with the coolwsd editor application right now. Here is a little snippet anyone can use in the developer console to approximately check, what the stack size limit that triggers the issue is on their machine:

function testf(n) {
  var tmp = new Uint8Array(n);
  for (var i = 0 ; i < n ; ++i ) {
    tmp[i] = 97;
  }
  console.log("testf", n, String.fromCharCode.apply(null, tmp));
}
testf(50);
testf(500000);
testf(5000000);

For me the size is different in Chromium and Firefox on the same machine even. The Chromium limit is around 128KB, about a fifth of what works on Firefox: image image

Please address the issue with either fix I proposed.

As a side-note: I'm not even sure if using String.fromCharCode is correct, because the WSD communication is UTF-8 encoded, but that function decodes UTF-16 code units according to MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/fromCharCode Is there a good reason not to use the TextDecoder API instead? https://developer.mozilla.org/en-US/docs/Web/API/TextDecoder