Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Browser crashes when we try to copy/paste data from spreadsheet #449

Open
f1ames opened this issue Jun 1, 2017 · 22 comments
Open

Browser crashes when we try to copy/paste data from spreadsheet #449

f1ames opened this issue Jun 1, 2017 · 22 comments
Labels
plugin:pastefromword The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. support An issue reported by a commercially licensed client. type:bug A bug.
Milestone

Comments

@f1ames
Copy link
Contributor

f1ames commented Jun 1, 2017

Are you reporting a feature or a bug?

Bug

Provide detailed reproduction steps (if any)

Use ie_data.zip

  1. Open document.
  2. Copy its content.
  3. Paste into editor.

Expected result

Content is pasted properly without freezing a browser or throwing errors.

Actual result

  • Chrome / Firefox: takes substantial amount of time to process the whole paste.
  • IE11 / Edge: browser crashes.

Other details

  • CKEditor version: 4.7.0
@f1ames f1ames added plugin:pastefromword The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. type:bug A bug. labels Jun 1, 2017
@f1ames f1ames self-assigned this Jun 1, 2017
@hponka
Copy link

hponka commented Jun 2, 2017

Maybe same bug: paste content to Ckeditor 4.7.0 inline editor crashes the editor. More specifically: editor disappears and only pasted content is showing.
Browser: Chrome 58.0.3029.110

@f1ames
Copy link
Contributor Author

f1ames commented Jun 2, 2017

@hponka, does it happen when pasting content from Excel or just regular content? If it is an issue with regular copy/paste, I would ask you to report a separate issue with reproduction steps.

@f1ames
Copy link
Contributor Author

f1ames commented Jun 2, 2017

Well, the file contains a lot of data so processing it takes a lot of time indeed. In Chrome pasting it takes about 35 seconds.
image

One function which needs to be investigated is clipboard cacheData method which copies the data from native data transfer and takes more than 11 seconds.

@hponka
Copy link

hponka commented Jun 2, 2017

@f1ames, it happen with regular content.

@mlewand
Copy link
Contributor

mlewand commented Jun 2, 2017

@hponka could you provide more examples? I mean like source docx / xlsx files - also env would be helpful (os, browser, office verison).

@hponka
Copy link

hponka commented Jun 2, 2017

@f1ames Not yet. The bug happened with full preset and many extra plugins. Need to test more - without extra plugins and with basic preset.

@mlewand
Copy link
Contributor

mlewand commented Jun 2, 2017

On a side note: in fact obtaining data from clipboard might be a serious bottleneck. I recall playing around with fetching rtf, which take 20mb or more in real documents. Unfortunately it locks the main thread, and can't be processed in web worker due to security reasons.

I know there was some movement with new asynchronous clipboard api, but I'd assume it's not going to be a reality soon, since even today regular clipboard api still has some implementation gaps between the browsers.

@f1ames the most important thing is to ensure that only required data type is cached (here text/html). Under any condition rtf should not be fetched unless required (which is not ATM).

But then again... clipboard api (thus data transfer) is not supported on IE11 and it still breaks the browser...

@hponka
Copy link

hponka commented Jun 2, 2017

@f1ames
It happens every times with Ckeditor 4.7.0 full preset (dynamically loaded inline editor) when you:

  1. press backspace in empty editor
  2. then paste some content (e.g. pure text)

But it doesn't happens with Ckeditor 4.7.0 basic preset. So the bug must be in some plugin which is included in the full preset but not in basic.

@f1ames
Copy link
Contributor Author

f1ames commented Jun 2, 2017

Thanks for the details @hponka. Would it be possible for you to report it as a separate issue so we can proceed it separately from this one?

@hponka
Copy link

hponka commented Jun 2, 2017

@f1ames Buggy plugin is tableselection. When I remove it the bug not happen.

@f1ames
Copy link
Contributor Author

f1ames commented Jun 2, 2017

@mlewand More than half of the execution time in cacheData method is spent accessing the native datatransfer files array (as Excel provides also an image of whole copied table). If optimizes it can be probably reduced to 5 seconds. And rest is html processing in general.

There is a room for improvement for sure, but this is a separate issue not connected to pastefromword plugin. I will extract it to a separate issue.

@hponka
Copy link

hponka commented Jun 2, 2017

@f1ames Reported paste problem as separate issue: #460

@f1ames
Copy link
Contributor Author

f1ames commented Jun 7, 2017

