345428 - No spellcheck on focus out

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Pro]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060710 Firefox/2.0b1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060710 Firefox/2.0b1

No spell checking occurs on last typed word if:
1. there are no non-word characters after it
2. textarea lost focus

Reproducible: Always

Steps to Reproduce:
1. Type some misspelled word into textarea (e.g. exxample)
2. Do not type any character after it and do not move cursor
3. Click outside the textarea

Actual Results:  
The misspelled word doesn't get underlined

Expected Results:  
The misspelled word should get underlined

Comment 1

[Martijn Wargers (dead)]

This reminds me of the discussion in bug 342511.

Comment 2

[Pro]

Still unfixed as of RC1

Comment 4

[Phil Ringnalda (:philor)]

And as Seth noted, also while tabbing out of single-line textboxes, like a Thunderbird compose subject.

Comment 5

[(not reading, please use [e-mail] instead)]

I was hoping to solve this by simply making mozInlineSpellChecker implement the nsIDOMUIListener (for FocusOut) or nsIDOMFocusListener (for Blur), and forcing a spelling corretion, but that didn't work.  (I didn't get called when I expected.)

for tab, I was able to add a case for nsIDOMKeyEvent::DOM_VK_TAB to mozInlineSpellChecker::KeyPress() to get called when tabbing, but implementing a blur listener seems like the right fix.

Comment 6

[Martijn Wargers (dead)]

Attachment: patch

I copied this code more or less from nsFormFillController.cpp and this works, but when I reload a bunch of textareas, I get a heap corruption error in my vc debugger, I have no idea why. I don't get this in mingw.

Comment 7

[Martijn Wargers (dead)]

Attachment: bunch of textareas

Comment 8

[(not reading, please use [e-mail] instead)]

martin, thanks for working on this!

a couple questions

1) why is this line necessary?

+NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsIDOMEventListener, nsIDOMFocusListener)

I don't think you need it, as we have:

NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsIDOMEventListener, nsIDOMKeyListener)

2)

+  /*BEGIN implementations of focus event handler interface*/
+  NS_IMETHOD Focus(nsIDOMEvent* aEvent);
+  NS_IMETHOD Blur(nsIDOMEvent* aEvent);

Following the style of the existing code, I think you want add:

 /*END implementations of focus event handler interface*/

3) I've made the corrections and I'm running locally, and I'm not seeing a heap corruption error

Comment 9

[Martijn Wargers (dead)]

Attachment: patchv2

Ok, I guess something goes very wrong when I make changes in my vc build :(

I made the changes you suggested here.

Comment 10

[(not reading, please use [e-mail] instead)]

martijn, thanks for making those changes.

1) we need to implement a stub for mozInlineSpellChecker::Focus(), but do need to register (and unregister) the focus event listener?  I think we can remove these lines of code:

+  target->AddEventListener(NS_LITERAL_STRING("focus"),
+                           static_cast<nsIDOMFocusListener *>(this),
+                           PR_TRUE);

and

+  target->RemoveEventListener(NS_LITERAL_STRING("focus"),
+                              static_cast<nsIDOMFocusListener *>(this),
+                              PR_TRUE);

2) 

can you add a comment to mozInlineSpellChecker::Blur() about why we call HandleNavigationEvent() on blur?  Or even just do:

HandleNavigationEvent(aKeyEvent, PR_TRUE /* force a spelling correction */);

3) I'm nervous about checking this in when we don't know why you are getting a heap corruption error, and I'm not.  to reproduce it, I should just reload https://bugzilla.mozilla.org/attachment.cgi?id=280585 (if so, I'm not seeing anything when I do that.)

Are there any good tools for tracking down heap corruption (google tells me about pageheap.exe and gflags.exe, see http://support.microsoft.com/kb/286470, but I've never used them.)

Comment 11

[Martijn Wargers (dead)]

Well, I saw some weird crashes earlier with my vc build, see bug 394534, comment 7, so I'm getting pretty sure there is something wrong with it.

Comment 12

[(not reading, please use [e-mail] instead)]

Martijn, I'd really like to see this land on the trunk (and maybe make it's way onto the MOZILLA_1_8_BRANCH, as I'm sure I'm not the only misspeller who would benefit from it.)

but, we should be confident that we're not introducing a heap corruption problem.  I assume a full clobber / rebuild didn't help?

Can you make the two changes in comment #10 and apply one more version of the fix (so I can r=)

Comment 13

[(not reading, please use [e-mail] instead)]

Comment on attachment 280756 [details] [diff] [review]
patchv2

r=sspitzer, once you make the changes in comment #10.

Is there anyone else that should review this?

Comment 14

[Martijn Wargers (dead)]

Attachment: patchv3

This is with the suggestion of comment 10.
I'll also ask Brett for review, since he is the owner of spellchecker, afaik.

I'll rebuild my vc build from scratch to see if I still get this heap corruption, but I'm pretty sure something is wrong with my vc tree.

