Changeset 232886 in webkit


Ignore:
Timestamp:
Jun 15, 2018 12:28:18 PM (6 years ago)
Author:
beidson@apple.com
Message:

Crash in both StorageProcess and UIProcess when using custom WKWebsiteDataStores for data management.
<rdar://problem/41019893> and ​https://bugs.webkit.org/show_bug.cgi?id=186682

Reviewed by Chris Dumez.

Source/WebKit:

  • UIProcess/Storage/StorageProcessProxy.cpp:

(WebKit::StorageProcessProxy::didClose): Protect this and the process pool as the cleanup that follows

might cause either to get destroyed.

  • UIProcess/WebsiteData/WebsiteDataStore.cpp:

(WebKit::WebsiteDataStore::fetchDataAndApply): Protect the operating WebsiteDataStore while async operations

are in flight. Otherwise if the data store is destroyed, the SessionIDs for those operations will get
destroyed before they complete.

(WebKit::WebsiteDataStore::removeData): Ditto.

Tools:

  • TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
  • TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:

(TEST):

Location:
trunk
Files:
3 added
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r232884 r232886  
     12018-06-15  Brady Eidson  <beidson@apple.com>
     2
     3        Crash in both StorageProcess and UIProcess when using custom WKWebsiteDataStores for data management.
     4        <rdar://problem/41019893> and https://bugs.webkit.org/show_bug.cgi?id=186682
     5
     6        Reviewed by Chris Dumez.
     7
     8        * UIProcess/Storage/StorageProcessProxy.cpp:
     9        (WebKit::StorageProcessProxy::didClose): Protect this and the process pool as the cleanup that follows
     10          might cause either to get destroyed.
     11
     12        * UIProcess/WebsiteData/WebsiteDataStore.cpp:
     13        (WebKit::WebsiteDataStore::fetchDataAndApply): Protect the operating WebsiteDataStore while async operations
     14          are in flight. Otherwise if the data store is destroyed, the SessionIDs for those operations will get
     15          destroyed before they complete.
     16        (WebKit::WebsiteDataStore::removeData): Ditto.
     17
    1182018-06-15  Per Arne Vollan  <pvollan@apple.com>
    219

  • trunk/Source/WebKit/UIProcess/Storage/StorageProcessProxy.cpp

    r231931 r232886  
    136136void StorageProcessProxy::didClose(IPC::Connection&)
    137137{
     138    auto protectedProcessPool = makeRef(m_processPool);
     139
    138140    // The storage process must have crashed or exited, so send any pending sync replies we might have.
    139141    while (!m_pendingConnectionReplies.isEmpty()) {
    … …  
    149151    }
    150152
    151     for (const auto& callback : m_pendingFetchWebsiteDataCallbacks.values())
    152         callback(WebsiteData());
    153     m_pendingFetchWebsiteDataCallbacks.clear();
    154 
    155     for (const auto& callback : m_pendingDeleteWebsiteDataCallbacks.values())
    156         callback();
    157     m_pendingDeleteWebsiteDataCallbacks.clear();
    158 
    159     for (const auto& callback : m_pendingDeleteWebsiteDataForOriginsCallbacks.values())
    160         callback();
    161     m_pendingDeleteWebsiteDataForOriginsCallbacks.clear();
     153    while (!m_pendingFetchWebsiteDataCallbacks.isEmpty())
     154        m_pendingFetchWebsiteDataCallbacks.take(m_pendingFetchWebsiteDataCallbacks.begin()->key)(WebsiteData { });
     155
     156    while (!m_pendingDeleteWebsiteDataCallbacks.isEmpty())
     157        m_pendingDeleteWebsiteDataCallbacks.take(m_pendingDeleteWebsiteDataCallbacks.begin()->key)();
     158
     159    while (!m_pendingDeleteWebsiteDataForOriginsCallbacks.isEmpty())
     160        m_pendingDeleteWebsiteDataForOriginsCallbacks.take(m_pendingDeleteWebsiteDataForOriginsCallbacks.begin()->key)();
    162161
    163162    // Tell ProcessPool to forget about this storage process. This may cause us to be deleted.

  • trunk/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp

    r232863 r232886  
    237237{
    238238    struct CallbackAggregator final : ThreadSafeRefCounted<CallbackAggregator> {
    239         explicit CallbackAggregator(OptionSet<WebsiteDataFetchOption> fetchOptions, RefPtr<WorkQueue>&& queue, Function<void(Vector<WebsiteDataRecord>)>&& apply)
     239        explicit CallbackAggregator(OptionSet<WebsiteDataFetchOption> fetchOptions, RefPtr<WorkQueue>&& queue, Function<void(Vector<WebsiteDataRecord>)>&& apply, WebsiteDataStore& dataStore)
    240240            : fetchOptions(fetchOptions)
    241241            , queue(WTFMove(queue))
    242242            , apply(WTFMove(apply))
     243            , protectedDataStore(dataStore)
    243244        {
    244245        }
    … …  
    345346
    346347        HashMap<String, WebsiteDataRecord> m_websiteDataRecords;
     348        Ref<WebsiteDataStore> protectedDataStore;
    347349    };
    348350
    349     RefPtr<CallbackAggregator> callbackAggregator = adoptRef(new CallbackAggregator(fetchOptions, WTFMove(queue), WTFMove(apply)));
     351    RefPtr<CallbackAggregator> callbackAggregator = adoptRef(new CallbackAggregator(fetchOptions, WTFMove(queue), WTFMove(apply), *this));
    350352
    351353#if ENABLE(VIDEO)
    … …  
    649651{
    650652    struct CallbackAggregator : ThreadSafeRefCounted<CallbackAggregator> {
    651         explicit CallbackAggregator(Function<void()>&& completionHandler)
     653        explicit CallbackAggregator(WebsiteDataStore& dataStore, Function<void()>&& completionHandler)
    652654            : completionHandler(WTFMove(completionHandler))
     655            , protectedDataStore(dataStore)
    653656        {
    654657        }
    … …  
    675678        unsigned pendingCallbacks = 0;
    676679        Function<void()> completionHandler;
     680        Ref<WebsiteDataStore> protectedDataStore;
    677681    };
    678682
    679     RefPtr<CallbackAggregator> callbackAggregator = adoptRef(new CallbackAggregator(WTFMove(completionHandler)));
     683    RefPtr<CallbackAggregator> callbackAggregator = adoptRef(new CallbackAggregator(*this, WTFMove(completionHandler)));
    680684
    681685#if ENABLE(VIDEO)
    … …  
    907911
    908912    struct CallbackAggregator : ThreadSafeRefCounted<CallbackAggregator> {
    909         explicit CallbackAggregator(Function<void()>&& completionHandler)
     913        explicit CallbackAggregator(WebsiteDataStore& dataStore, Function<void()>&& completionHandler)
    910914            : completionHandler(WTFMove(completionHandler))
     915            , protectedDataStore(dataStore)
    911916        {
    912917        }
    … …  
    933938        unsigned pendingCallbacks = 0;
    934939        Function<void()> completionHandler;
     940        Ref<WebsiteDataStore> protectedDataStore;
    935941    };
    936942
    937     RefPtr<CallbackAggregator> callbackAggregator = adoptRef(new CallbackAggregator(WTFMove(completionHandler)));
     943    RefPtr<CallbackAggregator> callbackAggregator = adoptRef(new CallbackAggregator(*this, WTFMove(completionHandler)));
    938944   
    939945    if (dataTypes.contains(WebsiteDataType::DiskCache)) {

  • trunk/Tools/ChangeLog

    r232885 r232886  
     12018-06-15  Brady Eidson  <beidson@apple.com>
     2
     3        Crash in both StorageProcess and UIProcess when using custom WKWebsiteDataStores for data management.
     4        <rdar://problem/41019893> and https://bugs.webkit.org/show_bug.cgi?id=186682
     5
     6        Reviewed by Chris Dumez.
     7
     8        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
     9        * TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:
     10        (TEST):
     11
    1122018-06-15  Carlos Alberto Lopez Perez  <clopez@igalia.com>
    213

  • trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj

    r232664 r232886  
    180180                51393E221523952D005F39C5 /* DOMWindowExtensionBasic_Bundle.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 51393E1D1523944A005F39C5 /* DOMWindowExtensionBasic_Bundle.cpp */; };
    181181                5142B2731517C8C800C32B19 /* ContextMenuCanCopyURL.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 5142B2721517C89100C32B19 /* ContextMenuCanCopyURL.html */; };
     182                51460E1220D421F2005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3 in Copy Resources */ = {isa = PBXBuildFile; fileRef = 51460E0F20D4216E005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3 */; };
     183                51460E1320D421F2005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3-shm in Copy Resources */ = {isa = PBXBuildFile; fileRef = 51460E1020D4216E005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3-shm */; };
     184                51460E1420D421F2005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3-wal in Copy Resources */ = {isa = PBXBuildFile; fileRef = 51460E1120D4216E005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3-wal */; };
    182185                514958BE1F7427AC00E87BAD /* WKWebViewAutofillTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 514958BD1F7427AC00E87BAD /* WKWebViewAutofillTests.mm */; };
    183186                515BE16F1D428BB100DD7C68 /* StoreBlobToBeDeleted.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 515BE16E1D4288FF00DD7C68 /* StoreBlobToBeDeleted.html */; };
    … …  
    885888                        dstSubfolderSpec = 7;
    886889                        files = (
     890                                51460E1220D421F2005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3 in Copy Resources */,
     891                                51460E1320D421F2005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3-shm in Copy Resources */,
     892                                51460E1420D421F2005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3-wal in Copy Resources */,
    887893                                1A9E52C913E65EF4006917F5 /* 18-characters.html in Copy Resources */,
    888894                                379028B914FAC24C007E6B43 /* acceptsFirstMouse.html in Copy Resources */,
    … …  
    13921398                5142B2701517C88B00C32B19 /* ContextMenuCanCopyURL.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = ContextMenuCanCopyURL.mm; sourceTree = "<group>"; };
    13931399                5142B2721517C89100C32B19 /* ContextMenuCanCopyURL.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = ContextMenuCanCopyURL.html; sourceTree = "<group>"; };
     1400                51460E0F20D4216E005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3 */ = {isa = PBXFileReference; lastKnownFileType = file; path = "SimpleServiceWorkerRegistrations-2.sqlite3"; sourceTree = "<group>"; };
     1401                51460E1020D4216E005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3-shm */ = {isa = PBXFileReference; lastKnownFileType = file; path = "SimpleServiceWorkerRegistrations-2.sqlite3-shm"; sourceTree = "<group>"; };
     1402                51460E1120D4216E005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3-wal */ = {isa = PBXFileReference; lastKnownFileType = file; path = "SimpleServiceWorkerRegistrations-2.sqlite3-wal"; sourceTree = "<group>"; };
    13941403                514958BD1F7427AC00E87BAD /* WKWebViewAutofillTests.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = WKWebViewAutofillTests.mm; sourceTree = "<group>"; };
    13951404                515BE16E1D4288FF00DD7C68 /* StoreBlobToBeDeleted.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = StoreBlobToBeDeleted.html; sourceTree = "<group>"; };
    … …  
    24702479                        isa = PBXGroup;
    24712480                        children = (
     2481                                51460E0F20D4216E005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3 */,
     2482                                51460E1020D4216E005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3-shm */,
     2483                                51460E1120D4216E005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3-wal */,
    24722484                                C25CCA0C1E5140E50026CB8A /* AllAhem.svg */,
    24732485                                F4A9202E1FEE34C800F59590 /* apple-data-url.html */,

  • trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm

    r231603 r232886  
    226226}
    227227
     228TEST(WebKit, CustomDataStorePathsVersusCompletionHandlers)
     229{
     230    // Copy the baked database files to the database directory
     231    NSURL *url1 = [[NSBundle mainBundle] URLForResource:@"SimpleServiceWorkerRegistrations-2" withExtension:@"sqlite3" subdirectory:@"TestWebKitAPI.resources"];
     232    NSURL *url2 = [[NSBundle mainBundle] URLForResource:@"SimpleServiceWorkerRegistrations-2" withExtension:@"sqlite3-shm" subdirectory:@"TestWebKitAPI.resources"];
     233    NSURL *url3 = [[NSBundle mainBundle] URLForResource:@"SimpleServiceWorkerRegistrations-2" withExtension:@"sqlite3-wal" subdirectory:@"TestWebKitAPI.resources"];
     234
     235    NSURL *swPath = [NSURL fileURLWithPath:[@"~/Library/Caches/TestWebKitAPI/WebKit/ServiceWorkers/" stringByExpandingTildeInPath]];
     236    [[NSFileManager defaultManager] removeItemAtURL:swPath error:nil];
     237    EXPECT_FALSE([[NSFileManager defaultManager] fileExistsAtPath:swPath.path]);
     238
     239    [[NSFileManager defaultManager] createDirectoryAtURL:swPath withIntermediateDirectories:YES attributes:nil error:nil];
     240    [[NSFileManager defaultManager] copyItemAtURL:url1 toURL:[swPath URLByAppendingPathComponent:@"ServiceWorkerRegistrations-2.sqlite3"] error:nil];
     241    [[NSFileManager defaultManager] copyItemAtURL:url2 toURL:[swPath URLByAppendingPathComponent:@"ServiceWorkerRegistrations-2.sqlite3-shm"] error:nil];
     242    [[NSFileManager defaultManager] copyItemAtURL:url3 toURL:[swPath URLByAppendingPathComponent:@"ServiceWorkerRegistrations-2.sqlite3-wal"] error:nil];
     243
     244    auto websiteDataStoreConfiguration = adoptNS([[_WKWebsiteDataStoreConfiguration alloc] init]);
     245    websiteDataStoreConfiguration.get()._serviceWorkerRegistrationDirectory = swPath;
     246    auto dataStore = adoptNS([[WKWebsiteDataStore alloc] _initWithConfiguration:websiteDataStoreConfiguration.get()]);
     247
     248    // Fetch SW records
     249    auto websiteDataTypes = adoptNS([[NSSet alloc] initWithArray:@[WKWebsiteDataTypeServiceWorkerRegistrations]]);
     250    static bool readyToContinue;
     251    [dataStore fetchDataRecordsOfTypes:websiteDataTypes.get() completionHandler:^(NSArray<WKWebsiteDataRecord *> *dataRecords) {
     252        EXPECT_EQ((int)dataRecords.count, 1);
     253        readyToContinue = true;
     254    }];
     255    TestWebKitAPI::Util::run(&readyToContinue);
     256    readyToContinue = false;
     257
     258    // Fetch records again, this time releasing our reference to the data store while the request is in flight.
     259    // Without a bug fix, this would crash the StorageProcess and the UI process wouldn't get the info back.
     260    [dataStore fetchDataRecordsOfTypes:websiteDataTypes.get() completionHandler:^(NSArray<WKWebsiteDataRecord *> *dataRecords) {
     261        EXPECT_EQ((int)dataRecords.count, 1);
     262        readyToContinue = true;
     263    }];
     264    dataStore = nil;
     265    TestWebKitAPI::Util::run(&readyToContinue);
     266    readyToContinue = false;
     267
     268    // Delete all SW records, releasing our reference to the data store while the request is in flight.
     269    // Same as above - We used to crash the storage process and the records weren't actually deleted.
     270    dataStore = adoptNS([[WKWebsiteDataStore alloc] _initWithConfiguration:websiteDataStoreConfiguration.get()]);
     271    [dataStore removeDataOfTypes:websiteDataTypes.get() modifiedSince:[NSDate distantPast] completionHandler:^() {
     272        readyToContinue = true;
     273    }];
     274    dataStore = nil;
     275    TestWebKitAPI::Util::run(&readyToContinue);
     276    readyToContinue = false;
     277
     278    // The StorageProcess should not have crashed, the records should have been deleted, and the callback should have been made.
     279    // Now refetch the records to verify they are gone.
     280    dataStore = adoptNS([[WKWebsiteDataStore alloc] _initWithConfiguration:websiteDataStoreConfiguration.get()]);
     281    [dataStore fetchDataRecordsOfTypes:websiteDataTypes.get() completionHandler:^(NSArray<WKWebsiteDataRecord *> *dataRecords) {
     282        EXPECT_EQ((int)dataRecords.count, 0);
     283        readyToContinue = true;
     284    }];
     285    TestWebKitAPI::Util::run(&readyToContinue);
     286}
     287
    228288TEST(WebKit, WebsiteDataStoreEphemeral)
    229289{
Note: See TracChangeset for help on using the changeset viewer.