1322666 - It is very delayed to pop up contextmenu if spelling checker is enabled, And No contextmenu pops up when e10s is disabled

Status: VERIFIED FIXED

(Core :: Spelling checker, defect)

Description

[Alice0775 White]

The problem is since Firefox47.

Steps To Reproduce:
1. Type "FindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFindFind" to the textfiled
2. Click out side of the textfield to move focused element
3. Right click on the textfiled

Actual Results:
It is very delayed to pop up contextmenu.
Browser is unresponsive for a while.
Sometime contextmenu is not shown.

Expected Results:
not delay

Regression Window:

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]: No context will pop up menu when e10s is disabled. this should be fixed for next 52ESR at least.

Actual Results:
No contextmenu pops up when e10s is disabled.
It is very delayed to pop up contextmenu when e10s is enabled.


Regression Window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=909af7c19ebbb6e9d4bcde44b56294cd9aff892e&tochange=f4317b12c1a36499b94c0ccdf910b195be13ce66

Regressed by :f4317b12c1a3	Ryan VanderMeulen — Bug 1257902 - Update Hunspell to version 1.4.0. r=ehsan

Comment 2

[Ryan VanderMeulen [:RyanVM]]

There's an upstream issue noting the same problem. A performance profile would be helpful here if possible to see if there's something that can be fixed more easily than waiting for 2.x (which is highly unlikely to ever get backported to an ESR branch).

Comment 3

[Julien Cristau [:jcristau]]

tracking this for 52 in case we find a reasonably contained fix

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Just ran a profile of the testcase from comment 0. Looks like it's spending ~71% of its time in HashMgr::hash, 14% in HashMgr::lookup, and another 8% in AffixMgr::compound_check. Maybe that helps point to a performance chokepoint?

https://cleopatra.io/#report=547f8eded0db4956540cefbcaa92779ba0cb9691&filter=%5B%7B%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A430307,%22end%22%3A432446%7D%5D

Comment 5

[Ryan VanderMeulen [:RyanVM]]

I'm hoping the testcase and profile information is enough to reproduce locally on the upstream end. I'm happy to create test builds with possible fixes as well if needed.

Comment 6

[Dimitrij Mijoski]

Why the call graph is only 4 levels deep? All the functions from Hunspell are on the same level. Is that because of inlining?

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Sorry, it was pointed out to me that I had tested this on a release build, which strips frame pointers and relies on pseudo stacks instead. Here's a profile from a nightly build, which has a more complete call stack.

https://cleopatra.io/#report=a2b23c8175323124fd315eb3f502f41810bb1cf6&search=spell&selection=0,140,141,142,143,144,145,1,146,147,148,149,150,151,152,153,227,154,374,375,376,433,434,435,436,437,438,439,440,441,452,443,444,455,456,457

If you think it's worth it, I can also work on trying to bisect down to an offending upstream commit as well.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

I was able to bisect this down to upstream commit 3bb456b.
https://github.com/hunspell/hunspell/commit/3bb456bb5c265f6e1f191a961ec5e90533464b9b

From the changelog entry:
> 2016-04-15 Caolán McNamara <caolanm at LibO>:
> 	* remove MAXWORDUTF8LEN and MAXWORDLEN limits
> 	  - prior to this words over certain limits would not be
> 	    spellchecked. Its possible that some clients may suffer
> 	    performance issues now that we have dropped the limits
> 	    and need to determine a length limit of their own
>           to use to not send strings for spellchecking.

So, it sounds like this was an intentional change. And also pretty edge-case. Alice, how did you come upon this to begin with? The testcase is 156 characters, so it previously would have not been spellchecked at all IIUC (limit was 100).

Comment 9

[Ryan VanderMeulen [:RyanVM]]

As a band-aid for the release branches, we could maybe just revert that commit for now until a better long-term fix can be found.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

I can also confirm that trimming the testcase to under 100 characters is pretty laggy in older Firefox versions too. So yeah, I don't think anything actually got any slower in Hunspell here. It's just more permissive about what gets checked than it used to.

Comment 11

[Alice0775 White]

> Alice, how did you come upon this to begin with?

I encounter this when I test Bug 1322655.

> No contextmenu pops up when e10s is disabled.

At least, this should be fixed.

Comment 12

[Dimitrij Mijoski]

Seems fine to me to add back the limit for the word size.

The long-term fix is, well, Hunspell version 2. Although, I'm working very slowly so far.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

