Closed Bug 581576 Opened 10 years ago Closed 10 years ago

hung up or too slow when press Enter key on Gmail editor which has a lot of misspelled words

Categories

(Core :: DOM: Editor, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b5
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: Peter6, Assigned: masayuki)

References

Details

(Keywords: perf, regression)

Attachments

(1 file, 4 obsolete files)

Mozilla/5.0 (Windows; Windows NT 5.1; rv:2.0b3pre) Gecko/20100723 Minefield/4.0b3pre ID:20100723040900

repro:
Open FF
Open an e-mail and press the answer button
Put the curson in the textfield to add some lines of text
See how FF almost freezes

Even if you switch to another tab the problem remains
It's hard to tell if this is the same issue as Bug 579260
blocking2.0: --- → ?
Peter, any chance of hunting down a regression range?
(In reply to comment #2)
> Peter, any chance of hunting down a regression range?

No problem Boris, I'll try to find some time for it the next few days.
works in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100411 Minefield/3.7a5pre ID:20100411035843

fails in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100412 Minefield/3.7a5pre ID:20100412040827 

so this regressed between changesets 7fc54ac24458 and 201589e94d3b

maybe bug 221820 or bug 542919 or bug 552914 (all CORE:EDITOR)

I have no hourly builds to narrow it down
Peter, if you don't have the ability to make your own builds to narrow this down further, we could make try server builds with intermediate changesets for you to try.
(In reply to comment #5)
> Peter, if you don't have the ability to make your own builds to narrow this
> down further, we could make try server builds with intermediate changesets for
> you to try.

No I don't build myself Timothy
If you could make the tryserver builds I'd be happy to try them.

these were the checked in bugs:
bug 221820 [Core:Editor]-CreateBogusNodeIfNeeded is extremely expensive creating Input fields [All]
bug 397823 [Core:ImageLib]-moz-icon URL for nonexistent content type doesn't return generic icon [Mac]
bug 542919 [Core:Editor]-Refactor the plain text editor initialization to facilitate lazy initialization [Mac]
bug 552163 [Core:Widget: Win32]-[OOPP]Can not start scroll page by mouse wheel when mouse cursor is over a flash video. [Win]
bug 552914 [Core:Editor]-nsEditor::mFlags is never modified by SetFlags() [All]
bug 553366 [Core:Layout]-When flushing we need to flush external documents too [All]
bug 558461 [Firefox:Tabbed Browser]-Firefox asks a confirmation message when trying to close just one tab: "You are about to close 1 tab. Are you sure you want to continue?" [All]
bug 558598 [Core:XUL]-XUL splitters should be collapsed on the correct side in RTL mode [Mac]
bug 558643 [Firefox:Toolbars]-Remove unused toolbox-top class [All
bug 554544  [Core:Layout]-Wikipedia crashes [@ nsDisplayTextDecoration::Paint(nsDisplayListBuilder*, nsIRenderingContext*) ] and [@ nsTextFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) ] [Mac]
bug 555711 [Core:DOM: Core & HTML]-nsAttrValue::ParseNonNegativeIntValue should return PR_FALSE when the value is not valid [All]

So maybe there are other suspicious checkins that could cause the huge processorload when the cursor is inside the iframe.
I'll try to test them later tonight (CEST) Timothy
both rev 369599ed72c0 and rev 81c847ae642e work fine Timothy
If you can provide more testbuilds I'd be happy to test them till we find the cause
81c847ae642e is the try server revision of mozilla-central revision 369599ed72c0. So they both refer to the same revision. Just to make sure, you are saying that both builds worked fine and you just mixed up the revisions in your last comment?
Ok, thank you. I will get some new builds for you going and post links when they are ready.
Peter, could you please try these two builds and report back? We should need at most one more round of this.

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/tnikkel@gmail.com-e4903ac1e352/
(this is m-c 0eda41f145c8)

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/tnikkel@gmail.com-66113c8e88b4/
(this is m-c 9acb882b2890)
Peter, could you try this build? It should be the last one.

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/tnikkel@gmail.com-0aae6b37d920/
(this is m-c 97aec61ae55f)
(In reply to comment #15)
> Peter, could you try this build? It should be the last one.
> 
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/tnikkel@gmail.com-0aae6b37d920/
> (this is m-c 97aec61ae55f)

Broken aswell Timothy
So that means that the regressing changeset is either b501122477cc or 97aec61ae55f. b501122477cc only changed debug code so I don't think it could be that. So it must be 97aec61ae55f, which is bug 552914. Masayuki, any idea what might be causing this?
If my patch is the cause, when you disable spellcheck on pref level, it might be fixed because nsEditor::SetFlags() are not executed before bug 552914, but after that, SyncRealTimeSpell() is called when the editor flag is updated. I think that SyncRealTimeSpell() should be called only when the necessity of spellchecker is changed.

# I cannot confirm the performance regression on my PC...
setting layout.spellcheckDefault = 0 (disable spellchecker) 
indeed fixes the problem
Right, some more info.
This happens in a mail in the dutch language about half a pag long with most words underlined because I use an en-us version of minefield with the default en-us spellchecking.

I copied the same amount of english text in the textfield and with just a few words underlined the problem is barely noticable
Attached patch Patch v1.0 (obsolete) — Splinter Review
Thank you for your test. This patch must fix this bug.

There are test builds:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/masayuki@d-toybox.com-d33a8cef5a55/

Ehsan: The cause of this bug is we always call SyncRealTimeSpellCheck() when SetFlags() is called, so, the spellchecker always rechecked all words when user type Enter in Gmail editor. Therefore, we should call SynRealTimeSpellCheck() from nsEditor::SetFlags() only when the change causes spellchecker enabled state change.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #461846 - Flags: review?(ehsan)
Component: General → Editor
OS: Windows XP → All
QA Contact: general → editor
Hardware: x86 → All
Attached patch Patch v1.0 (obsolete) — Splinter Review
Oops, sorry for the spam, I attached another bug's patch.
Attachment #461846 - Attachment is obsolete: true
Attachment #461847 - Flags: review?(ehsan)
Attachment #461846 - Flags: review?(ehsan)
Summary: FF almost locks up when I answer an e-mail in Gmail → hung up or too slow when press Enter key on Gmail editor which has very many words
about the changed title
"hung up or too slow when press Enter key on Gmail editor which has very many
words"
It doesn't cover what I experience.

"Gmail editor is very slow with lots of misspelled words (e.g. other language than Minefield locale)"

comes much closer, the enter key has nothing to do with it, simply using the
arrowkey to navigate left/right/up/down shows the problem best with massive
processorload
Comment on attachment 461847 [details] [diff] [review]
Patch v1.0

Firstly, this looks really good, I even wonder why we didn't do this before!

But I have two comments:

1. As far as I can see, the only effect that setting mFlags can have on the result of GetDesiredSpellCheckState is if it changes the expression |IsPasswordEditor() || IsReadonly() || IsDisabled()|.  Therefore, I don't see why we need to run all the other checks that that function does here.

2. Could you please just use NS_ENSURE_SUCCESS?  The code pattern with NS_FAILED and NS_WARNING loses the return value itself.
Attachment #461847 - Flags: review?(ehsan)
(In reply to comment #24)
> 1. As far as I can see, the only effect that setting mFlags can have on the
> result of GetDesiredSpellCheckState is if it changes the expression
> |IsPasswordEditor() || IsReadonly() || IsDisabled()|.  Therefore, I don't see
> why we need to run all the other checks that that function does here.

Hmm, then, should I create a new flag check method like CanEnableSpellCheck() or something? And I think that it should be checked first in GetDesiredSpellCheckState() because I think that it didn't expect that nsIEditor::setSpellcheckUserOverride() can override the state of such editors. E.g., on readonly editor and password editor, the "Check Spelling" menu is hidden.

> 2. Could you please just use NS_ENSURE_SUCCESS?  The code pattern with
> NS_FAILED and NS_WARNING loses the return value itself.

I think that even if SyncRealTimeSpellCheck() is failed, we should continue for other jobs. And looks like SyncRealTimeSpellCheck() never returns error code but it's an interface method, so, should I create |void SyncRealTimeSpellCheckInternal()| without virtual? Then, we can reduce the virtual call during editing.
(In reply to comment #25)
> (In reply to comment #24)
> > 1. As far as I can see, the only effect that setting mFlags can have on the
> > result of GetDesiredSpellCheckState is if it changes the expression
> > |IsPasswordEditor() || IsReadonly() || IsDisabled()|.  Therefore, I don't see
> > why we need to run all the other checks that that function does here.
> 
> Hmm, then, should I create a new flag check method like CanEnableSpellCheck()
> or something? And I think that it should be checked first in
> GetDesiredSpellCheckState() because I think that it didn't expect that
> nsIEditor::setSpellcheckUserOverride() can override the state of such editors.
> E.g., on readonly editor and password editor, the "Check Spelling" menu is
> hidden.

Yes, CanEnableSpellCheck is what we want, and checking it first in GetDesiredSpellCheckState also makes sense.

> > 2. Could you please just use NS_ENSURE_SUCCESS?  The code pattern with
> > NS_FAILED and NS_WARNING loses the return value itself.
> 
> I think that even if SyncRealTimeSpellCheck() is failed, we should continue for
> other jobs. And looks like SyncRealTimeSpellCheck() never returns error code
> but it's an interface method, so, should I create |void
> SyncRealTimeSpellCheckInternal()| without virtual? Then, we can reduce the
> virtual call during editing.

OK, but can you do that in a follow-up bug?  I don't want this patch to change the behavior of SetFlags like that.
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #461847 - Attachment is obsolete: true
Attachment #464300 - Flags: review?(ehsan)
Comment on attachment 464300 [details] [diff] [review]
Patch v1.1

Looks great!
Attachment #464300 - Flags: review?(ehsan) → review+
Comment on attachment 464300 [details] [diff] [review]
Patch v1.1

This is a new regression, and the risk is low.
Attachment #464300 - Flags: approval2.0?
Summary: hung up or too slow when press Enter key on Gmail editor which has very many words → hung up or too slow when press Enter key on Gmail editor which has a lot of misspelled words
Attachment #464300 - Flags: approval2.0? → approval2.0+
Comment on attachment 464300 [details] [diff] [review]
Patch v1.1

>-  // Check for password/readonly/disabled, which are not spellchecked
>-  // regardless of DOM
>-  if (IsPasswordEditor() || IsReadonly() || IsDisabled()) {
>+  if (CanEnableSpellCheck()) {
>     return PR_FALSE;
>   }

I may be being incredibly stupid but shouldn't this be
if (!CanEnableSpellCheck()) {
Attached patch Patch for fix the nit (obsolete) — Splinter Review
Oh, thanks.
Attachment #464739 - Flags: review?(ehsan)
Whiteboard: both patched were landed, but the follow up patch needs r+a
Whiteboard: both patched were landed, but the follow up patch needs r+a → both patches were landed, but the follow up patch needs r+a
Comment on attachment 464739 [details] [diff] [review]
Patch for fix the nit

backed out the both patches because they cause test_contextmenu failure.
Attachment #464739 - Flags: review?(ehsan)
Whiteboard: both patches were landed, but the follow up patch needs r+a
Did both of the patches get backed out?
Yes, I'm looking for the cause.

# Unfortunately, today's nightly build includes both patches.
Attached patch Patch v2.0Splinter Review
Maybe, the cause is that when PostCreate() calls SetFlags(mFlags), SyncRealTimeSpellCheck() isn't always called.

test_contextmenu is passed on my environment, I posted the patch to tryserver, I'll request review if all tests passed on tryserver.
Attachment #464300 - Attachment is obsolete: true
Attachment #464739 - Attachment is obsolete: true
Depends on: 586405
Comment on attachment 464874 [details] [diff] [review]
Patch v2.0

Okay, this patch works fine on tryserver.
Attachment #464874 - Flags: review?(ehsan)
Attachment #464874 - Flags: review?(ehsan) → review+
Comment on attachment 464874 [details] [diff] [review]
Patch v2.0

All tests passed on tryserver. So, I think that this patch doesn't change the actual logic except the optimization.
Attachment #464874 - Flags: approval2.0?
Comment on attachment 464874 [details] [diff] [review]
Patch v2.0

a2.0=dbaron
Attachment #464874 - Flags: approval2.0? → approval2.0+
blocking2.0: ? → betaN+
http://hg.mozilla.org/mozilla-central/rev/705ea088433a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
Depends on: 589577
You need to log in before you can comment on or make changes to this bug.