Last Comment Bug 635618 - Typing goes incredibly slow in a contentEditable div when there are a lot of divs
: Typing goes incredibly slow in a contentEditable div when there are a lot of ...
Status: RESOLVED FIXED
: perf, testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86 Windows 7
: P1 normal (vote)
: mozilla11
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
http://www.groundbooth.com/
Depends on: 684638 713427 CVE-2012-4213
Blocks: 700538
  Show dependency treegraph
 
Reported: 2011-02-20 09:54 PST by Art Geigel
Modified: 2012-10-11 03:17 PDT (History)
9 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
sample.html (896.67 KB, text/html)
2011-11-03 18:11 PDT, Alice0775 White
no flags Details
part 1. Nix the slow IsElementVisible and replace it with a direct primary frame check. (1.86 KB, patch)
2011-11-04 19:42 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
part 2. Add nsINode overloads of various editor helper methods. (18.95 KB, patch)
2011-11-04 19:44 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
part 3. Switch GetPriorNodeImpl and GetNextNodeImpl to working with nsINode. (11.14 KB, patch)
2011-11-04 19:45 PDT, Boris Zbarsky [:bz]
ehsan: review+
Details | Diff | Splinter Review
part 4. Switch GetNextNode and GetPriorNode to working with nsINode. (5.34 KB, patch)
2011-11-04 19:45 PDT, Boris Zbarsky [:bz]
ehsan: review+
Details | Diff | Splinter Review
part 5. Pass an optional argument to GetPriorNode and GetNextNode to limit the search. (296 bytes, patch)
2011-11-04 19:45 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Part 5 for real (15.54 KB, patch)
2011-11-04 19:49 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Part 5 with silly whitespace issue fixed too (15.54 KB, patch)
2011-11-04 19:52 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Part 1 with the necessary IsElement check reinstated so we pass the browserscope editor tests (2.16 KB, patch)
2011-11-07 15:41 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Part 2 merged to the part 1 changes (19.53 KB, patch)
2011-11-07 15:47 PST, Boris Zbarsky [:bz]
ehsan: review+
Details | Diff | Splinter Review
Part 5 with a necessary check on entry into this code to prevent assertions triggering (15.61 KB, patch)
2011-11-07 15:49 PST, Boris Zbarsky [:bz]
ehsan: review+
Details | Diff | Splinter Review
Part 1 updated to work with LazyFC, as discussed on irc (3.47 KB, patch)
2011-11-07 18:41 PST, Boris Zbarsky [:bz]
ehsan: review+
Details | Diff | Splinter Review
Part 2 updated to comments (19.45 KB, patch)
2011-11-07 18:50 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Part 3 updated to comments (11.18 KB, patch)
2011-11-07 18:56 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Part 4 updated to comments (5.40 KB, patch)
2011-11-07 19:06 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Part 5 with those changes (16.16 KB, patch)
2011-11-07 19:37 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Interdiff for part 5 (1.97 KB, patch)
2011-11-07 19:37 PST, Boris Zbarsky [:bz]
ehsan: review+
Details | Diff | Splinter Review

Description Art Geigel 2011-02-20 09:54:52 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/534.13 (KHTML, like Gecko) Chrome/9.0.597.98 Safari/534.13
Build Identifier: 3.6.13

When there are several contentEditable divs on a page, typing is incredibly slow when typing in one div, but only on the first line.  Typing takes forever, but if you hit enter to go to the next line within the contentEditable div the typing speed is normal.  It seems that it goes slowly only on the first line.

Reproducible: Always

Steps to Reproduce:
1.Create many contentEditable div objects on the screen (100 should be sufficient)
2.Place the cursor inside one of the div objects to begin typing
3.Typing on the first line goes really slowly.  Hit enter to go to the next line.  Notice that the speed of typing is normal.
Actual Results:  
Slow typing

Expected Results:  
Normal typing speed within a contentEditable div object when there are a lot of contentEditable div objects.

