Summary: | [iOS] TestWebKitAPI.WebKit.WKHTTPCookieStoreWithoutProcessPool fails because cookies use different files with/without processpool | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryan Haddad <ryanhaddad> | ||||||||
Component: | New Bugs | Assignee: | Sihui Liu <sihui_liu> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, cdumez, commit-queue, ddkilzer, realdawei, sihui_liu, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | Other | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=144820 | ||||||||||
Attachments: |
|
Description
Ryan Haddad
2018-05-21 12:43:18 PDT
It looks like this started with https://trac.webkit.org/changeset/231999/webkit ASSERT_TRUE(cookies == "PersistentCookieName=CookieValue; SessionCookieName=CookieValue" || cookies == "SessionCookieName=CookieValue; PersistentCookieName=CookieValue"); We could try EXPECT_TRUE instead of ASSERT_TRUE here, to make sure the completionHandler gets called. However, I think this means the string comparison fails and the test is thus still flaky. Created attachment 340990 [details]
Patch
Comment on attachment 340990 [details]
Patch
r=me
Comment on attachment 340990 [details] Patch Clearing flags on attachment: 340990 Committed r232083: <https://trac.webkit.org/changeset/232083> All reviewed patches have been landed. Closing bug. The test is no longer crashing on iOS after this change, but now it is failing again: TestWebKitAPI.WebKit.WKHTTPCookieStoreWithoutProcessPool /Volumes/Data/slave/ios-simulator-11-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:381 Value of: cookies == "PersistentCookieName=CookieValue; SessionCookieName=CookieValue" || cookies == "SessionCookieName=CookieValue; PersistentCookieName=CookieValue" Actual: false Expected: true https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Release%20WK2%20%28Tests%29/builds/5186 Reopening to attach new patch. Created attachment 341206 [details]
Patch
The TestWebKitAPI.WebKit.WKHTTPCookieStoreWithoutProcessPool test is still failing on the iOS 11 Simulator WK2 tests. For example: <https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)/builds/5259> Ran 1590 tests of 1611 with 1589 successful ------------------------------ Test suite failed Failed TestWebKitAPI.WebKit.WKHTTPCookieStoreWithoutProcessPool /Volumes/Data/slave/ios-simulator-11-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:436 Value of: cookies == "PersistentCookieName=CookieValue; SessionCookieName=CookieValue" || cookies == "SessionCookieName=CookieValue; PersistentCookieName=CookieValue" Actual: false Expected: true Testing completed, Exit status: 3 (In reply to David Kilzer (:ddkilzer) from comment #11) > The TestWebKitAPI.WebKit.WKHTTPCookieStoreWithoutProcessPool test is still > failing on the iOS 11 Simulator WK2 tests. For example: > > <https://build.webkit.org/builders/ > Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)/builds/5259> > > Ran 1590 tests of 1611 with 1589 successful > ------------------------------ > Test suite failed > > Failed > > TestWebKitAPI.WebKit.WKHTTPCookieStoreWithoutProcessPool > > > /Volumes/Data/slave/ios-simulator-11-release/build/Tools/TestWebKitAPI/Tests/ > WebKitCocoa/WKHTTPCookieStore.mm:436 > Value of: cookies == "PersistentCookieName=CookieValue; > SessionCookieName=CookieValue" || cookies == "SessionCookieName=CookieValue; > PersistentCookieName=CookieValue" > Actual: false > Expected: true > > > Testing completed, Exit status: 3 Oops! Didn't see Comment #8 before posting this. Comment on attachment 341206 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341206&action=review r=me, but consider making changes in feedback. > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:380 > - auto cookies = String(message.UTF8String); > - EXPECT_TRUE(cookies == "PersistentCookieName=CookieValue; SessionCookieName=CookieValue" || cookies == "SessionCookieName=CookieValue; PersistentCookieName=CookieValue"); > + EXPECT_STREQ("PersistentCookieName=CookieValue,SessionCookieName=CookieValue", message.UTF8String); Much better to use EXPECT_STREQ() here so we can see how the strings differ when they fail! Reminder: The expected value will change from using a comma to a semi-colon if #2 is changed below. > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:401 > - NSString *alertCookieHTML = @"<script>alert(document.cookie);</script>"; > + NSString *alertCookieHTML = @"<script>var cookies = document.cookie.split(';'); for (let i = 0; i < cookies.length; i ++) {cookies[i] = cookies[i].trim();} cookies.sort(); alert(cookies);</script>"; This is okay, but two things: 1. There should be spaces around the '{' and '}' brackets in the inline JavaScript. 2. It looks like alert(cookies) uses a comma to separate cookies; I would prefer you use join(';') so the list still looks cookie-like. NSString *alertCookieHTML = @"<script>var cookies = document.cookie.split(';'); for (let i = 0; i < cookies.length; i ++) { cookies[i] = cookies[i].trim(); } cookies.sort(); alert(cookies.join(';'));</script>"; Created attachment 341464 [details]
Patch
Comment on attachment 341464 [details]
Patch
r=me
Any reason not to land this? It would be good to get this test passing on the bots. (In reply to Ryan Haddad from comment #16) > Any reason not to land this? It would be good to get this test passing on > the bots. Looking at the patch, I do not think it will fix the flakiness/failure on the bots. It will merely provide more useful output when failing. Comment on attachment 341464 [details] Patch Clearing flags on attachment: 341464 Committed r232357: <https://trac.webkit.org/changeset/232357> All reviewed patches have been landed. Closing bug. Here is the failure output now: TestWebKitAPI.WebKit.WKHTTPCookieStoreWithoutProcessPool /Volumes/Data/slave/ios-simulator-11-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:471 Value of: message.UTF8String Actual: "SessionCookieName=CookieValue" Expected: "PersistentCookieName=CookieValue; SessionCookieName=CookieValue" https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Release%20WK2%20%28Tests%29/builds/5348/steps/run-api-tests/logs/stdio What is the current state of this bug? It seems like the first patch here got reverted in https://trac.webkit.org/r234730 We disabled this test on iOS as the change is breaking some apps. The problem is that network process may not have access to the cookie storage file the UI process is using because of sandbox. |