Bug 179981 – Fix dictionary leak in lookup, convert FindOptions to OptionSet, tweak code style nearby

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Darin Adler]

Fix dictionary leak in lookup, convert FindOptions to OptionSet, tweak code style nearby

[Darin Adler]

Created attachment 327512 [details]
Patch

[EWS Watchlist]

Attachment 327512 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/OptionSet.h:133:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WTF/wtf/OptionSet.h:138:  Missing spaces around |  [whitespace/operators] [3]
Total errors found: 2 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Created attachment 327520 [details]
Patch

[EWS Watchlist]

Attachment 327520 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/OptionSet.h:133:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WTF/wtf/OptionSet.h:138:  Missing spaces around |  [whitespace/operators] [3]
Total errors found: 2 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 327520 [details]
Patch

Attachment 327520 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5345274

New failing tests:
editing/text-iterator/findString.html
editing/text-iterator/find-string-on-flat-tree.html

[EWS Watchlist]

Created attachment 327523 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Darin Adler]

Created attachment 327555 [details]
Patch

[EWS Watchlist]

Attachment 327555 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/OptionSet.h:133:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WTF/wtf/OptionSet.h:138:  Missing spaces around |  [whitespace/operators] [3]
Total errors found: 2 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 327555 [details]
Patch

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

> Source/WTF/wtf/OptionSet.h:151
> +    constexpr friend OptionSet operator|(OptionSet lhs, OptionSet rhs)
> +    {
> +        return fromRaw(lhs.m_storage | rhs.m_storage);
> +    }
> +
> +    constexpr friend OptionSet operator|(OptionSet lhs, T rhs)
> +    {
> +        return lhs | OptionSet { rhs };
> +    }
> +
>      constexpr friend OptionSet operator-(OptionSet lhs, OptionSet rhs)
>      {
> -        return OptionSet::fromRaw(lhs.m_storage & ~rhs.m_storage);
> +        return fromRaw(lhs.m_storage & ~rhs.m_storage);
> +    }
> +
> +    constexpr friend OptionSet operator-(OptionSet lhs, T rhs)
> +    {
> +        return lhs - OptionSet { rhs };
>      }

It would be good to add test cases for these to the existing OptionSet tests in TestWebKitAPI.

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:111
> +    std::tuple<String, RetainPtr<PDFSelection>, RetainPtr<NSDictionary>> lookupTextAtLocation(const WebCore::FloatPoint&, WebHitTestResultData&) const;

I am surprised you don't have to #include <tuple>

[Darin Adler]

Comment on attachment 327555 [details]
Patch

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

>> Source/WTF/wtf/OptionSet.h:151
>>      }
> 
> It would be good to add test cases for these to the existing OptionSet tests in TestWebKitAPI.

I will do that. In a separate patch.

>> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:111
>> +    std::tuple<String, RetainPtr<PDFSelection>, RetainPtr<NSDictionary>> lookupTextAtLocation(const WebCore::FloatPoint&, WebHitTestResultData&) const;
> 
> I am surprised you don't have to #include <tuple>

Someone includes it I guess.

[Darin Adler]

Committed r225142: <https://trac.webkit.org/changeset/225142>

[Radar WebKit Bug Importer]

<rdar://problem/35685066>

[Matt Lewis]

This change has caused an API timeout on Sierra for the test: 
WKWebViewCandidateTests.ClickingInTextFieldDoesNotThrashCandidateVisibility

https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK2%20(Tests)/builds/4175/steps/run-api-tests/logs/stdio
https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK2%20(Tests)/builds/4175

[Tim Horton]

Comment on attachment 327555 [details]
Patch

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

> Source/WebCore/editing/mac/DictionaryLookup.mm:67
> +    NSDictionary *retainedOptions = nil;
> +    auto result = [classLULookupDefinitionModule tokenRangeForString:string range:range options:&retainedOptions];
> +    *options = adoptNS(retainedOptions);

What led you to this leak? What indication is there that options is not autoreleased?

[Darin Adler]

(In reply to Tim Horton from comment #14)
> What led you to this leak?

Reading the source code.

> What indication is there that options is not autoreleased?

I read the source code of the LULookupDefinitionModule. Maybe I misread it?

[Darin Adler]

(In reply to Darin Adler from comment #15)
> > What indication is there that options is not autoreleased?
> 
> I read the source code of the LULookupDefinitionModule. Maybe I misread it?

I did misread it! Oops, will fix right away.

[Darin Adler]

Reading the tokenRangeForString source code I saw a copy, but missed an autorelease later in the function.

[Darin Adler]

Comment on attachment 327555 [details]
Patch

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

>> Source/WebCore/editing/mac/DictionaryLookup.mm:67
>> +    *options = adoptNS(retainedOptions);
> 
> What led you to this leak? What indication is there that options is not autoreleased?

My bad, I misread the source code to this function. I will fix this right now.

[Darin Adler]

Patch too big to just land without review, opening new bug 180202 with my patch.

[Tim Horton]

(In reply to Darin Adler from comment #19)
> Patch too big to just land without review, opening new bug 180202 with my
> patch.

I already fixed this in https://trac.webkit.org/changeset/225196/webkit

[Tim Horton]

(In reply to Tim Horton from comment #20)
> (In reply to Darin Adler from comment #19)
> > Patch too big to just land without review, opening new bug 180202 with my
> > patch.
> 
> I already fixed this in https://trac.webkit.org/changeset/225196/webkit

Just somehow managed to not mention it here :(

[Darin Adler]

Oops, OK.