Bug 185831 – [iOS] TestWebKitAPI.WebKit.WKHTTPCookieStoreWithoutProcessPool fails because cookies use different files with/without processpool

• Component: New Bugs
• Status: RESOLVED FIXED

[Ryan Haddad]

TestWebKitAPI.WebKit.WKHTTPCookieStoreWithoutProcessPool
        2018-05-21 12:14:45.165 TestWebKitAPI[42919:17694317] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Completion handler passed to -[CookieUIDelegate webView:runJavaScriptAlertPanelWithMessage:initiatedByFrame:completionHandler:] was not called'
        *** First throw call stack:
        (
        	0   CoreFoundation                      0x0000000111cab1e6 __exceptionPreprocess + 294
        	1   libobjc.A.dylib                     0x00000001112ad031 objc_exception_throw + 48
        	2   CoreFoundation                      0x0000000111d20975 +[NSException raise:format:] + 197
        	3   WebKit                              0x00000001092d55bf _ZN6WebKit28CompletionHandlerCallCheckerD2Ev + 129
        	4   WebKit                              0x00000001092df11c _ZNK3WTF20ThreadSafeRefCountedIN6WebKit28CompletionHandlerCallCheckerELNS_17DestructionThreadE0EE5derefEv + 36
        	5   WebKit                              0x00000001093f0193 _ZZN3WTF8BlockPtrIFvvEE12fromCallableIZN6WebKit10UIDelegate8UIClient18runJavaScriptAlertEPNS4_12WebPageProxyERKNS_6StringEPNS4_13WebFrameProxyERKN7WebCore18SecurityOriginDataEONS_8FunctionIS1_EEE3$_1EES2_T_ENUlPKvE_8__invokeESO_ + 29
        	6   libsystem_blocks.dylib              0x00000001122ea98a _Block_release + 111
        	7   WebKit                              0x00000001093ee344 _ZN6WebKit10UIDelegate8UIClient18runJavaScriptAlertEPNS_12WebPageProxyERKN3WTF6StringEPNS_13WebFrameProxyERKN7WebCore18SecurityOriginDataEONS4_8FunctionIFvvEEE + 376
        	8   WebKit                              0x00000001094e7aa7 _ZN6WebKit12WebPageProxy18runJavaScriptAlertEyRKN7WebCore18SecurityOriginDataERKN3WTF6StringEONS5_17CompletionHandlerIFvvEEE + 199
        	9   WebKit                              0x000000010950c4b9 _ZN3IPC20handleMessageDelayedIN8Messages12WebPageProxy18RunJavaScriptAlertEN6WebKit12WebPageProxyEMS5_FvyRKN7WebCore18SecurityOriginDataERKN3WTF6StringEONSA_17CompletionHandlerIFvvEEEEEEvRNS_10ConnectionERNS_7DecoderERNSt3__110unique_ptrINS_7EncoderENSO_14default_deleteISQ_EEEEPT0_T1_ + 175
        	10  WebKit                              0x000000010930cde7 _ZN3IPC18MessageReceiverMap19dispatchSyncMessageERNS_10ConnectionERNS_7DecoderERNSt3__110unique_ptrINS_7EncoderENS5_14default_deleteIS7_EEEE + 141
        	11  WebKit                              0x0000000109568e94 _ZN6WebKit15WebProcessProxy21didReceiveSyncMessageERN3IPC10ConnectionERNS1_7DecoderERNSt3__110unique_ptrINS1_7EncoderENS6_14default_deleteIS8_EEEE + 28
        	12  WebKit                              0x00000001092d81a9 _ZN3IPC10Connection19dispatchSyncMessageERNS_7DecoderE + 209
        	13  WebKit                              0x00000001092d5b20 _ZN3IPC10Connection15dispatchMessageENSt3__110unique_ptrINS_7DecoderENS1_14default_deleteIS3_EEEE + 104
        	14  WebKit                              0x00000001092d83d1 _ZN3IPC10Connection18dispatchOneMessageEv + 177
        	15  JavaScriptCore                      0x000000010836d9dc _ZN3WTF7RunLoop11performWorkEv + 236
        	16  JavaScriptCore                      0x000000010836dc82 _ZN3WTF7RunLoop11performWorkEPv + 34
        	17  CoreFoundation                      0x0000000111c4dbb1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
        	18  CoreFoundation                      0x0000000111c324af __CFRunLoopDoSources0 + 271
        	19  CoreFoundation                      0x0000000111c31a6f __CFRunLoopRun + 1263
        	20  CoreFoundation                      0x0000000111c3130b CFRunLoopRunSpecific + 635
        	21  Foundation                          0x0000000110ca8b4a -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 274
        	22  TestWebKitAPI                       0x0000000107e56d54 _ZN13TestWebKitAPI4Util3runEPb + 119
        	23  TestWebKitAPI                       0x0000000107e84eb9 _ZN47WebKit_WKHTTPCookieStoreWithoutProcessPool_Test8TestBodyEv + 1849
        	24  TestWebKitAPI                       0x0000000107eb7018 _ZN7testing4Test3RunEv + 92
        	25  TestWebKitAPI                       0x0000000107eb74d6 _ZN7testing8internal12TestInfoImpl3RunEv + 180
        	26  TestWebKitAPI                       0x0000000107eb784e _ZN7testing8TestCase3RunEv + 196
        	27  TestWebKitAPI                       0x0000000107ebaab8 _ZN7testing8internal12UnitTestImpl11RunAllTestsEv + 614
        	28  TestWebKitAPI                       0x0000000107e070a4 _ZN13TestWebKitAPI15TestsController3runEiPPc + 120
        	29  TestWebKitAPI                       0x0000000107e9c0d8 main + 322
        	30  libdyld.dylib                       0x0000000112278955 start + 1
        )
        libc++abi.dylib: terminating with uncaught exception of type NSException
        Child process terminated with signal 6: Abort trap

https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Release%20WK2%20%28Tests%29/builds/5109

[Ryan Haddad]

It looks like this started with https://trac.webkit.org/changeset/231999/webkit

[Chris Dumez]

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.

[Sihui Liu]

Created attachment 340990 [details]
Patch

[Chris Dumez]

Comment on attachment 340990 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 340990 [details]
Patch

Clearing flags on attachment: 340990

Committed r232083: <https://trac.webkit.org/changeset/232083>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40468716>

[Ryan Haddad]

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

[Sihui Liu]

Reopening to attach new patch.

[Sihui Liu]

Created attachment 341206 [details]
Patch

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

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>";

[Sihui Liu]

Created attachment 341464 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 341464 [details]
Patch

r=me

[Ryan Haddad]

Any reason not to land this? It would be good to get this test passing on the bots.

[Chris Dumez]

(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.

[WebKit Commit Bot]

Comment on attachment 341464 [details]
Patch

Clearing flags on attachment: 341464

Committed r232357: <https://trac.webkit.org/changeset/232357>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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

[Alexey Proskuryakov]

What is the current state of this bug? It seems like the first patch here got reverted in https://trac.webkit.org/r234730

[Sihui Liu]

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.