Continuation of bug 1100966: Open the attachment in Firefox. You will see: thiss is stilll not fixed Position behind the "stilll" and backspace to delete the third "l". Then type space. The "thiss" loses its red underline. It doesn't matter whether it's <tt> or some other font. Having a font element is essential for the bug. This affects Thunderbird, since the core editor is used to type messages.

Need info for you kind attention - as per bug 1100966 comment #29 ;-)

Sorry I have no time to devote to this right now. It would be great if you can help debug this if you have the tme. The same debugging techniques as the previous bug can help you here as well.

OK, after the 23rd of April. Debugging is no fun on a small travel laptop.

I remembered that this is debugged with printing out stuff, since debugging is impossible in the debugger in this event driven stuff. I still had the code. Here is the debug: When loading the page: [3816] WARNING: NS_ENSURE_TRUE(rootContent) failed: file d:\mozilla-source\mozilla-central\editor\composer\nsEditorSpellCheck.cpp, line 697 info: 0, warning: 0, error: 2 Done === Entering mozInlineSpellChecker::CleanupRangesInSelection -- count: 0 === Leaving mozInlineSpellChecker::CleanupRangesInSelection === Entering mozInlineSpellChecker::DoSpellCheck -- originalRangeCount: 0 -- SetEnd,SetPosition: done -- before while loop ->Got word "thiss" ->Got word "is" ->Got word "stilll" ->Got word "not" ->Got word "fixed" === Leaving mozInlineSpellChecker::DoSpellCheck when clicking behind "stilll", two red underlines appear === Entering mozInlineSpellChecker::CleanupRangesInSelection -- count: 0 === Leaving mozInlineSpellChecker::CleanupRangesInSelection === Entering mozInlineSpellChecker::DoSpellCheck -- originalRangeCount: 0 -- SetEnd,SetPosition: done -- before while loop ->Got word "thiss" ->Got word "is" ->Got word "stilll" ->Got word "not" ->Got word "fixed" === Leaving mozInlineSpellChecker::DoSpellCheck when deleting the third "l" with a backspace === Entering mozInlineSpellChecker::CleanupRangesInSelection -- count: 2 === Leaving mozInlineSpellChecker::CleanupRangesInSelection === Entering mozInlineSpellChecker::DoSpellCheck -- originalRangeCount: 2 -- SetEnd,SetPosition: done -- before while loop ->Got word "is" -- for (...) RemoveRange: done ->Got word "still" === Entering mozInlineSpellChecker::RemoveRange -- decremented mNumWordsInSpellSelection, current value 1 === Leaving mozInlineSpellChecker::RemoveRange with error code 0 -- for (...) RemoveRange: done === Leaving mozInlineSpellChecker::DoSpellCheck when typing space, red underline under "thiss" is lost === Entering mozInlineSpellChecker::CleanupRangesInSelection -- count: 1 -- calling RemoveRange with loop index 0 === Entering mozInlineSpellChecker::RemoveRange -- decremented mNumWordsInSpellSelection, current value 0 === Leaving mozInlineSpellChecker::RemoveRange with error code 0 === Leaving mozInlineSpellChecker::CleanupRangesInSelection === Entering mozInlineSpellChecker::DoSpellCheck -- originalRangeCount: 0 -- SetEnd,SetPosition: done -- before while loop ->Got word "is" ->Got word "still" === Leaving mozInlineSpellChecker::DoSpellCheck Later on === Entering mozInlineSpellChecker::CleanupRangesInSelection -- count: 0 === Leaving mozInlineSpellChecker::CleanupRangesInSelection === Entering mozInlineSpellChecker::DoSpellCheck === Leaving mozInlineSpellChecker::DoSpellCheck early due to aStatus->mRange->Collapsed() My comment: After the space key is pressed, a call to CleanupRangesInSelection is happening. In there, the remaining valid range corresponding to "thiss" is erroneously removed: -- count: 1 -- calling RemoveRange with loop index 0 === Entering mozInlineSpellChecker::RemoveRange -- decremented mNumWordsInSpellSelection, current value 0 which comes from: if (checkRange->Collapsed()) { printf("-- calling RemoveRange with loop index %d\n", index); RemoveRange(aSelection, checkRange); and printf("=== Entering mozInlineSpellChecker::RemoveRange\n"); aSpellCheckSelection->RemoveRange(*aRange, rv); if (!rv.Failed() && mNumWordsInSpellSelection) { mNumWordsInSpellSelection--; printf("-- decremented mNumWordsInSpellSelection, current value %d\n", mNumWordsInSpellSelection); I wonder which collapsed range it wants to remove, since one got removed before, and why it hits the still valid and non-collapsed range containing the "thiss". Obviously some processing corrupted the internals.

You're looking for the wrong thing, you should look for another place where the editor code is ripping out the selections from underneath the spell checker, just like the other bug. If I had to make a wild guess, I would suspect nsEditor::SplitNodeImpl, since it has the same broken logic, and we are splitting a text node here. You should be able to do exactly what I did in my patch there for this function and see if that fixes this issue.

Will do, thanks.

Comment on attachment 8593304 [details] mozInlineSpellChecker-with-debug-april-2015.zip, if you want to see where the debug is printed I did a cut & paste job, it fixes the problem, if there are no objections, I'll submit it to the try server.

Comment on attachment 8593513 [details] [diff] [review] Proposed patch, tests still missing Review of attachment 8593513 [details] [diff] [review]: ----------------------------------------------------------------- This is the right idea for a fix, but I don't think it's a good patch since it duplicates a lot of code that we have in JoinNodesImpl. Ideally you'd add two helper functions, for example SaveAllSelectionRanges, and RestoreAllSelectionRanges which both of these functions would call before and after modifying the DOM, but I haven't looked closely to see if the logic can be extracted very cleanly. At the very least, I suspect that refactoring the first half into SaveAllSelectionRanges should be possible. Can you please see if you can refactor things this way? Thanks!

The stuff you want to factor out is too different: In the join part you have this extra code: // If selection endpoint is between the nodes, remember it as being // in the one that is going away instead. This simplifies later selection // adjustment logic at end of this method. if (range.mStartNode) { if (range.mStartNode == parent && joinOffset < range.mStartOffset && range.mStartOffset <= keepOffset) { range.mStartNode = aNodeToJoin; range.mStartOffset = firstNodeLength; } if (range.mEndNode == parent && joinOffset < range.mEndOffset && range.mEndOffset <= keepOffset) { range.mEndNode = aNodeToJoin; range.mEndOffset = firstNodeLength; } } The only thing in common are about eight lines. Not worth the effort. The second part is also too different to factor out the common stuff: Join: if (range.mStartNode == aNodeToJoin) { range.mStartNode = aNodeToKeep; } else if (range.mStartNode == aNodeToKeep) { range.mStartOffset += firstNodeLength; } Split: if (range.mStartNode == &aExistingRightNode) { if (range.mStartOffset < aOffset) { range.mStartNode = &aNewLeftNode; } else { range.mStartOffset -= aOffset; } } Why make it extra complicated for the sake of saving a few common lines? OK, I did what you said, I looked, and my result is that it's now worth doing. I will go ahead with another copy&paste job and adapt your test from bug 1100966 to this new case.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e599d277f32 Too bad that also includes the fix to bug 1140105, but that won't make a difference. I haven't worked out how to submit a single patch, they told me to use: mc-try = ssh://mozilla@jorgk.com@hg.mozilla.org/try hg push -f -r tip mc-try

(In reply to Jorg K (at the beach until 22nd April) from comment #10) > The stuff you want to factor out is too different: OK, fair. Can you please set the review flag once this passes tests to avoid round-trips? My review queue is super long these days. :( Thanks!

Comment on attachment 8593539 [details] [diff] [review] Proposed patch, complete with test case Passed the Mochitests (no idea why M2 always fails).

Comment on attachment 8593539 [details] [diff] [review] Proposed patch, complete with test case The M2 failures are almost definitely caused by the patch. :-)

As I said in comment #11: This try run included the patch to bug 1140105 as well. The try run to this bug (see bug 1140105 comment #28) https://treeherder.mozilla.org/#/jobs?repo=try&revision=018f3fb1f578 most likely caused the M2 failures. So I pulled the other patch out and submitted this change again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8820dca180fe

Comment on attachment 8593758 [details] [diff] [review] Proposed patch, complete with test case (conflict with bug 1140105 removed) Here is the *clean* try run with *only* the patch from this current bug: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8820dca180fe The failure in M2 was caused by the accidentally included patch for bug 1140105 (where it had slipped the review). Refer to the other bug 1140105 for details.

Comment on attachment 8593758 [details] [diff] [review] Proposed patch, complete with test case (conflict with bug 1140105 removed) Review of attachment 8593758 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, but minusing mainly for the comment about the test. ::: editor/libeditor/nsEditor.cpp @@ +2609,5 @@ > nsEditor::SplitNodeImpl(nsIContent& aExistingRightNode, > int32_t aOffset, > nsIContent& aNewLeftNode) > { > + Nit: please remove the unneeded blank line. @@ +2627,5 @@ > + for (uint32_t j = 0; j < range.mSelection->RangeCount(); ++j) { > + nsRefPtr<nsRange> r = range.mSelection->GetRangeAt(j); > + if (!r->IsPositioned()) { > + continue; > + } You didn't need to copy this condition. This is what GetStartNodeAndOffset and GetEndNodeAndOffset checked, and those functions were used in JoinNodesImpl. They are not used here, so you're changing the semantics by this check (although I think it's harmless since these ranges should always be positioned.) I think it's probably better to convert both this check and the similar one in JoinNodesImpl to |MOZ_ASSERT(r->IsPositioned());|. ::: editor/libeditor/tests/test_bug1154791.html @@ +37,5 @@ > + synthesizeKey(" ", {}); > + > + var sel = getSpellCheckSelection(); > + is(sel.rangeCount, 1, "We should have one misspelled word"); > + is(sel.getRangeAt(0), "thiss", "Correct misspelled word"); Please use a test case with two misspelled words, otherwise this test wouldn't test whether we can restore multiple ranges successfully. My original patch for the previous bug was buggy and didn't handle this case correctly, and I didn't realize that since my test was originally examining only one misspelled range.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #17) > > + for (uint32_t j = 0; j < range.mSelection->RangeCount(); ++j) { > > + nsRefPtr<nsRange> r = range.mSelection->GetRangeAt(j); > > + if (!r->IsPositioned()) { > > + continue; > > + } > > You didn't need to copy this condition. This is what GetStartNodeAndOffset > and GetEndNodeAndOffset checked, and those functions were used in > JoinNodesImpl. They are not used here, so you're changing the semantics by > this check (although I think it's harmless since these ranges should always > be positioned.) > > I think it's probably better to convert both this check and the similar one > in JoinNodesImpl to |MOZ_ASSERT(r->IsPositioned());|. Your instructions are not clear: First you said "no need to copy" they you said "convert *both* this check and the similar one ...". Sadly, a contradiction. Please choose one: 1) remove here and convert the other 1a) remove here and leave the other one as you originally had it 2) leave here and convert both 3) Other: please specify.

I said you _didn't_ need to copy. :-) Please add the assertions in both places as I asked in comment 17.

Here is the next and hopefully final round: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce581612c902 sadly with a few failures, but I guess that has nothing to do with us. All I changed in the code from the previous run was the MOZ_ASSERT(r->IsPositioned()); Frankly, English is not my native tongue despite living 16 years in Australia. "You didn't need to copy" can mean two things: "You didn't need to copy, but it's good that you did" or "You didn't need to copy, and please undo it". Looks like you meant the former (despite saying "you're changing the semantics"). Have a good weekend!

(In reply to Jorg K (at the beach until 22nd April) from comment #20) > Here is the next and hopefully final round: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce581612c902 > sadly with a few failures, but I guess that has nothing to do with us. All I > changed in the code from the previous run was the > MOZ_ASSERT(r->IsPositioned()); None of them are related to your push, you're good! :-) > Frankly, English is not my native tongue despite living 16 years in > Australia. "You didn't need to copy" can mean two things: "You didn't need > to copy, but it's good that you did" or "You didn't need to copy, and please > undo it". Looks like you meant the former (despite saying "you're changing > the semantics"). Haha, it's not my native language either. What I meant to say was "copying this part was not necessary for this fix, since the original code didn't call IsPositioned()." Sorry for the confusion! > Have a good weekend! You too!

Comment on attachment 8594206 [details] [diff] [review] Proposed patch, complete with test case (hopefully final version) Review of attachment 8594206 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, thanks a lot for your patch!

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9107041&repo=mozilla-inbound

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/45d726bed683

What's going on here?? 01:23:28 INFO - 1693 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug1154791.html | Correct misspelled word - got thiss, expected thiss 01:23:28 INFO - 1694 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug1154791.html | Correct misspelled word - got onee, expected onee The test says: is(sel.getRangeAt(0), "thiss", "Correct misspelled word"); is(sel.getRangeAt(1), "onee", "Correct misspelled word"); We got what we expected and the test fails? How good is that? Never failed on the try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce581612c902

@Aryeh: Ehsan seems to be very busy, can you tell me what's going on here, please.

I don't know without debugging the test, which might be hard if you can't reproduce locally. It could be the strings aren't the same but they look the same in the log output, like maybe there's trailing whitespace (although it looks like there's not). It's also theoretically possible that it's not actually caused by your commit, because test runs on inbound are coalesced, i.e., the test suite is run once for many pushes. The sherriffs do their best to back out the correct push, but mistakes do occasionally happen. It doesn't seem likely that anyone else changed the spellchecker at the same time, though. Unfortunately, I can't help much more than that without putting time into it, and I work only when on break from other things, which I no longer am. Next break starts July 27, so if you haven't figured it out by then I could try to take a look.

Carrying forward Ehsan's review+ Solved the riddle why the test failed: Test was: is(sel.getRangeAt(0), "thiss", "Correct misspelled word"); is(sel.getRangeAt(1), "onee", "Correct misspelled word"); After bug 949614 landed, this must now be: is(String(sel.getRangeAt(0)), "thiss", "Correct misspelled word"); is(String(sel.getRangeAt(1)), "onee", "Correct misspelled word"); Fixed up the test and it's ready to go again.

Works now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dcaa065a2ad

(In reply to Jorg K from comment #29) > Solved the riddle why the test failed: > Test was: > is(sel.getRangeAt(0), "thiss", "Correct misspelled word"); > is(sel.getRangeAt(1), "onee", "Correct misspelled word"); > > After bug 949614 landed, this must now be: > is(String(sel.getRangeAt(0)), "thiss", "Correct misspelled word"); > is(String(sel.getRangeAt(1)), "onee", "Correct misspelled word"); We recently switched the mochitest is() function from doing comparisons with == to doing them with ===. In JS, comparing using == can convert one side of the comparison to another type, but === compares more strictly and returns false if the two sides are not the same type. getRangeAt returns a Range object which is convertible to string, so before that change your patch was green on try, but you got unlucky and your patch landed after that change. That should explain the failure and why converting the Range object to a String directly fixed it