The typing speed should not be any different no matter how many contentEditable div objects there are.  This type of issue is not seen in IE nor Chrome.
Comment 1 Ed Morley [:emorley] 2011-02-20 10:01:13 PST
Could you attach a reduced html testcase please.

Does this occur in the latest Firefox 4 beta?
Comment 2 Art Geigel 2011-02-20 10:22:18 PST
You can view the bug by visiting: http://www.groundbooth.com/ff_div_issue.php

Scroll down on the page until you see the "Add Case" section.  Click within the contentEditable div associated with "Case Name" (or whatever) and note how slow the typing runs on the first line.  Hit enter to go to the next and see how typing runs at a normal speed.

I will dl 4beta and test now...

AG3
Comment 3 Art Geigel 2011-02-20 10:46:49 PST
The problem exists in 4beta also.  I just confirmed.

AG3
Comment 4 (mostly gone) XtC4UaLL [:xtc4uall] 2011-02-20 18:09:08 PST
Confirmed using Mozilla/5.0 (Windows NT 5.1; rv:2.0b12pre) Gecko/20110220 Firefox/4.0b12pre ID:20110220030351.
Comment 5 Boris Zbarsky [:bz] 2011-02-22 11:31:40 PST
Hmm.  I can't reproduce this on the testcase in comment 2 (on Mac, with a current nightly)...
Comment 6 Chris 2011-11-03 06:03:57 PDT
Is there any updates on this bug?

I'm using version 7.0.1 and I'm getting the same problem. My page has over 8000 DOM elements and only one contentEditable div is set when a users edits a section (removed afterwards on save/cancel). I get the slow typing on the first line depending on how far in the DOM is my contentEditable div set. Towards the end of my document it's very slow while setting a contentEditable div at the beginning is fast.
Note that I cannot reproduce the problem in IE or Chrome.

Thanks
Comment 7 Boris Zbarsky [:bz] 2011-11-03 06:15:26 PDT
The latest update is comment 5:  I tried to reproduce the problem and could not.

If you have a testcase that shows the problem, please attach it to the bug.  That would be very much appreciated.
Comment 8 Boris Zbarsky [:bz] 2011-11-03 10:56:15 PDT
Chris pointed me to a testcase that he unfortunately can't make public.

What I see on his testcase is that when I first see the contenteditable area there's some lag and right after I click in it there's a few seconds of lag.  After that, typing is fast for me.  If I blur the contenteditable field I get the lag again.  If I focus it after that I get it again.  Chris, is that what you see as well?

On a hunch, I logged spellcheck events, and when the contenteditable area is shown or when I click in it or when I blur it there are three such events posted.  Each one takes about 2 seconds to process.  So you get 6 seconds of lag.  No idea where the "three" comes from, offhand.

Now the good news is that with the patches in bug 684638 there is only one spellcheck event instead of 3, and it takes us 20ms to handle it, not 2 seconds.  At that point I'm not seeing much lag at all.  If people are interested in trying the builds with those patches, they're available at <http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bzbarsky@mozilla.com-5a1c9caa43d8>.  I'd really appreciate knowing whether those solve the problems people are seeing!
Comment 9 Chris 2011-11-03 11:52:56 PDT
The lag that I see is on the keydown in the contenteditable div area. The problem is not on the inital focusin, since no matter how long I wait or type the keys are showing very slowly.

Again just like the initial post, the problem is on the first line. If I add a line feed and type on the second line or more it's always fast and if I type again on the first line the characters are rendered slowly.

What is more confusing is that I never get this problem with a contentEditable div at the start of my document. If I have one that the rendering is slow and move the DOM object to the beginning of my document the rendering will be normal.
Comment 10 Boris Zbarsky [:bz] 2011-11-03 12:41:28 PDT
Hmm.  Ok, I definitely can't reproduce that behavior...

