Created attachment 613391 [details] spellcheckperfbug.html 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).
Last good nightly: 2011-08-17 First bad nightly: 2011-08-18 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dcb25d71220d&tochan ge=f69a10f23bf3 bug 674212 ?
Regression window(cached m-i) Cannot reproduce http://hg.mozilla.org/integration/mozilla-inbound/rev/05268baefef7 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110817 Firefox/9.0a1 ID:20110817115336 Can reproduce http://hg.mozilla.org/integration/mozilla-inbound/rev/800f7541fb20 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110817 Firefox/9.0a1 ID:20110817141601 Pushlog http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=05268baefef7&tochange=800f7541fb20 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
Aryeh, can you take a look at this, please?
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?
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.
Autoland Patchset: Patches: 617836 Branch: mozilla-central => try Destination: http://hg.mozilla.org/try/pushloghtml?changeset=27ae8cb68de3 Try run started, revision 27ae8cb68de3. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=27ae8cb68de3
Try run for 27ae8cb68de3 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=27ae8cb68de3 Results (out of 224 total builds): exception: 1 success: 182 warnings: 38 failure: 3 Builds (or logs if builds failed) available at: http://firstname.lastname@example.org
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!
Isn't that what the reftest is for? Manually checking the original test-case seems to show that it still works fine too. https://hg.mozilla.org/integration/mozilla-inbound/rev/733f13a5cca6
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 on attachment 617836 [details] [diff] [review] Patch v1 [Triage comment] Low risk, please go ahead.
Given this is fixed, is qawanted still needed?
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.
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.