As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact
Last Comment Bug 743819 - Spell checking causes performance degradation in Firefox 10+
: Spell checking causes performance degradation in Firefox 10+
: regression
Product: Core
Classification: Components
Component: Spelling checker (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Aryeh Gregor (:ayg) (next working March 28-April 26)
: Jet Villegas (:jet)
Depends on: 923376
Blocks: 674212
  Show dependency treegraph
Reported: 2012-04-09 14:16 PDT by albright
Modified: 2013-10-07 10:02 PDT (History)
12 users (show)
ayg: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

spellcheckperfbug.html (1.86 KB, text/html)
2012-04-09 14:16 PDT, albright
no flags Details
Patch v1 (2.82 KB, patch)
2012-04-24 03:55 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
ehsan: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image albright 2012-04-09 14:16:58 PDT
Created attachment 613391 [details]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/536.5 (KHTML, like Gecko) Chrome/19.0.1084.15 Safari/536.5

Steps to reproduce:

The attached web page demonstrates the problem. A large table is constructed on the page and two fixed position content editable divs are synchronized via an onkeypress handler. Typing in the left input is copied to the one on the right on each key press.

Actual results:

Typing speed is very laggy - its easy to get ahead of the UI making keyboard access very slow.

Expected results:

This test runs very well in FF 9. Also, there is a button on the right of the page that will turn off spell checking at the document level (disabling it in the content editables). This causes the performance to return to normal. One note, a key difference between FF 9 and 10+ is that content in the destination content editable is spell checked immediately  in FF 10+ whereas it was not in FF 9. This bug also seems to be dependent on the total amount of DOM on the page (the size of the background table).
Comment 1 User image Matthias Versen [:Matti] 2012-04-09 16:45:13 PDT
Last good nightly: 2011-08-17
First bad nightly: 2011-08-18


bug 674212 ?
Comment 2 User image Alice0775 White 2012-04-10 20:28:10 PDT
Regression window(cached m-i)
Cannot reproduce
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110817 Firefox/9.0a1 ID:20110817115336
Can reproduce
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110817 Firefox/9.0a1 ID:20110817141601

In local build
Last good ;2c5ef30a89b2
First bad :800f7541fb20

Triggered by:
800f7541fb20	Fabien Cazenave — Bug 674212 - Modifying text of a contenteditable DOM Node removes spellcheck underlinings; r=ehsan
Comment 3 User image :Ehsan Akhgari 2012-04-23 15:43:47 PDT
Aryeh, can you take a look at this, please?
Comment 4 User image Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-04-23 23:55:06 PDT
Well, the problem seems to be that bug 674212 calls nsEditor::SyncRealTimeSpell(), which calls nsIInlineSpellChecker::SetEnableRealTimeSpell(), which is meant to be used when spellcheck is first initialized.  That calls nsIInlineSpellChecker::SpellCheckRange(nsnull), which just re-spellchecks everything.  If we knew what range changed, we could call SpellCheckRange() directly with an appropriate range so it only has to recheck that.  But nsHTMLEditRules::DocumentModifiedWorker doesn't seem to know about what nodes changed.  Is there some way for it to find out?  Maybe the re-spellchecking can be moved to some other method?
Comment 5 User image Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-04-24 03:55:59 PDT
Created attachment 617836 [details] [diff] [review]
Patch v1

This seems to work.  I take it it doesn't need a test, because it's perf-only.
Comment 6 User image Mozilla RelEng Bot 2012-04-24 04:00:11 PDT
Autoland Patchset:
	Patches: 617836
	Branch: mozilla-central => try
Try run started, revision 27ae8cb68de3. To cancel or monitor the job, see:
Comment 7 User image Mozilla RelEng Bot 2012-04-24 09:16:05 PDT
Try run for 27ae8cb68de3 is complete.
Detailed breakdown of the results available here:
Results (out of 224 total builds):
    exception: 1
    success: 182
    warnings: 38
    failure: 3
Builds (or logs if builds failed) available at:
Comment 8 User image :Ehsan Akhgari 2012-04-24 15:24:26 PDT
Comment on attachment 617836 [details] [diff] [review]
Patch v1

Review of attachment 617836 [details] [diff] [review]:

This looks good, but please verify that it doesn't regress bug 674212 before landing.  Thanks!
Comment 9 User image Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-04-24 22:57:07 PDT
Isn't that what the reftest is for?  Manually checking the original test-case seems to show that it still works fine too.
Comment 11 User image :Ehsan Akhgari 2012-04-25 07:47:03 PDT
Comment on attachment 617836 [details] [diff] [review]
Patch v1

Review of attachment 617836 [details] [diff] [review]:

This is a safe fix for a perf regression since Firefox 10.  I'd like to take it on Aurora and Beta if possible.
Comment 12 User image Lukas Blakk [:lsblakk] use ?needinfo 2012-04-25 11:59:26 PDT
Comment on attachment 617836 [details] [diff] [review]
Patch v1

[Triage comment]
Low risk, please go ahead.
Comment 14 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-26 14:12:53 PDT
Given this is fixed, is qawanted still needed?
Comment 15 User image :Ehsan Akhgari 2012-04-26 14:24:21 PDT
Comment 16 User image Virgil Dicu [:virgil] [QA] 2012-05-09 08:22:52 PDT
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0

Verified in F13b3 using attached test case on Mac OS 10.6, Ubuntu 12.04 and Windows 7. Typing speed normal, the same when toggling spell checker on/off.
Comment 17 User image Virgil Dicu [:virgil] [QA] 2012-06-07 08:53:20 PDT
Mozilla/5.0 (X11; Linux i686; rv:14.0) Gecko/20100101 Firefox/14.0

Also verified in F14 on same platforms, using the same test case.

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