Closed Bug 345428 Opened 16 years ago Closed 15 years ago

No spellcheck on focus out

Categories

(Core :: Spelling checker, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: hyrurg, Assigned: martijn.martijn)

References

Details

Attachments

(4 files, 3 obsolete files)

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
Version: unspecified → 2.0 Branch
This reminds me of the discussion in bug 342511.
Assignee: nobody → mscott
Component: General → Spelling checker
Product: Firefox → Core
QA Contact: general → spelling-checker
Version: 2.0 Branch → Trunk
Still unfixed as of RC1
Duplicate of this bug: 395730
And as Seth noted, also while tabbing out of single-line textboxes, like a Thunderbird compose subject.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Attached patch patch (obsolete) — Splinter Review
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.
Attached file bunch of textareas
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
Attached patch patchv2 (obsolete) — Splinter Review
Ok, I guess something goes very wrong when I make changes in my vc build :(

I made the changes you suggested here.
Attachment #280756 - Flags: review?(sspitzer)
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.)
Assignee: mscott → martijn.martijn
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.
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 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?
Attachment #280756 - Flags: review?(sspitzer) → review+
Attached patch patchv3 (obsolete) — Splinter Review
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.
Attachment #280893 - Flags: review?(brettw)
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 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.
Attachment #280893 - Flags: review?(brettw) → review+
How do I test that the destructor of the spellchecking object gets called still with the patch?
(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
Ok, the destructors seems to get called still, afaict.
Attachment #280893 - Flags: approval1.9?
Attachment #280893 - Flags: approval1.9? → approval1.9+
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.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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)
This is leaking on Mac and Linux leak tinderboxes
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backed out: memory leaks went from ~1.5K to ~6.6K
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).
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.
Assignee: martijn.martijn → mscott
Status: REOPENED → NEW
No, we don't have a windows leak box set up. It probably leaks on windows too.
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.
Assignee: mscott → nobody
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.
Assignee: nobody → martijn.martijn
Never mind, scratch comment 28, now I suddenly don't leak anymore at all that way, with or without the patch.
Attached file 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
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.
Attached patch patchv4Splinter Review
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
Attachment #280584 - Attachment is obsolete: true
Attachment #280756 - Attachment is obsolete: true
Attachment #280893 - Attachment is obsolete: true
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.
Attachment #281829 - Attachment is patch: true
Attachment #281829 - Attachment mime type: application/octet-stream → text/plain
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.
Attachment #281829 - Flags: review?(Olli.Pettay)
Attachment #281829 - Flags: review?(Olli.Pettay) → review+
Attachment #281829 - Flags: review?(brettw)
Attachment #281829 - Flags: review?(brettw) → review+
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.
Attachment #281829 - Flags: approval1.9?
Attachment #281829 - Flags: approval1.9? → approval1.9+
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.
Status: NEW → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
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.
This has been backed out to investigate the Tp regression talos is showing
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
(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.
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.
(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.
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.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Martijn, this seems like a pretty useful fix for the spell checker for Fx and Tb, should we consider it for the branch? 
Flags: blocking1.8.1.9?
I guess so, if you really think this is a major problem that needs to be fixed asap.
Not blocking a branch release, but will consider approval requests. Seems like a largish patch though, what's the risk like?
Flags: blocking1.8.1.12? → wanted1.8.1.x+
You need to log in before you can comment on or make changes to this bug.