Comment 15

[Martijn Wargers (dead)]

Ok, my problems with the vc build turned out to be that I had ac_add_options --disable-auto-deps set, so that should not be a concern anymore.

Comment 16

[Brett Wilson]

Comment on attachment 280893 [details] [diff] [review]
patchv3

Just to be sure, being a listener doesn't make the spellchecking object get leaked, right? You should make sure the destructor gets called still.

Otherwise, looks fine.

Comment 17

[Martijn Wargers (dead)]

How do I test that the destructor of the spellchecking object gets called still with the patch?

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #17)
> How do I test that the destructor of the spellchecking object gets called still
> with the patch?

By setting a breakpoint with a debugger, or even just a simple printf(), and then making sure it gets called when the object should be destroyed.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/spellcheck/src/mozInlineSpellChecker.cpp&rev=1.39#531

Comment 19

[Martijn Wargers (dead)]

Ok, the destructors seems to get called still, afaict.

Comment 20

[Martijn Wargers (dead)]

Checking in mozInlineSpellChecker.h;
/cvsroot/mozilla/extensions/spellcheck/src/mozInlineSpellChecker.h,v  <--  mozIn
lineSpellChecker.h
new revision: 1.12; previous revision: 1.11
done
Checking in mozInlineSpellChecker.cpp;
/cvsroot/mozilla/extensions/spellcheck/src/mozInlineSpellChecker.cpp,v  <--  moz
InlineSpellChecker.cpp
new revision: 1.40; previous revision: 1.39
done

Checked in.
Thanks for the help.

Comment 21

[(not reading, please use [e-mail] instead)]

Martijn, thanks for fixing this.  If it bakes and verifies on the trunk, I think this would be a good fix for the MOZILLA_1_8_BRANCH (for firefox / thunderbird 2.0.0.x)

Comment 22

[Robert Sayre]

This is leaking on Mac and Linux leak tinderboxes

Comment 23

[Daniel Veditz [:dveditz]]

Backed out: memory leaks went from ~1.5K to ~6.6K

Comment 24

[David Baron :dbaron:]

Er, RLk went from 0 (Linux) / 4.81K (Mac) to 337K (Linux) / 323K (Mac).  Lk went from 1.5M (Linux) / 1.04M (Mac) to 6.5M (Linux) / 3.9M (Mac).

Comment 25

[Martijn Wargers (dead)]

I'm sorry, I only checked for greenness on tinderbox.
So if I understand correctly, windows didn't leak?
Then it's impossible (at least for now) for me to check how/where/why it's leaking.

Comment 26

[Robert Sayre]

No, we don't have a windows leak box set up. It probably leaks on windows too.

Comment 27

[Daniel Veditz [:dveditz]]

Meant "M" not "K" in comment 23. I was using the bigger Linux numbers to emphasize this was not a trivial or borderline leak.

I really doubt mscott is going to take this, reassigning to nobody so no one has any false hopes or gets discouraged from taking it on.

Comment 28

[Martijn Wargers (dead)]

Ok, apparently, with the patch, I leak 2 windows and 2 documents after using form autocomplete.
Leaked 2 out of 7 DOM Windows
Leaked 2 out of 45 documents
I don't get this leak with the patch backed out.
It doesn't matter if the text input in question has spell check enabled or not.

Comment 29

[Martijn Wargers (dead)]

Never mind, scratch comment 28, now I suddenly don't leak anymore at all that way, with or without the patch.

Comment 30

[Martijn Wargers (dead)]

Attachment: bloatview

This are the numbers I get when loading this url:
http://www.mozilla.org/newlayout/samples/test8.html
The first numbers are without the patch, the last numbers are with the patch.

So I guess Total Leaked is the number where Rlk is coming from?

Before patch:
Totol Leaked    4140 bytes
After patch:
Total Leaked    4196 bytes

Comment 31

[Martijn Wargers (dead)]

Sorry, probably I did something wrong after patch in comment 30.
I think I quite reproducible got appr. 361679 bytes for Total Leaked with the patch.

Comment 32

[Martijn Wargers (dead)]

Attachment: patchv4

Ok, this seems to work and afaict, doesn't cause memory leaks.

This is basically following the code here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/editor/libeditor/base/nsEditor.cpp&rev=1.501#354

Comment 33

[Martijn Wargers (dead)]

Attachment: leak log with and without the patch

These are the numbers I get with and without the patch.
I don't seen an increase in leaks with this, afaict, but I'm not sure how this correlates to the Rlk number.

Comment 34

[Martijn Wargers (dead)]

Comment on attachment 281829 [details] [diff] [review]
patchv4

Olli, maybe you can review this and tell if this is a valid way of calling event listeners.

Comment 35

[Martijn Wargers (dead)]

Comment on attachment 281829 [details] [diff] [review]
patchv4

I promise I'll take a good look at leak numbers this time if I can check this in.

Comment 36

[Martijn Wargers (dead)]

