Bug 95436 - [GTK][EFL] 'dictIter' iterator is not initialized for an inner loop
Summary: [GTK][EFL] 'dictIter' iterator is not initialized for an inner loop
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P2 Normal
Assignee: Grzegorz Czajkowski
URL:
Keywords:
Depends on:
Blocks: 91854
  Show dependency treegraph
 
Reported: 2012-08-30 01:44 PDT by Grzegorz Czajkowski
Modified: 2012-09-03 01:15 PDT (History)
7 users (show)

See Also:


Attachments
initialize iteraor, add test to TestExpectations (3.94 KB, patch)
2012-08-30 06:49 PDT, Grzegorz Czajkowski
gns: review+
Details | Formatted Diff | Diff
updated patch (4.15 KB, patch)
2012-09-03 00:21 PDT, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Grzegorz Czajkowski 2012-08-30 01:44:53 PDT
Hi,

While testing the spelling implementation of WebKit-EFL I found out an interesting bug in the shared code (WebCore/platform/text/enchant/TextChecker.cpp)

It's connected with editing/spelling/spelling-backspace-between-lines.html. This test has been added here https://bugs.webkit.org/show_bug.cgi?id=41423. It looks like Chromium port had similar issue, already resolved https://bugs.webkit.org/show_bug.cgi?id=45438

This bug is not connected with Enchant and latest changes at all, rather with no possibilities to mark misspelled words (location and length) in checkSpellingOfString method which may take more than one word (as spelling-backspace-between-lines.html does).

Let me explain step by step why spelling-backspace-between-lines.html passes in the present implementation for both WebKit-Gtk and WebKit-EFL.

We've included English dictionary (size of 'm_enchantDictionaries' vector is 1) and 'checkSpellingOfString' method is called for "OK zz OK" string.

'checkSpellingOfString' sets 'dictIter' at the beginning of method! and starts to go through each word:

Vector<EnchantDict*>::const_iterator dictIter = m_enchantDictionaries.begin();   

for (size_t i = 0; i < numberOfCharacters + 1; i++) {        // iterate through the words                 

....

    for (; dictIter != m_enchantDictionaries.end(); ++dictIter) {   
        if (enchant_dict_check(*dictIter, cstart, numberOfBytes)) {          
            misspellingLocation = start;                            
            misspellingLength = wordLength;                                  
         } else {                                                             
           // Stop checking, this word is ok in at least one dict. 
           misspellingLocation = -1;                                        
           misspellingLength = 0;                                  
           break;
         }                                                                    
     }

} 

1) For first word 'OK'.
   We go into an inner loop and we spelling is checked. The word is fine so its location and length is saved to -1 and 0, then we break the loop, 'dictIter' is not increased!

2) For second word 'zz'.
   We go into an inner loop, 'dictIter' still points on the beginning of the vector. The word is wrong - misspelled location is updated to 3 and length to 2 for "OK zz OK" string. The inner loop increases 'dictIter'.

3) For third word 'OK'
  We go into an inner loop and we check whether 'dictIter' doesn't bound out of the vector. In this case - YES. We do not go to the loop here!

Finally 'checkSpellingOfString' returns misspelled location and length from the the previous iteration of the inner loop (3,2). What is the most surprising that exactly spelling-backspace-between-lines.html expects!

My proposals of resolving this bug is to initialize 'dictIter' at the beginning of the loop and:
1) temporally add spelling-backspace-between-lines.html to TestExpectations.
2) find a new way how to save misspelled location and length for multiple words for example "OK zz OK zz" (3,2) and (9,2)
Comment 1 Grzegorz Czajkowski 2012-08-30 06:49:31 PDT
Created attachment 161459 [details]
initialize iteraor, add test to TestExpectations
Comment 2 Gustavo Noronha (kov) 2012-08-31 07:13:01 PDT
Comment on attachment 161459 [details]
initialize iteraor, add test to TestExpectations

View in context: https://bugs.webkit.org/attachment.cgi?id=161459&action=review

r- for the test expectations change

> LayoutTests/platform/gtk/TestExpectations:1025
> +// TextCheckerEnchant::checkSpellingOfString doesn't return misspelled location and length for multiple words.
> +BUGWKGTK : editing/spelling/spelling-backspace-between-lines.html = TEXT
> +

I don't understand why you're adding this, this test seems to run and pass on our bot:

06:57:30.535 22495 worker/22 editing/spelling/spelling-backspace-between-lines.html passed

> Source/WebCore/ChangeLog:10
> +        dictionaries ('dictIter') whether the word exists in at lest one dictionary.

I'd remove 'whether the word exists...', that's better explained in the paragraph bellow.

> Source/WebCore/platform/text/enchant/TextCheckerEnchant.cpp:102
> +            Vector<EnchantDict*>::const_iterator dictIter = m_enchantDictionaries.begin();

Makes sense, can we declare this inside the for? Would be more idiomatic I think.
Comment 3 Grzegorz Czajkowski 2012-08-31 07:20:32 PDT
(In reply to comment #2)
> (From update of attachment 161459 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161459&action=review
> 
> r- for the test expectations change
> 
> > LayoutTests/platform/gtk/TestExpectations:1025
> > +// TextCheckerEnchant::checkSpellingOfString doesn't return misspelled location and length for multiple words.
> > +BUGWKGTK : editing/spelling/spelling-backspace-between-lines.html = TEXT
> > +
> 
> I don't understand why you're adding this, this test seems to run and pass on our bot:

Thanks for review.
Believe or not this bug (that iterator is not initialized) makes that the test passes :) See the first comment, there you can find more details.
Comment 4 Gustavo Noronha (kov) 2012-08-31 07:24:35 PDT
Comment on attachment 161459 [details]
initialize iteraor, add test to TestExpectations

View in context: https://bugs.webkit.org/attachment.cgi?id=161459&action=review

>>> LayoutTests/platform/gtk/TestExpectations:1025
>>> +
>> 
>> I don't understand why you're adding this, this test seems to run and pass on our bot:
>> 
>> 06:57:30.535 22495 worker/22 editing/spelling/spelling-backspace-between-lines.html passed
> 
> Thanks for review.
> Believe or not this bug (that iterator is not initialized) makes that the test passes :) See the first comment, there you can find more details.

Aha, thanks, sorry I didn't read the first comment.
Comment 5 Grzegorz Czajkowski 2012-09-03 00:16:40 PDT
Comment on attachment 161459 [details]
initialize iteraor, add test to TestExpectations

View in context: https://bugs.webkit.org/attachment.cgi?id=161459&action=review

>> Source/WebCore/ChangeLog:10
>> +        dictionaries ('dictIter') whether the word exists in at lest one dictionary.
> 
> I'd remove 'whether the word exists...', that's better explained in the paragraph bellow.

Done.

>> Source/WebCore/platform/text/enchant/TextCheckerEnchant.cpp:102
>> +            Vector<EnchantDict*>::const_iterator dictIter = m_enchantDictionaries.begin();
> 
> Makes sense, can we declare this inside the for? Would be more idiomatic I think.

Sure. Done.
Comment 6 Grzegorz Czajkowski 2012-09-03 00:21:03 PDT
Created attachment 161868 [details]
updated patch
Comment 7 WebKit Review Bot 2012-09-03 01:15:54 PDT
Comment on attachment 161868 [details]
updated patch

Clearing flags on attachment: 161868

Committed r127404: <http://trac.webkit.org/changeset/127404>
Comment 8 WebKit Review Bot 2012-09-03 01:15:58 PDT
All reviewed patches have been landed.  Closing bug.