Bug 187422 - StringView operator==(char*) should check the length of the string
Summary: StringView operator==(char*) should check the length of the string
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks: 186761
  Show dependency treegraph
 
Reported: 2018-07-06 16:31 PDT by youenn fablet
Modified: 2018-07-09 23:15 PDT (History)
14 users (show)

See Also:


Attachments
Patch (6.22 KB, patch)
2018-07-06 16:35 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (7.60 KB, patch)
2018-07-06 16:59 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.52 MB, application/zip)
2018-07-06 18:22 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews100 for mac-sierra (2.55 MB, application/zip)
2018-07-06 18:48 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews115 for mac-sierra (3.54 MB, application/zip)
2018-07-06 19:11 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (6.79 MB, application/zip)
2018-07-06 19:36 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews114 for mac-sierra (3.03 MB, application/zip)
2018-07-06 20:56 PDT, EWS Watchlist
no flags Details
Patch (9.86 KB, patch)
2018-07-06 21:54 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (9.38 KB, patch)
2018-07-09 13:57 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2018-07-06 16:31:37 PDT
StringView operator==(char*) should check the length of the string
Comment 1 youenn fablet 2018-07-06 16:35:32 PDT
Created attachment 344482 [details]
Patch
Comment 2 EWS Watchlist 2018-07-06 16:36:40 PDT
Attachment 344482 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:305:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:306:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:307:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 youenn fablet 2018-07-06 16:59:51 PDT
Created attachment 344490 [details]
Patch
Comment 4 EWS Watchlist 2018-07-06 17:01:11 PDT
Attachment 344490 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:305:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:306:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:307:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 EWS Watchlist 2018-07-06 18:22:40 PDT
Comment on attachment 344490 [details]
Patch

Attachment 344490 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8462338

New failing tests:
imported/w3c/web-platform-tests/eventsource/format-field-parsing.htm
Comment 6 EWS Watchlist 2018-07-06 18:22:41 PDT
Created attachment 344496 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 7 EWS Watchlist 2018-07-06 18:48:10 PDT
Comment on attachment 344490 [details]
Patch

Attachment 344490 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8462574

New failing tests:
imported/w3c/web-platform-tests/eventsource/format-field-parsing.htm
media/video-fullscreen-reload-crash.html
Comment 8 EWS Watchlist 2018-07-06 18:48:11 PDT
Created attachment 344498 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 9 EWS Watchlist 2018-07-06 19:11:12 PDT
Comment on attachment 344490 [details]
Patch

Attachment 344490 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8462495

New failing tests:
imported/w3c/web-platform-tests/eventsource/format-field-parsing.htm
Comment 10 EWS Watchlist 2018-07-06 19:11:14 PDT
Created attachment 344501 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 11 EWS Watchlist 2018-07-06 19:36:12 PDT
Comment on attachment 344490 [details]
Patch

Attachment 344490 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8462694

New failing tests:
imported/w3c/web-platform-tests/eventsource/format-field-parsing.htm
Comment 12 EWS Watchlist 2018-07-06 19:36:14 PDT
Created attachment 344504 [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.13.4
Comment 13 EWS Watchlist 2018-07-06 20:56:04 PDT
Comment on attachment 344490 [details]
Patch

Attachment 344490 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8463204

New failing tests:
imported/w3c/web-platform-tests/eventsource/format-field-parsing.htm
Comment 14 EWS Watchlist 2018-07-06 20:56:05 PDT
Created attachment 344510 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 15 youenn fablet 2018-07-06 21:54:13 PDT
Created attachment 344515 [details]
Patch
Comment 16 EWS Watchlist 2018-07-06 21:55:46 PDT
Attachment 344515 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:305:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:306:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:307:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:311:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:312:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
Total errors found: 5 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Chris Dumez 2018-07-09 09:48:32 PDT
Comment on attachment 344515 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:312
> +    EXPECT_FALSE(a == "Hell");

Is this really the expected behavior, I would have personally expected this to be TRUE. Wouldn't strcmp() be a match here?
Comment 18 Saam Barati 2018-07-09 10:12:08 PDT
Comment on attachment 344515 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:310
> +    a = StringView { (const LChar*)test, 5 };

static_cast

>> Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:312
>> +    EXPECT_FALSE(a == "Hell");
> 
> Is this really the expected behavior, I would have personally expected this to be TRUE. Wouldn't strcmp() be a match here?

Why? Isn't the point here that our strings are allowed to have the null byte in them and they are meaningful bytes?
Comment 19 youenn fablet 2018-07-09 10:36:27 PDT
(In reply to Saam Barati from comment #18)
> Comment on attachment 344515 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344515&action=review
> 
> > Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:310
> > +    a = StringView { (const LChar*)test, 5 };
> 
> static_cast
> 
> >> Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:312
> >> +    EXPECT_FALSE(a == "Hell");
> > 
> > Is this really the expected behavior, I would have personally expected this to be TRUE. Wouldn't strcmp() be a match here?
> 
> Why? Isn't the point here that our strings are allowed to have the null byte
> in them and they are meaningful bytes?

That was also my reasoning.

It is true that there is some inconsistency with StringView case insensitive comparison.
In that case, strlen() is used for any const char* pointer.
That would mean that for "hell\0o", case insensitive check would check the first 4 bytes while for this patch, we would try to check the 6 first bytes.

Also, if we were to add back a StringView operator==(char*), we would probably use strlen() and a comparison of the same pointer depending on its type might end up with different results.

Maybe we should add an ASSERT there, something like ASSERT(length == strlen(b))
Or just use strlen() everywhere.
Comment 20 Saam Barati 2018-07-09 10:38:05 PDT
What does String do here?
Comment 21 youenn fablet 2018-07-09 12:22:14 PDT
auto test1 = "Hell\0";
a = StringView { (const LChar*)test1, 5 };

String test2 = a.toString();
EXPECT_FALSE(test2 == "Hell\0");
EXPECT_FALSE(test2 == "Hell");

String test3 = "Hello";
EXPECT_TRUE(test3 == "Hello\0");
EXPECT_TRUE(test3 == "Hello");

I believe String is doing as if using strlen(). It seems safer to align.
Comment 22 youenn fablet 2018-07-09 13:57:37 PDT
Created attachment 344611 [details]
Patch
Comment 23 youenn fablet 2018-07-09 13:59:07 PDT
I went the strlen() easy approach.
I guess we could try to optimize it here a bit but this seems more urgent to fix the correctness here.
Comment 24 EWS Watchlist 2018-07-09 14:02:06 PDT
Attachment 344611 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:305:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:306:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:307:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:311:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:312:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:315:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:316:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
Total errors found: 7 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 WebKit Commit Bot 2018-07-09 15:30:47 PDT
Comment on attachment 344611 [details]
Patch

Clearing flags on attachment: 344611

Committed r233660: <https://trac.webkit.org/changeset/233660>
Comment 26 WebKit Commit Bot 2018-07-09 15:30:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 27 Radar WebKit Bug Importer 2018-07-09 15:31:51 PDT
<rdar://problem/41993422>
Comment 28 Zan Dobersek 2018-07-09 23:15:36 PDT
Comment on attachment 344611 [details]
Patch

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

> Source/WTF/wtf/linux/MemoryFootprintLinux.cpp:75
> -                isAnonymous = pathString == "[heap]"_s || pathString.startsWith("[stack");
> +                isAnonymous = pathString == "[heap]" | pathString.startsWith("[stack");

This OR operation was I guess accidentally made bitwise. Reverted in r233677.
https://trac.webkit.org/changeset/233677/webkit