Checking in Makefile.in;
/cvsroot/mozilla/extensions/spellcheck/src/Makefile.in,v  <--  Makefile.in
new revision: 1.16; previous revision: 1.15
done
Checking in mozInlineSpellChecker.h;
/cvsroot/mozilla/extensions/spellcheck/src/mozInlineSpellChecker.h,v  <--  mozIn
lineSpellChecker.h
new revision: 1.14; previous revision: 1.13
done
Checking in mozInlineSpellChecker.cpp;
/cvsroot/mozilla/extensions/spellcheck/src/mozInlineSpellChecker.cpp,v  <--  moz
InlineSpellChecker.cpp
new revision: 1.43; previous revision: 1.42
done

Checked in on trunk.

Comment 37

[Martijn Wargers (dead)]

Before patch checked in:
Linux fxdbug-linux-tbox Dep: RLk:8.00B Lk:6.99MB
MacOSX Darwin 8.8.4 bm-xserve11 Dep Debug + Leak Test: RLk:12.0B Lk:962KB

After patch checked in:
Linux fxdbug-linux-tbox Dep: RLk:8.00B Lk:7.02MB
MacOSX Darwin 8.8.4 bm-xserve11 Dep Debug + Leak Test: RLk:12.0B Lk:962KB

So this doesn't seem to have caused any extra leaks with current tests, afaict.

Comment 38

[Dave Townsend [:mossop]]

This has been backed out to investigate the Tp regression talos is showing

Comment 39

[Dave Townsend [:mossop]]

The tree has been reopened but I am unable to get to re-checking things in. The numbers are a little unclear as to whether this was a cause of some Tp regression so please exercise care when checking back in.

Comment 40

[Martijn Wargers (dead)]

This was checked in at 2007-09-25 04:20:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-09-25+03&maxdate=2007-09-25+20&cvsroot=%2Fcvsroot

Afaics, the Tp regression on Talos started just before 18:00:00 25 Sep 2007:
(qm-pxp01 tp_Working Set_avg)
http://graphs.mozilla.org/graph.html#spst=range&spss=1190610264.7428572&spse=1190813684.9828572&spstart=1180657203&spend=1190828215&bpst=Cursor&bpstart=1190610264.7428572&bpend=1190813684.9828572&m1tid=9&m1bl=0&m1avg=0
(qm-pxp01 tp_Private Bytes_avg)
http://graphs.mozilla.org/graph.html#spst=range&spss=1190711974.862857&spse=1190813684.9828572&spstart=1180657203&spend=1190828215&bpst=Cursor&bpstart=1190711974.862857&bpend=1190813684.9828572&m1tid=8&m1bl=0&m1avg=0
Other numbers from talos don't seem useful or show a regression.

Am I looking at the right data here?
Is there a time difference between talos and bonsai?
Because at the time when I checked in, there wasn't a talos perf regression, afaics.

Comment 41

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #40)
> Am I looking at the right data here?
> Is there a time difference between talos and bonsai?

From the discussion yesterday in #developers, I think there are two problems with the Talos numbers:
1) The graphs page seems to display the data using your local time, rather than PDT (which is what Bonsai uses)
2) The start times reported to tinderbox "test start", not "checkout start". When the test starts, it pulls a build from the FTP server, which is the last built "nightly" build.

Comment 42

[Martijn Wargers (dead)]

Ok, thanks Gavin.
Ben Hearsum also explained me what you're explaining in point 2.
I'll try to reland this tomorrow, and I'll try to keep an eye on the talos numbers.

Comment 43

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #41)
> 1) The graphs page seems to display the data using your local time

I filed that as bug 397683.

> 2) The start times reported to tinderbox "test start", not "checkout start".

That's bug 396133.

Comment 44

[Martijn Wargers (dead)]

Checking in Makefile.in;
/cvsroot/mozilla/extensions/spellcheck/src/Makefile.in,v  <--  Makefile.in
new revision: 1.18; previous revision: 1.17
done
Checking in mozInlineSpellChecker.h;
/cvsroot/mozilla/extensions/spellcheck/src/mozInlineSpellChecker.h,v  <--  mozIn
lineSpellChecker.h
new revision: 1.16; previous revision: 1.15
done
Checking in mozInlineSpellChecker.cpp;
/cvsroot/mozilla/extensions/spellcheck/src/mozInlineSpellChecker.cpp,v  <--  moz
InlineSpellChecker.cpp
new revision: 1.45; previous revision: 1.44
done

Patchv4 checked in again.

Comment 45

[Scott MacGregor]

Martijn, this seems like a pretty useful fix for the spell checker for Fx and Tb, should we consider it for the branch? 

Comment 46

[Martijn Wargers (dead)]

I guess so, if you really think this is a major problem that needs to be fixed asap.

Comment 47

[Daniel Veditz [:dveditz]]

Not blocking a branch release, but will consider approval requests. Seems like a largish patch though, what's the risk like?