To test PFW performance I temporarily removed all code accessing datatransfer.$.files as it also creates some overheat in this case (as described in #462).

Without PFW pasting whole table takes few seconds. Memory allocation profile:
screen shot 2017-06-07 at 15 08 58

With PFW:
screen shot 2017-06-07 at 15 15 56

It looks like processing html in the htmlparser uses the most resources as it operates on huge arrays (visible in the console) containing parsed html parts.
Need to verify if it is same case for IE.

@f1ames
Copy link
Contributor Author

f1ames commented Jun 8, 2017

Digging deeper (using Chrome profiling tools which are more convenient than IE ones) it looks like the cause of the browser freeze/slowdown is not in a one place, but in a few places. As the data in the clipboard is quite big (text/html data inside clipboard has more than 22k lines) and pastefromword requires more processing (and filtering) than normal paste it is the effect of scale that the processing takes that long. Below are three partial call stacks which takes 90% of the whole data processing on paste:

screen shot 2017-06-08 at 15 05 54

The fakeSelectionPasteHandler method is inside tableselection plugin and it does some processing on the pasted table.

screen shot 2017-06-08 at 15 08 04

The processDataForInsertion is a core method which prepares pasted data so it can be inserted into the editor.

screen shot 2017-06-08 at 15 08 57

The last one is the filter call triggered by cleanWord method in pastefromword plugin.

We may try to optimize each of those places separately or think of more general solution for pasting big sets of data (not really sure what it could be).

I looked also into IE11/Edge and was able to paste only 500 rows table (the whole document has about 1700 rows) without hanging the browser, but the call stack looked similar to the one in Chrome.

So it seems as a more complex problem than optimizing one bottleneck.

@f1ames
Copy link
Contributor Author

f1ames commented Jun 27, 2017

Tested with plain contenteditable:
IE11: ~ 15 seconds
Edge: ~ 50 seconds (making Edge not responding)
Chrome: 1-2 seconds
FF: 1-2 seconds

Which means in Chrome and FF it is a CKEditor code which adds the whole overheat (as described in a previous comments). However, for IE11 and Edge there is some initial overheat already, so even if with optimise processing of pasted content it will still take a substantial amount of time in IE11 and especially Edge.

Reported Edge issue https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/12514691/.

@f1ames
Copy link
Contributor Author

f1ames commented Jun 28, 2017

One additional test was performed to see how disabling tableselection and pastefromword plugins affects execution time.

Chrome

Tableselection Pastefromword Paset execution time [seconds]
N N ~14
Y N ~15
N Y ~30
Y Y ~38

One interesting finding is that tableselection by itself does not affects execution time that much (fakeSelectionPasteHandler is the most expensive tableselection function):
screen shot 2017-06-27 at 15 36 07

but when pastefromeword is also enabled, tableselection execution time grows significantly:
screen shot 2017-06-27 at 15 38 50

IE11

Tests were performed without browser dev console (pasting with opened console takes almost 4 minutes in first case).

Tableselection Pastefromword Paset execution time [seconds]
N N ~73 [1]
Y N ~190[2]
N Y ~190[2]
Y Y -
  1. In some tests the pasted content was not inserted (but browser and CKEditor was functional). After successful paste, the browser window was quite unresponsive (few seconds delay).
  2. Browser prompts with error Unspecified error after given time. The editor and browser is usable after closing error dialog. It seems it is the default timeout for IE11 to stop long running scripts execution.

@f1ames f1ames removed their assignment Jun 28, 2017
@f1ames
Copy link
Contributor Author

f1ames commented Jun 28, 2017

@mlewand should we mark it as wontfix or we will get back to it some day?

@mlewand
Copy link
Contributor

mlewand commented Jun 28, 2017

Let's leave it open and we'll revisit it in the future.

@mlewand mlewand added the support An issue reported by a commercially licensed client. label Jul 3, 2017
@mlewand mlewand added this to the Backlog milestone Jul 4, 2017
@mlewand mlewand added the target:minor Any docs related issue that can be merged into a master or major branch. label Aug 30, 2017
@Ruud-cb
Copy link

Ruud-cb commented Sep 27, 2019

Hi,

Seeing this issue is already 2+ years old, what suggestion do you guys have to improve on this? I do use ckeditor5, but I am getting complains that an large amount of text can't be copied without the browser complaining about a slow page and asking the user to force-quit the tab (Chrome).
Also when I have loaded the webpage on read-only format, the entire text displays just fine (just displaying the HTML text). When I try to edit the record (so that ckeditor initiates) is a pain, in such a way that, after the long initialization, writing a single word takes more then 50 seconds. to write.

How do I solve this?

First part I press enter. Then you see a pause. Then I try to write "Test". Insanely long.

image

I can understand a little bit that coping might take long, then a workaround would be to copy it in parts. But unfortunately after all text is copied it still produces issues like mentioned above.

@f1ames
Copy link
Contributor Author

f1ames commented Sep 30, 2019

Hello @Ruud-cb!

I do use ckeditor5, but I am getting complains that an large amount of text can't be copied without the browser complaining about a slow page and asking the user to force-quit the tab (Chrome).

Does your issue concerns CKEditor 4 or CKEditor 5? If it's about CKEditor 5, please report to https://github.com/ckeditor/ckeditor5.

As for CKEditor 4, we haven't been able to make any visible improvements regarding this issue. This is a complex one, since it's partially browser native issue (there is a slowdown when using plain contenteditable too) and a sum of smaller performance improvements which can be introduced to CKEditor itself, but requires significant amount of work for analysis and implementation.

@Ruud-cb
Copy link

Ruud-cb commented Sep 30, 2019

Hi @f1ames, yes it's CKeditor5 I am using, just commenting here because there is already a large discussion happened here and the #1 google search result.

Sorry to hear that you have not found any solution so far. I will do some more research myself for the default and other HTML-editors to see if that changes something. If it does then I guess I will open a new issue at CKeditor5's repo.

@f1ames
Copy link
Contributor Author

f1ames commented Sep 30, 2019

I will do some more research myself for the default and other HTML-editors to see if that changes something.

@Ruud-cb feel free to share any findings here too, so we may think about some fixes based on that 👍

@f1ames f1ames removed the target:minor Any docs related issue that can be merged into a master or major branch. label Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin:pastefromword The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. support An issue reported by a commercially licensed client. type:bug A bug.
Projects
None yet
Development

No branches or pull requests

4 participants