Bug 55989 – [GTK] Possible leaks after splitting TextCheckerClientEnchant.

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Ryuan Choi]

I removed g_free(ctext); in Bug 51587,
but I am missing to change it's type.

[Ryuan Choi]

Created attachment 85121 [details]
Patch

[Martin Robinson]

Comment on attachment 85121 [details]
Patch

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

Thanks for fixing this.

> Source/WebKit/gtk/WebCoreSupport/TextCheckerClientEnchant.cpp:81
> +    int utflen = g_utf8_strlen(ctext.get(), -1);

Please change ctext to utf8Text and utflen to utf8Length. I know that you didn't add this code, but utflen makes no sense at all. :)

[Ryuan Choi]

Created attachment 85131 [details]
simplechanges

[Ryuan Choi]

(In reply to comment #2)
> (From update of attachment 85121 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85121&action=review
> 
> Thanks for fixing this.
> 
> > Source/WebKit/gtk/WebCoreSupport/TextCheckerClientEnchant.cpp:81
> > +    int utflen = g_utf8_strlen(ctext.get(), -1);
> 
> Please change ctext to utf8Text and utflen to utf8Length. I know that you didn't add this code, but utflen makes no sense at all. :)

I changed as you mentioned.
Additionally I fixed some coding style and removed remained g_free.

[Ryuan Choi]

Created attachment 85132 [details]
simplechanges

[Collabora GTK+ EWS bot]

Attachment 85131 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8113347

[Martin Robinson]

Comment on attachment 85132 [details]
simplechanges

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

Thanks for continuing to clean this code up! I really appreciate it. I've one suggested change.

> Source/WebKit/gtk/WebCoreSupport/TextCheckerClientEnchant.cpp:113
> +                g_utf8_strncpy(word.get(), cstart, end - start);

We've already calculated the final offset here, why not do this:

CString word(cstart, bytes);
int result = enchant_dict_check(dict, word.data(), -1);

[Ryuan Choi]

Created attachment 85576 [details]
Patch

[Ryuan Choi]

(In reply to comment #7)
> (From update of attachment 85132 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85132&action=review
> 
> Thanks for continuing to clean this code up! I really appreciate it. I've one suggested change.
> 
> > Source/WebKit/gtk/WebCoreSupport/TextCheckerClientEnchant.cpp:113
> > +                g_utf8_strncpy(word.get(), cstart, end - start);
> 
> We've already calculated the final offset here, why not do this:
> 
> CString word(cstart, bytes);
> int result = enchant_dict_check(dict, word.data(), -1);

Personally, it's my pleasure to contribute something for WebKit and get your review.

I update patch as you mentioned.
Additionally, I thought that some logic will be moved out of nested for loop because it looks not changed in nested for loop.

[Martin Robinson]

Comment on attachment 85576 [details]
Patch

Thanks! Great cleanup.

[WebKit Commit Bot]

Comment on attachment 85576 [details]
Patch

Rejecting attachment 85576 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'build-..." exit_code: 2

Last 500 characters of output:
......................................
tables/mozilla_expected_failures/other ..
transforms ....
transforms/2d ............
transforms/3d/general .....
transforms/3d/hit-testing ....
transforms/3d/point-mapping ........
transitions .................
transitions/interrupted-accelerated-transition.html -> failed

Exiting early after 1 failures. 21676 tests run.
525.42s total testing time

21675 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
13 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8175073

[Ryuan Choi]

Hmm. I don't know why that test case was failed.

I tested in my PC, but It looks OK like below.

root@ryuan2-desktop:/workspace/gtk_webkit# WebKitBuild/Release/Programs/DumpRenderTree LayoutTests/transitions/interrupted-accelerated-transition.html
main frame - has 1 onunload handler(s)
PASS - "opacity" property for "box" element at 0.5s saw something close to: 0.5

#EOF


Can I know how to reproduce?

[Martin Robinson]

I'm pretty sure this patch is unrelated to that failure. Probaly the test is just flaky. I'll flip the bit again.

[WebKit Commit Bot]

Comment on attachment 85576 [details]
Patch

Clearing flags on attachment: 85576

Committed r81102: <http://trac.webkit.org/changeset/81102>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.