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

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Peter van der Woude [:Peter6]]

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

Comment 1

[Peter van der Woude [:Peter6]]

It's hard to tell if this is the same issue as Bug 579260

Comment 2

[Boris Zbarsky [:bzbarsky]]

Peter, any chance of hunting down a regression range?

Comment 3

[Peter van der Woude [:Peter6]]

(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.

Comment 4

[Peter van der Woude [:Peter6]]

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

Comment 5

[Timothy Nikkel (:tnikkel)]

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.

Comment 6

[Peter van der Woude [:Peter6]]

(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.

Comment 7

[Timothy Nikkel (:tnikkel)]

For my reference, the range from comment 4 http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7fc54ac24458&tochange=201589e94d3b

Peter, could you try these two builds and report back?

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-4d7dd99aaa99/
(this is rev 35f66b11ec8b in the range)

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-81c847ae642e/
(this is rev 369599ed72c0 in the range)

Comment 8

[Peter van der Woude [:Peter6]]

I'll try to test them later tonight (CEST) Timothy

Comment 9

[Peter van der Woude [:Peter6]]

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

Comment 10

[Timothy Nikkel (:tnikkel)]

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?

Comment 11

[Peter van der Woude [:Peter6]]

both builds, from 
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-4d7dd99aaa99/
and
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-81c847ae642e/
worked fine

Comment 12

[Timothy Nikkel (:tnikkel)]

Ok, thank you. I will get some new builds for you going and post links when they are ready.

Comment 13

[Timothy Nikkel (:tnikkel)]

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/[e-mail]-e4903ac1e352/
(this is m-c 0eda41f145c8)

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-66113c8e88b4/
(this is m-c 9acb882b2890)

Comment 14

[Peter van der Woude [:Peter6]]

both 

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-e4903ac1e352/
(this is m-c 0eda41f145c8)

and 

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-66113c8e88b4/
(this is m-c 9acb882b2890)

show the problem Timothy

Comment 15

[Timothy Nikkel (:tnikkel)]

Peter, could you try this build? It should be the last one.

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-0aae6b37d920/
(this is m-c 97aec61ae55f)

Comment 16

[Peter van der Woude [:Peter6]]

(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/[e-mail]-0aae6b37d920/
> (this is m-c 97aec61ae55f)

Broken aswell Timothy

Comment 17

[Timothy Nikkel (:tnikkel)]

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?

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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...

Comment 19

[Peter van der Woude [:Peter6]]

setting layout.spellcheckDefault = 0 (disable spellchecker) 
indeed fixes the problem

Comment 20

[Peter van der Woude [:Peter6]]

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

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

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/[e-mail]-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.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

Oops, sorry for the spam, I attached another bug's patch.

Comment 23

[Peter van der Woude [:Peter6]]

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 24

[(no longer active)]

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.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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.

Comment 26

[(no longer active)]

(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.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.1

Comment 28

[(no longer active)]

Comment on attachment 464300 [details] [diff] [review]
Patch v1.1

Looks great!

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 464300 [details] [diff] [review]
Patch v1.1

This is a new regression, and the risk is low.

Comment 30

[John P Baker]

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()) {

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for fix the nit

Oh, thanks.

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/f05b59f1ff3d
http://hg.mozilla.org/mozilla-central/rev/ba956b17d834

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 464739 [details] [diff] [review]
Patch for fix the nit

backed out the both patches because they cause test_contextmenu failure.

Comment 34

[(no longer active)]

Did both of the patches get backed out?

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yes, I'm looking for the cause.

# Unfortunately, today's nightly build includes both patches.

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.0

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.

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 464874 [details] [diff] [review]
Patch v2.0

Okay, this patch works fine on tryserver.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 464874 [details] [diff] [review]
Patch v2.0

a2.0=dbaron

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/705ea088433a