I'd really appreciate you trying the builds linked to in comment 8 and reporting back whether you still see the problem in those.
Comment 11 Boris Zbarsky [:bz] 2011-11-03 12:58:23 PDT
One other thing worth checking:  Do you see the issue in safe mode?
Comment 12 Chris 2011-11-03 17:42:58 PDT
I tried the nightly build you provided above and the issue wasn't there. Everything was working fine. No lag whatsoever.
Safe Mode didn't change anything.
Comment 13 Alice0775 White 2011-11-03 18:11:16 PDT
Created attachment 571861 [details]
sample.html

This sample contains 9000div and contentEdittable.


[STR]
1. Open sample html
2. Press "k" and hold keydown. 

3. Input Enter key so that the caret go to 2nd line.
4. Press "k" and hold keydown. 

5. Compare speed of the key repeat of step 2 and step 4.


[Actual]
Key in speed of Step2 is slower than Step4.
Comment 14 Alice0775 White 2011-11-03 18:32:22 PDT
And the tryserver build in comment 8 does not seem to help this problem.
Comment 15 Boris Zbarsky [:bz] 2011-11-03 19:12:48 PDT
OK.  So maybe we have two separate issues here.

Just to check... Chris, if you try a build from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2011/11/2011-11-01-03-11-08-mozilla-central/ does _that_ show the problem you saw?  That's just this morning's nightly build without any spellchecker fixes.

Alice0775, I'll look at your testcase tomorrow.
Comment 16 Chris 2011-11-04 06:08:44 PDT
No, it's the same issue. The provided file by Alice0775 generates the behaviour that I'm getting. However in my case holding keydown will make Firefox Unresponsive. The rendering of characters on the first line is much slower than in the provided file. But we can still see in the file that Alice0775 provided that keys are rendered much slower on the first line.

I tried to create a much bigger file with some actual content, but at this point either the file is not big enough or structured in a too simple format to generate the "Page Unresponsive" or my file is so big that editing the contentEditable makes the browser crash.

- Build in comment 8 solves the problem for me.
- Build in comment 15 solves the problem, but now the first key of each line is rendered slowly. Holding keydown takes literally over 1 second, before the first key is shown and then the behaviour is normal.
- Safe Mode doesn't change anything to problem found in 7.0.1
Comment 17 Boris Zbarsky [:bz] 2011-11-04 07:39:12 PDT
OK.  So given comment 16, I think we have the following:

1)  The main issue Chris is seeing is just fixed on trunk already.
2)  The remaining issue (first char on each line) is fixed as of this morning.

Both of those will ship in Firefox 10.

On Alice's testcase, I do see the problem if I squint enough, or if I throttle my CPU back enough.  A profile shows that it's a problem similar to bug 684638 but in editor, not spellcheck.  More details once my debug build finishes.  This should not be particularly hard to fix now that I can reproduce it.  The key is the display:none content, which the groundbooth site also has...
Comment 18 Boris Zbarsky [:bz] 2011-11-04 13:34:17 PDT
So what happens in Alice0775's testcase is that nsHTMLEditRules::WillInsert and nsHTMLEditRules::CheckInterlinePosition (called from nsHTMLEditRules::AfterEditInner) call nsHTMLEditor::GetPriorHTMLNode which calls nsEditor::GetPriorNode.

Now nsEditor::GetPriorNode has three problems:

1)  It gets computed style.  This is expensive for display:none elements.  Furthermore, it just wants to know whether the display is "none", which is easier to check by checking for a frame, I think.  In fact it's better that way, because display might be none on some ancestor.  And right now it uses a non-flushing computed style lookup, so we wouldn't even need to Flush_Frames.

2)  nsIDOMNode everywhere.

3)  It walks right out of the contenteditable area.