(In reply to Alice0775 White from comment #11)
> At least, this should be fixed.

I can't reproduce this on Nightly, Aurora, or Release (with their respective Hunspell versions). In all cases, I get the hang and eventually the popup with e10s enabled and disabled.

I took a quick look at reverting the upstream commit. Looks like it'll be easy to do on version 1.4.1 for 52 and lower, but less so on version 1.5.4 for 53+.

Comment 14

[Alice0775 White]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)
> (In reply to Alice0775 White from comment #11)
> > At least, this should be fixed.
> 
> I can't reproduce this on Nightly, Aurora, or Release (with their respective
> Hunspell versions). In all cases, I get the hang and eventually the popup
> with e10s enabled and disabled.
> 

If you cannot reproduce, right click again.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Interesting, this time I didn't get the popup on the first right-click, but did on all subsequent ones. One the first instance, I got a temporary flash of Windows title bar badness, which I assume is related. Probably a timing issue. I assume putting the max length check back in will resolve it.

Of course, now I'm more confused given https://github.com/hunspell/hunspell/commit/418a66d779b87fd5c95d61974f9278a012014225. I'm out of time to look into this for now. Will investigate more later.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Oh, this actually looks trivial to fix. Just need to change that 176 back to 100 and we should be good to go.

And I'll file an upstream issue about adding an #ifndef on that so it can be easily overridden at build time instead of having to modify the upstream sources.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Yep, setting it back to 100 makes the testcase nice and responsive again.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Attachment: [Gecko 52 and older] Set MAXWORDLEN back to 100

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1257902
[User impact if declined]: Browser hangs (and crashes from the looks of bug 1322655) in some extreme cases.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: I was able to easily trigger the bad behavior using the testcase in this bug and verified that the patch fixes it. Happy to let this request sit until it bakes on Nightly for a bit if RelMan so desires, though.
[Needs manual test from QE? If yes, steps to reproduce]: None needed
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's just reverting us back to a maximum word length of 100 characters, which was the default for all previous Hunspell versions since we originally imported it back in 2008.
[String changes made/needed]: None

I wouldn't normally request ESR45 approval on this given that it's kind of edge-casey, but the patch is so trivial that I see little reason not to, frankly.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Attachment: [Gecko 53+] Set MAXWORDLEN back to 100

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3028e2f40e0
Change MAXWORDLEN to 100, matching the previous default from Hunspell 1.3. r=masayuki

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f3028e2f40e0

Comment 22

[Gerry Chang [:gchang]]

Hi Alice, 
could you help verify if this issue is fixed as expected on the latest Nightly build? Thanks!

Comment 23

[Alice0775 White]

(In reply to Gerry Chang [:gchang] from comment #22)
> Hi Alice, 
> could you help verify if this issue is fixed as expected on the latest
> Nightly build? Thanks!

I verified that the problem was fixed on latest m-c build[1].
No longer reproduce the long delay and/or contextmenu problem with/without e10s.

[1]
https://hg.mozilla.org/mozilla-central/rev/c750a7de1194d71f2d2ef73f6a919d26b9640cae
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0 ID:20161215051900

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Great find, Alice0775 White. Thanks for reporting!

Comment 25

[Julien Cristau [:jcristau]]

Comment on attachment 8818754 [details] [diff] [review]
[Gecko 52 and older] Set MAXWORDLEN back to 100

bring hunspell max word length back to its previous value to fix perf regression, for aurora52

Comment 26

[Gerry Chang [:gchang]]

Track 51+ as perf regression.

Comment 27

[Gerry Chang [:gchang]]

Comment on attachment 8818754 [details] [diff] [review]
[Gecko 52 and older] Set MAXWORDLEN back to 100

Fix a perf regression. Beta51+. Should be in 51 beta 9.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6f0be357dd95

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f499cd347969

Comment 30

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Alice0775 White from comment #23)
> (In reply to Gerry Chang [:gchang] from comment #22)
> > Hi Alice, 
> > could you help verify if this issue is fixed as expected on the latest
> > Nightly build? Thanks!
> 
> I verified that the problem was fixed on latest m-c build[1].
> No longer reproduce the long delay and/or contextmenu problem with/without
> e10s.
> 
> [1]
> https://hg.mozilla.org/mozilla-central/rev/
> c750a7de1194d71f2d2ef73f6a919d26b9640cae
> Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0
> ID:20161215051900

Setting corresponding bug status and status flag to verified.

Comment 31

[Dimitrij Mijoski]

I published Hunspell v1.6.0 and the patch made here is applied in upstream, MAXWORDLEN is reduced to 100 in upstream too.

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8818754 [details] [diff] [review]
[Gecko 52 and older] Set MAXWORDLEN back to 100

Fix has been verified on multiple channels, seems very low risk, let's take it in ESR45.7

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/576f03e362c5