Bug 186806

Summary: REGRESSION (r231850): Cookie file cannot be read or written by network process
Product: WebKit Reporter: Sihui Liu <sihui_liu>
Component: WebKit2Assignee: Sihui Liu <sihui_liu>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ggaren, realdawei, ryanhaddad, sihui_liu, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing
none
Patch
none
Patch for landing
none
Patch for landing none

Description Sihui Liu 2018-06-19 09:45:16 PDT
Safari signed out of Gmail/Twitter/Facebook/Reddit unexpectedly.
Comment 1 Sihui Liu 2018-06-19 09:46:50 PDT
<rdar://problem/41113791>
Comment 2 Sihui Liu 2018-06-19 11:11:13 PDT
Created attachment 343069 [details]
Patch
Comment 3 Geoffrey Garen 2018-06-19 14:32:46 PDT
Comment on attachment 343069 [details]
Patch

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

r=me

This bug is super mysterious, and it should trigger in our API tests bug doesn't. I think it's worth your time to do a little more debugging to come up with a possibly more cohesive story of this bug -- maybe that path will teach us how to write a regression test for this.

> Source/WebKit/ChangeLog:10
> +        Default websiteDataStore may be added wrongly to network process before default session was 
> +        set, as messages were asynchronous, so the cookie storage could be improperly set.   

Even though messages are async, they are guaranteed to arrive in order. So, I don't think it's possible for the AddWebsiteDataStore message to arrive before the Initialize message. (You can try this out if you're curious.)

But, we do know by testing that moving the initialization of pending cookies to the initialization message fixes this bug. So I think that's a good change.
Comment 4 Sihui Liu 2018-06-19 14:40:06 PDT
Created attachment 343101 [details]
Patch for landing
Comment 5 WebKit Commit Bot 2018-06-19 15:05:01 PDT
Comment on attachment 343101 [details]
Patch for landing

Clearing flags on attachment: 343101

Committed r232989: <https://trac.webkit.org/changeset/232989>
Comment 6 WebKit Commit Bot 2018-06-19 15:05:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Dawei Fenton (:realdawei) 2018-06-20 16:23:01 PDT
(In reply to WebKit Commit Bot from comment #5)
> Comment on attachment 343101 [details]
> Patch for landing
> 
> Clearing flags on attachment: 343101
> 
> Committed r232989: <https://trac.webkit.org/changeset/232989>


Looks like this revision is causing API failures on Mac 
I reproduced locally and it failed, (previous revisions passed)

Her are the commands I used to reproduce locally: ("testbuild-232989" is my local spade)

run-api-tests --root testbuild-232989 --no-build --debug --verbose ServiceWorkers.ServiceWorkerAndCacheStorageSpecificDirectories
and
run-api-tests --root testbuild-232989 --no-build --debug --verbose ServiceWorkers.ServiceWorkerAndCacheStorageDefaultDirectories
  

Sample output:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/5150/steps/run-api-tests/logs/stdio

Failed

    TestWebKitAPI.ServiceWorkers.ServiceWorkerAndCacheStorageDefaultDirectories
        Received data during response processing, queuing it.
        Received data during response processing, queuing it.
        
        /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:1175
        Value of: retrievedString.contains("/Caches/TestWebKitAPI/WebKit/CacheStorage")
          Actual: false
        Expected: true
        

    TestWebKitAPI.ServiceWorkers.ServiceWorkerAndCacheStorageSpecificDirectories
        Received data during response processing, queuing it.
        Received data during response processing, queuing it.
        
        /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:1223
        Value of: retrievedString.contains("\"path\": \"/var/tmp\"")
          Actual: false
        Expected: true
Comment 8 Sihui Liu 2018-06-21 14:54:20 PDT
Tracking test failure in https://bugs.webkit.org/show_bug.cgi?id=186897.
Comment 9 Ryan Haddad 2018-06-21 15:47:24 PDT
This change was rolled out in https://trac.webkit.org/changeset/233044/webkit
Comment 10 Sihui Liu 2018-06-21 17:06:11 PDT
Created attachment 343291 [details]
Patch
Comment 11 Sihui Liu 2018-06-22 09:41:47 PDT
*** Bug 186897 has been marked as a duplicate of this bug. ***
Comment 12 WebKit Commit Bot 2018-06-22 10:06:50 PDT
Comment on attachment 343291 [details]
Patch

Clearing flags on attachment: 343291

Committed r233084: <https://trac.webkit.org/changeset/233084>
Comment 13 WebKit Commit Bot 2018-06-22 10:06:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Dawei Fenton (:realdawei) 2018-06-22 13:35:16 PDT
(In reply to WebKit Commit Bot from comment #12)
> Comment on attachment 343291 [details]
> Patch
> 
> Clearing flags on attachment: 343291
> 
> Committed r233084: <https://trac.webkit.org/changeset/233084>


Looks like we are getting an API test failure after this patch:

https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/5195/steps/run-api-tests/logs/stdio

Failed

    TestWebKitAPI.WebKit.WKHTTPCookieStoreWithoutProcessPool
        
        /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:471
        Value of: message.UTF8String
          Actual: "PersistentCookieName=CookieValue"
        Expected: “PersistentCookieName=CookieValue; SessionCookieName=CookieValue"
Comment 15 Sihui Liu 2018-06-22 14:43:05 PDT
Missed a changed file in the patch which caused the API test failure. 
Running all API & Layout tests locally now to make sure the new patch doesn't cause any present tests to fail.
Comment 16 Dawei Fenton (:realdawei) 2018-06-22 14:50:13 PDT
(In reply to Sihui Liu from comment #15)
> Missed a changed file in the patch which caused the API test failure. 
> Running all API & Layout tests locally now to make sure the new patch
> doesn't cause any present tests to fail.

cool :)
Comment 17 Sihui Liu 2018-06-22 15:41:12 PDT
Created attachment 343377 [details]
Patch for landing
Comment 18 Sihui Liu 2018-06-22 15:42:30 PDT
Created attachment 343379 [details]
Patch for landing
Comment 19 WebKit Commit Bot 2018-06-22 16:22:27 PDT
Comment on attachment 343379 [details]
Patch for landing

Clearing flags on attachment: 343379

Committed r233108: <https://trac.webkit.org/changeset/233108>
Comment 20 WebKit Commit Bot 2018-06-22 16:22:29 PDT
All reviewed patches have been landed.  Closing bug.