#1 is trivial to fix.  #2 is not that hard.  Thinking about #3.
Comment 19 Boris Zbarsky [:bz] 2011-11-04 19:34:42 PDT
OK, patches coming up.  Without them the time spent under GetPriorNode is about 65% of the time needed to process the keypress for me.  The first patch (addressing #1 above) cuts down that number to about 20%.  The next 3 patches (addressing #2 above) cut it down to about 6%.  The last patch (addressing #3 above) cuts it down to under 0.1% for sure; hard to say more exactly at that point because the random sampling noise probably dominates the data.
Comment 20 Boris Zbarsky [:bz] 2011-11-04 19:42:40 PDT
Created attachment 572156 [details] [diff] [review]
part 1.  Nix the slow IsElementVisible and replace it with a direct primary frame check.
Comment 21 Boris Zbarsky [:bz] 2011-11-04 19:44:50 PDT
Created attachment 572157 [details] [diff] [review]
part 2.  Add nsINode overloads of various editor helper methods.
Comment 22 Boris Zbarsky [:bz] 2011-11-04 19:45:13 PDT
Created attachment 572158 [details] [diff] [review]
part 3.  Switch GetPriorNodeImpl and GetNextNodeImpl to working with nsINode.
Comment 23 Boris Zbarsky [:bz] 2011-11-04 19:45:35 PDT
Created attachment 572159 [details] [diff] [review]
part 4.  Switch GetNextNode and GetPriorNode to working with nsINode.
Comment 24 Boris Zbarsky [:bz] 2011-11-04 19:45:55 PDT
Created attachment 572160 [details] [diff] [review]
part 5.  Pass an optional argument to GetPriorNode and GetNextNode to limit the search.
Comment 25 Boris Zbarsky [:bz] 2011-11-04 19:49:00 PDT
Created attachment 572161 [details] [diff] [review]
Part 5 for real
Comment 26 Boris Zbarsky [:bz] 2011-11-04 19:52:39 PDT
Created attachment 572162 [details] [diff] [review]
Part 5 with silly whitespace issue fixed too
Comment 27 Boris Zbarsky [:bz] 2011-11-04 20:29:27 PDT
Ehsan, you probably want to wait on the reviews a bit; the tests are triggering at least some of the asserts I added.  I'll look into that on Monday.
Comment 28 Mozilla RelEng Bot 2011-11-04 23:30:26 PDT
Try run for 22b56a583a98 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=22b56a583a98
Results (out of 192 total builds):
    success: 163
    warnings: 29
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bzbarsky@mozilla.com-22b56a583a98
Comment 29 Boris Zbarsky [:bz] 2011-11-07 15:41:16 PST
Created attachment 572651 [details] [diff] [review]
Part 1 with the necessary IsElement check reinstated so we pass the browserscope editor tests
Comment 30 Boris Zbarsky [:bz] 2011-11-07 15:47:44 PST
Created attachment 572656 [details] [diff] [review]
Part 2 merged to the part 1 changes
Comment 31 Boris Zbarsky [:bz] 2011-11-07 15:49:42 PST
Created attachment 572659 [details] [diff] [review]
Part 5 with a necessary check on entry into this code to prevent assertions triggering
Comment 32 Boris Zbarsky [:bz] 2011-11-07 16:16:58 PST
That last set of patches should not pass all the tests.  I pushed them to try to make sure.
Comment 33 :Ehsan Akhgari 2011-11-07 18:17:52 PST
Comment on attachment 572656 [details] [diff] [review]
Part 2 merged to the part 1 changes

Having to add these overloads is really sad, but I'm not going to ask you to fix the entire editor code.  r=me.

(Nit: please use for(;;) instead of do/while(1), and adjust the assertion comment following those two occurrences.)
Comment 34 :Ehsan Akhgari 2011-11-07 18:27:32 PST
Comment on attachment 572158 [details] [diff] [review]
part 3.  Switch GetPriorNodeImpl and GetNextNodeImpl to working with nsINode.

Nit: please pass goForward as an argument to FindNextLeafNode and make it non-template.

Carrying forward the do/while nit.

Also please file a follow-up to get nsHTMLEditor::IsRootNode overrides checking against the active editable area root.
Comment 35 :Ehsan Akhgari 2011-11-07 18:28:59 PST
Comment on attachment 572159 [details] [diff] [review]
part 4.  Switch GetNextNode and GetPriorNode to working with nsINode.

Carrying forward the boolean template argument nit.
Comment 36 Boris Zbarsky [:bz] 2011-11-07 18:41:29 PST
Created attachment 572715 [details] [diff] [review]
Part 1 updated to work with LazyFC, as discussed on irc
Comment 37 Boris Zbarsky [:bz] 2011-11-07 18:50:39 PST
Created attachment 572716 [details] [diff] [review]
Part 2 updated to comments
Comment 38 Boris Zbarsky [:bz] 2011-11-07 18:56:46 PST
Created attachment 572718 [details] [diff] [review]
Part 3 updated to comments

Filed bug 700538
Comment 39 Boris Zbarsky [:bz] 2011-11-07 19:06:24 PST
Created attachment 572720 [details] [diff] [review]
Part 4 updated to comments
Comment 40 Boris Zbarsky [:bz] 2011-11-07 19:32:18 PST
I was updating part 5, when I realized that my previous try run still had two oranges.

I looked at those, and found that:

1)  editor/libeditor/base/crashtests/633709.xhtml ends up with two <input> elements.  Then
    it calls nsHTMLEditor::GetPriorHTMLNode with |inParent| being one of the <input>s 
    while activeEditingHost is the _other_ <input>.  That seems pretty broken, and
    triggers asserts about bogus aCurrentNode not descending from editing root arguments.
