Summary: | Fix dictionary leak in lookup, convert FindOptions to OptionSet, tweak code style nearby | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||||
Component: | WebCore Misc. | Assignee: | Darin Adler <darin> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ews-watchlist, jlewis3, sam, thorton, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Darin Adler
2017-11-23 12:19:17 PST
Created attachment 327512 [details]
Patch
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.
Created attachment 327520 [details]
Patch
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.
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 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
Created attachment 327555 [details]
Patch
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.
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> 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. Committed r225142: <https://trac.webkit.org/changeset/225142> 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 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? (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? (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. Reading the tokenRangeForString source code I saw a copy, but missed an autorelease later in the function. 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. Patch too big to just land without review, opening new bug 180202 with my patch. (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 (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 :( Oops, OK. |