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

RESOLVED FIXED in mozilla2.0b5

Status

()

Core
Editor
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Peter6, Assigned: masayuki)

Tracking

({perf, regression})

Trunk
mozilla2.0b5
perf, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Comment 1

7 years ago
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?
(Reporter)

Comment 3

7 years ago
(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.
Keywords: regressionwindow-wanted
(Reporter)

Comment 4

7 years ago
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.
(Reporter)

Comment 6

7 years ago
(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.
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/tnikkel@gmail.com-4d7dd99aaa99/
(this is rev 35f66b11ec8b in the range)

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/tnikkel@gmail.com-81c847ae642e/
(this is rev 369599ed72c0 in the range)
(Reporter)

Comment 8

7 years ago
I'll try to test them later tonight (CEST) Timothy
(Reporter)

Comment 9

7 years ago
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?
(Reporter)

Comment 11

7 years ago
both builds, from 
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/tnikkel@gmail.com-4d7dd99aaa99/
and
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/tnikkel@gmail.com-81c847ae642e/
worked fine
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)
(Reporter)

Comment 14

7 years ago
both 

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

and 

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/tnikkel@gmail.com-66113c8e88b4/
(this is m-c 9acb882b2890)

show the problem Timothy
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)
(Reporter)

Comment 16

7 years ago
(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?
Blocks: 552914
Keywords: regressionwindow-wanted
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...
(Reporter)

Comment 19

7 years ago
setting layout.spellcheckDefault = 0 (disable spellchecker) 
indeed fixes the problem
(Reporter)

Comment 20

7 years ago
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
Created attachment 461846 [details] [diff] [review]
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/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
Created attachment 461847 [details] [diff] [review]
Patch v1.0

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

Comment 23

7 years ago
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

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

Comment 26

7 years ago
(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.
Created attachment 464300 [details] [diff] [review]
Patch v1.1
Attachment #461847 - Attachment is obsolete: true
Attachment #464300 - Flags: review?(ehsan)

Comment 28

7 years ago
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 30

7 years ago
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()) {
Created attachment 464739 [details] [diff] [review]
Patch for fix the nit

Oh, thanks.
Attachment #464739 - Flags: review?(ehsan)
http://hg.mozilla.org/mozilla-central/rev/f05b59f1ff3d
http://hg.mozilla.org/mozilla-central/rev/ba956b17d834
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

Comment 34

7 years ago
Did both of the patches get backed out?
Yes, I'm looking for the cause.

# Unfortunately, today's nightly build includes both patches.
Created attachment 464874 [details] [diff] [review]
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.
Attachment #464300 - Attachment is obsolete: true
Attachment #464739 - Attachment is obsolete: true

Updated

7 years ago
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)

Updated

7 years ago
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
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5

Updated

7 years ago
Depends on: 589577
You need to log in before you can comment on or make changes to this bug.