2)  editor/libeditor/html/crashtests/582138-1.xhtml calls the (parent, offset) version of
    GetPriorNode with that parent being the parent of our editable root and the offset
    being 0 (which is the offset of the editable root in that parent).  This ends up
    calling the (currrent-node) version of GetPriorNode with that parent as the argument,
    which asserts.

I suppose I could deal with both cases by checking that the input node is a descendant of the root in the (current-node) version of GetPriorNode.  I'll do that for now.
Comment 41 Boris Zbarsky [:bz] 2011-11-07 19:37:18 PST
Created attachment 572731 [details] [diff] [review]
Part 5 with those changes
Comment 42 Boris Zbarsky [:bz] 2011-11-07 19:37:46 PST
Created attachment 572732 [details] [diff] [review]
Interdiff for part 5
Comment 43 Mozilla RelEng Bot 2011-11-07 21:10:28 PST
Try run for 7418264b40fb is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=7418264b40fb
Results (out of 214 total builds):
    success: 189
    warnings: 25
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bzbarsky@mozilla.com-7418264b40fb
Comment 44 Mozilla RelEng Bot 2011-11-07 23:30:29 PST
Try run for 95712fff53d1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=95712fff53d1
Results (out of 216 total builds):
    success: 195
    warnings: 20
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bzbarsky@mozilla.com-95712fff53d1
Comment 45 :Ms2ger (⌚ UTC+1/+2) 2011-11-07 23:38:38 PST
Comment on attachment 572720 [details] [diff] [review]
Part 4 updated to comments

>--- a/editor/libeditor/base/nsEditor.cpp
>+++ b/editor/libeditor/base/nsEditor.cpp
>-  nsCOMPtr<nsIDOMNode> candidate =
>-    do_QueryInterface(FindNextLeafNode(currentNode, false, bNoBlockCrossing));

>-    nsCOMPtr<nsIDOMNode> notEditableNode = do_QueryInterface(candidate);

\o/
Comment 46 :Ehsan Akhgari 2011-11-25 11:04:17 PST
Comment on attachment 572715 [details] [diff] [review]
Part 1 updated to work with LazyFC, as discussed on irc

Review of attachment 572715 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Comment 47 :Ehsan Akhgari 2011-11-25 11:04:41 PST
Comment on attachment 572732 [details] [diff] [review]
Interdiff for part 5

Review of attachment 572732 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Note You need to log in before you can comment on or make changes to this bug.