Changeset 234664 in webkit
- Timestamp:
- Aug 7, 2018 12:09:16 PM (6 years ago)
- Location:
- trunk/Source/WebKit
- Files:
-
- 5 edited
Legend:
- Unmodified
- Added
- Removed
-
trunk/Source/WebKit/ChangeLog
r234662 r234664 1 2018-08-07 Chris Dumez <cdumez@apple.com> 2 3 StorageManager should stop ref'ing IPC::Connections as this is leak-prone 4 https://bugs.webkit.org/show_bug.cgi?id=188380 5 6 Reviewed by Alex Christensen. 7 8 StorageManager should stop ref'ing IPC::Connections as this is leak-prone. Instead, assign a unique identifier 9 to each IPC::Connection and store this identifier intead of a RefPtr<IPC::Connection>. When the StorageManager 10 needs an actual IPC::Connection, it can look it up from the identifier. 11 12 * Platform/IPC/Connection.cpp: 13 (IPC::Connection::Connection): 14 (IPC::Connection::~Connection): 15 (IPC::Connection::connection): 16 * Platform/IPC/Connection.h: 17 (IPC::Connection::uniqueID const): 18 * UIProcess/WebStorage/StorageManager.cpp: 19 (WebKit::StorageManager::StorageArea::addListener): 20 (WebKit::StorageManager::StorageArea::removeListener): 21 (WebKit::StorageManager::StorageArea::hasListener const): 22 (WebKit::StorageManager::StorageArea::setItem): 23 (WebKit::StorageManager::StorageArea::removeItem): 24 (WebKit::StorageManager::StorageArea::clear): 25 (WebKit::StorageManager::StorageArea::dispatchEvents const): 26 (WebKit::StorageManager::SessionStorageNamespace::allowedConnection const): 27 (WebKit::StorageManager::SessionStorageNamespace::setAllowedConnection): 28 (WebKit::StorageManager::setAllowedSessionStorageNamespaceConnection): 29 (WebKit::StorageManager::processDidCloseConnection): 30 (WebKit::StorageManager::createLocalStorageMap): 31 (WebKit::StorageManager::createTransientLocalStorageMap): 32 (WebKit::StorageManager::createSessionStorageMap): 33 (WebKit::StorageManager::destroyStorageMap): 34 (WebKit::StorageManager::setItem): 35 (WebKit::StorageManager::removeItem): 36 (WebKit::StorageManager::clear): 37 (WebKit::StorageManager::applicationWillTerminate): 38 (WebKit::StorageManager::findStorageArea const): 39 * UIProcess/WebStorage/StorageManager.h: 40 1 41 2018-08-07 Eric Carlson <eric.carlson@apple.com> 2 42 -
trunk/Source/WebKit/Platform/IPC/Connection.cpp
r233808 r234664 232 232 } 233 233 234 static HashMap<IPC::Connection::UniqueID, Connection*>& allConnections() 235 { 236 static NeverDestroyed<HashMap<IPC::Connection::UniqueID, Connection*>> map; 237 return map; 238 } 239 234 240 Connection::Connection(Identifier identifier, bool isServer, Client& client) 235 241 : m_client(client) 242 , m_uniqueID(generateObjectIdentifier<UniqueIDType>()) 236 243 , m_isServer(isServer) 237 244 , m_syncRequestID(0) … … 249 256 { 250 257 ASSERT(RunLoop::isMain()); 258 allConnections().add(m_uniqueID, this); 251 259 252 260 platformInitialize(identifier); … … 260 268 Connection::~Connection() 261 269 { 270 ASSERT(RunLoop::isMain()); 262 271 ASSERT(!isValid()); 272 273 allConnections().remove(m_uniqueID); 274 } 275 276 Connection* Connection::connection(UniqueID uniqueID) 277 { 278 ASSERT(RunLoop::isMain()); 279 return allConnections().get(uniqueID); 263 280 } 264 281 -
trunk/Source/WebKit/Platform/IPC/Connection.h
r233111 r234664 40 40 #include <wtf/HashMap.h> 41 41 #include <wtf/Lock.h> 42 #include <wtf/ObjectIdentifier.h> 42 43 #include <wtf/OptionSet.h> 43 44 #include <wtf/RunLoop.h> … … 87 88 class UnixMessage; 88 89 89 class Connection : public ThreadSafeRefCounted<Connection > {90 class Connection : public ThreadSafeRefCounted<Connection, WTF::DestructionThread::Main> { 90 91 public: 91 92 class Client : public MessageReceiver { … … 151 152 152 153 Client& client() const { return m_client; } 154 155 enum UniqueIDType { }; 156 using UniqueID = ObjectIdentifier<UniqueIDType>; 157 158 static Connection* connection(UniqueID); 159 UniqueID uniqueID() const { return m_uniqueID; } 153 160 154 161 void setOnlySendMessagesAsDispatchWhenWaitingForSyncReplyWhenProcessingSuchAMessage(bool); … … 272 279 273 280 Client& m_client; 281 UniqueID m_uniqueID; 274 282 bool m_isServer; 275 283 std::atomic<bool> m_isValid { true }; -
trunk/Source/WebKit/UIProcess/WebStorage/StorageManager.cpp
r234611 r234664 50 50 const WebCore::SecurityOriginData& securityOrigin() const { return m_securityOrigin; } 51 51 52 void addListener(IPC::Connection &, uint64_t storageMapID);53 void removeListener(IPC::Connection &, uint64_t storageMapID);54 bool hasListener(IPC::Connection &, uint64_t storageMapID) const;52 void addListener(IPC::Connection::UniqueID, uint64_t storageMapID); 53 void removeListener(IPC::Connection::UniqueID, uint64_t storageMapID); 54 bool hasListener(IPC::Connection::UniqueID, uint64_t storageMapID) const; 55 55 56 56 Ref<StorageArea> clone() const; 57 57 58 void setItem(IPC::Connection *sourceConnection, uint64_t sourceStorageAreaID, const String& key, const String& value, const String& urlString, bool& quotaException);59 void removeItem(IPC::Connection *sourceConnection, uint64_t sourceStorageAreaID, const String& key, const String& urlString);60 void clear(IPC::Connection *sourceConnection, uint64_t sourceStorageAreaID, const String& urlString);58 void setItem(IPC::Connection::UniqueID sourceConnection, uint64_t sourceStorageAreaID, const String& key, const String& value, const String& urlString, bool& quotaException); 59 void removeItem(IPC::Connection::UniqueID sourceConnection, uint64_t sourceStorageAreaID, const String& key, const String& urlString); 60 void clear(IPC::Connection::UniqueID sourceConnection, uint64_t sourceStorageAreaID, const String& urlString); 61 61 62 62 const HashMap<String, String>& items() const; … … 70 70 void openDatabaseAndImportItemsIfNeeded() const; 71 71 72 void dispatchEvents(IPC::Connection *sourceConnection, uint64_t sourceStorageAreaID, const String& key, const String& oldValue, const String& newValue, const String& urlString) const;72 void dispatchEvents(IPC::Connection::UniqueID sourceConnection, uint64_t sourceStorageAreaID, const String& key, const String& oldValue, const String& newValue, const String& urlString) const; 73 73 74 74 // Will be null if the storage area belongs to a session storage namespace. … … 81 81 82 82 RefPtr<StorageMap> m_storageMap; 83 HashSet<std::pair< RefPtr<IPC::Connection>, uint64_t>> m_eventListeners;83 HashSet<std::pair<IPC::Connection::UniqueID, uint64_t>> m_eventListeners; 84 84 }; 85 85 … … 186 186 } 187 187 188 void StorageManager::StorageArea::addListener(IPC::Connection & connection, uint64_t storageMapID)189 { 190 ASSERT(!m_eventListeners.contains(std::make_pair( &connection, storageMapID)));191 m_eventListeners.add(std::make_pair( &connection, storageMapID));192 } 193 194 void StorageManager::StorageArea::removeListener(IPC::Connection & connection, uint64_t storageMapID)195 { 196 ASSERT(isSessionStorage() || m_eventListeners.contains(std::make_pair( &connection, storageMapID)));197 m_eventListeners.remove(std::make_pair( &connection, storageMapID));198 } 199 200 bool StorageManager::StorageArea::hasListener(IPC::Connection & connection, uint64_t storageMapID) const201 { 202 return m_eventListeners.contains(std::make_pair( &connection, storageMapID));188 void StorageManager::StorageArea::addListener(IPC::Connection::UniqueID connectionID, uint64_t storageMapID) 189 { 190 ASSERT(!m_eventListeners.contains(std::make_pair(connectionID, storageMapID))); 191 m_eventListeners.add(std::make_pair(connectionID, storageMapID)); 192 } 193 194 void StorageManager::StorageArea::removeListener(IPC::Connection::UniqueID connectionID, uint64_t storageMapID) 195 { 196 ASSERT(isSessionStorage() || m_eventListeners.contains(std::make_pair(connectionID, storageMapID))); 197 m_eventListeners.remove(std::make_pair(connectionID, storageMapID)); 198 } 199 200 bool StorageManager::StorageArea::hasListener(IPC::Connection::UniqueID connectionID, uint64_t storageMapID) const 201 { 202 return m_eventListeners.contains(std::make_pair(connectionID, storageMapID)); 203 203 } 204 204 … … 213 213 } 214 214 215 void StorageManager::StorageArea::setItem(IPC::Connection *sourceConnection, uint64_t sourceStorageAreaID, const String& key, const String& value, const String& urlString, bool& quotaException)215 void StorageManager::StorageArea::setItem(IPC::Connection::UniqueID sourceConnection, uint64_t sourceStorageAreaID, const String& key, const String& value, const String& urlString, bool& quotaException) 216 216 { 217 217 openDatabaseAndImportItemsIfNeeded(); … … 232 232 } 233 233 234 void StorageManager::StorageArea::removeItem(IPC::Connection *sourceConnection, uint64_t sourceStorageAreaID, const String& key, const String& urlString)234 void StorageManager::StorageArea::removeItem(IPC::Connection::UniqueID sourceConnection, uint64_t sourceStorageAreaID, const String& key, const String& urlString) 235 235 { 236 236 openDatabaseAndImportItemsIfNeeded(); … … 250 250 } 251 251 252 void StorageManager::StorageArea::clear(IPC::Connection *sourceConnection, uint64_t sourceStorageAreaID, const String& urlString)252 void StorageManager::StorageArea::clear(IPC::Connection::UniqueID sourceConnection, uint64_t sourceStorageAreaID, const String& urlString) 253 253 { 254 254 openDatabaseAndImportItemsIfNeeded(); … … 281 281 } 282 282 283 for (auto it = m_eventListeners.begin(), end = m_eventListeners.end(); it != end; ++it) 284 it->first->send(Messages::StorageAreaMap::ClearCache(), it->second); 283 for (auto it = m_eventListeners.begin(), end = m_eventListeners.end(); it != end; ++it) { 284 RunLoop::main().dispatch([connectionID = it->first, destinationStorageAreaID = it->second] { 285 if (auto* connection = IPC::Connection::connection(connectionID)) 286 connection->send(Messages::StorageAreaMap::ClearCache(), destinationStorageAreaID); 287 }); 288 } 285 289 } 286 290 … … 301 305 } 302 306 303 void StorageManager::StorageArea::dispatchEvents(IPC::Connection* sourceConnection, uint64_t sourceStorageAreaID, const String& key, const String& oldValue, const String& newValue, const String& urlString) const 304 { 305 for (HashSet<std::pair<RefPtr<IPC::Connection>, uint64_t>>::const_iterator it = m_eventListeners.begin(), end = m_eventListeners.end(); it != end; ++it) { 306 uint64_t storageAreaID = it->first == sourceConnection ? sourceStorageAreaID : 0; 307 308 it->first->send(Messages::StorageAreaMap::DispatchStorageEvent(storageAreaID, key, oldValue, newValue, urlString), it->second); 307 void StorageManager::StorageArea::dispatchEvents(IPC::Connection::UniqueID sourceConnection, uint64_t sourceStorageAreaID, const String& key, const String& oldValue, const String& newValue, const String& urlString) const 308 { 309 for (auto it = m_eventListeners.begin(), end = m_eventListeners.end(); it != end; ++it) { 310 sourceStorageAreaID = it->first == sourceConnection ? sourceStorageAreaID : 0; 311 312 RunLoop::main().dispatch([connectionID = it->first, sourceStorageAreaID, destinationStorageAreaID = it->second, key = key.isolatedCopy(), oldValue = oldValue.isolatedCopy(), newValue = newValue.isolatedCopy(), urlString = urlString.isolatedCopy()] { 313 if (auto* connection = IPC::Connection::connection(connectionID)) 314 connection->send(Messages::StorageAreaMap::DispatchStorageEvent(sourceStorageAreaID, key, oldValue, newValue, urlString), destinationStorageAreaID); 315 }); 309 316 } 310 317 } … … 374 381 bool isEmpty() const { return m_storageAreaMap.isEmpty(); } 375 382 376 IPC::Connection * allowedConnection() const { return m_allowedConnection.get(); }377 void setAllowedConnection(IPC::Connection *);383 IPC::Connection::UniqueID allowedConnection() const { return m_allowedConnection; } 384 void setAllowedConnection(IPC::Connection::UniqueID); 378 385 379 386 Ref<StorageArea> getOrCreateStorageArea(SecurityOriginData&&); … … 410 417 explicit SessionStorageNamespace(unsigned quotaInBytes); 411 418 412 RefPtr<IPC::Connection>m_allowedConnection;419 IPC::Connection::UniqueID m_allowedConnection; 413 420 unsigned m_quotaInBytes; 414 421 … … 430 437 } 431 438 432 void StorageManager::SessionStorageNamespace::setAllowedConnection(IPC::Connection *allowedConnection)439 void StorageManager::SessionStorageNamespace::setAllowedConnection(IPC::Connection::UniqueID allowedConnection) 433 440 { 434 441 m_allowedConnection = allowedConnection; … … 486 493 void StorageManager::setAllowedSessionStorageNamespaceConnection(uint64_t storageNamespaceID, IPC::Connection* allowedConnection) 487 494 { 488 m_queue->dispatch([this, protectedThis = makeRef(*this), connection = RefPtr<IPC::Connection>(allowedConnection), storageNamespaceID]() mutable { 495 auto allowedConnectionID = allowedConnection ? allowedConnection->uniqueID() : IPC::Connection::UniqueID { }; 496 m_queue->dispatch([this, protectedThis = makeRef(*this), allowedConnectionID, storageNamespaceID]() mutable { 489 497 ASSERT(m_sessionStorageNamespaces.contains(storageNamespaceID)); 490 498 491 m_sessionStorageNamespaces.get(storageNamespaceID)->setAllowedConnection( connection.get());499 m_sessionStorageNamespaces.get(storageNamespaceID)->setAllowedConnection(allowedConnectionID); 492 500 }); 493 501 } … … 520 528 connection.removeWorkQueueMessageReceiver(Messages::StorageManager::messageReceiverName()); 521 529 522 m_queue->dispatch([this, protectedThis = makeRef(*this), connection = Ref<IPC::Connection>(connection)]() mutable {523 Vector<std::pair< RefPtr<IPC::Connection>, uint64_t>> connectionAndStorageMapIDPairsToRemove;530 m_queue->dispatch([this, protectedThis = makeRef(*this), connectionID = connection.uniqueID()]() mutable { 531 Vector<std::pair<IPC::Connection::UniqueID, uint64_t>> connectionAndStorageMapIDPairsToRemove; 524 532 for (auto& storageArea : m_storageAreasByConnection) { 525 if (storageArea.key.first != connection .ptr())533 if (storageArea.key.first != connectionID) 526 534 continue; 527 535 528 storageArea.value->removeListener( *storageArea.key.first, storageArea.key.second);536 storageArea.value->removeListener(storageArea.key.first, storageArea.key.second); 529 537 connectionAndStorageMapIDPairsToRemove.append(storageArea.key); 530 538 } … … 666 674 void StorageManager::createLocalStorageMap(IPC::Connection& connection, uint64_t storageMapID, uint64_t storageNamespaceID, SecurityOriginData&& securityOriginData) 667 675 { 668 std::pair< RefPtr<IPC::Connection>, uint64_t> connectionAndStorageMapIDPair(&connection, storageMapID);676 std::pair<IPC::Connection::UniqueID, uint64_t> connectionAndStorageMapIDPair(connection.uniqueID(), storageMapID); 669 677 670 678 // FIXME: This should be a message check. 671 ASSERT((HashMap<std::pair< RefPtr<IPC::Connection>, uint64_t>, RefPtr<StorageArea>>::isValidKey(connectionAndStorageMapIDPair)));672 673 HashMap<std::pair<RefPtr<IPC::Connection>, uint64_t>, RefPtr<StorageArea>>::AddResultresult = m_storageAreasByConnection.add(connectionAndStorageMapIDPair, nullptr);679 ASSERT((HashMap<std::pair<IPC::Connection::UniqueID, uint64_t>, RefPtr<StorageArea>>::isValidKey(connectionAndStorageMapIDPair))); 680 681 auto result = m_storageAreasByConnection.add(connectionAndStorageMapIDPair, nullptr); 674 682 675 683 // FIXME: These should be a message checks. … … 683 691 684 692 auto storageArea = localStorageNamespace->getOrCreateStorageArea(WTFMove(securityOriginData)); 685 storageArea->addListener(connection , storageMapID);693 storageArea->addListener(connection.uniqueID(), storageMapID); 686 694 687 695 result.iterator->value = WTFMove(storageArea); … … 691 699 { 692 700 // FIXME: This should be a message check. 693 ASSERT(m_storageAreasByConnection.isValidKey({ &connection, storageMapID }));701 ASSERT(m_storageAreasByConnection.isValidKey({ connection.uniqueID(), storageMapID })); 694 702 695 703 // See if we already have session storage for this connection/origin combo. 696 704 // If so, update the map with the new ID, otherwise keep on trucking. 697 705 for (auto it = m_storageAreasByConnection.begin(), end = m_storageAreasByConnection.end(); it != end; ++it) { 698 if (it->key.first != &connection)706 if (it->key.first != connection.uniqueID()) 699 707 continue; 700 708 Ref<StorageArea> area = *it->value; … … 703 711 if (!origin.securityOrigin()->isSameSchemeHostPort(area->securityOrigin().securityOrigin().get())) 704 712 continue; 705 area->addListener(connection , storageMapID);713 area->addListener(connection.uniqueID(), storageMapID); 706 714 // If the storageMapID used as key in m_storageAreasByConnection is no longer one of the StorageArea's listeners, then this means 707 715 // that destroyStorageMap() was already called for that storageMapID but it decided not to remove it from m_storageAreasByConnection 708 716 // so that we could reuse it later on for the same connection/origin combo. In this case, it is safe to remove the previous 709 717 // storageMapID from m_storageAreasByConnection. 710 if (!area->hasListener(connection , it->key.second))718 if (!area->hasListener(connection.uniqueID(), it->key.second)) 711 719 m_storageAreasByConnection.remove(it); 712 m_storageAreasByConnection.add({ &connection, storageMapID }, WTFMove(area));713 return; 714 } 715 716 auto& slot = m_storageAreasByConnection.add({ &connection, storageMapID }, nullptr).iterator->value;720 m_storageAreasByConnection.add({ connection.uniqueID(), storageMapID }, WTFMove(area)); 721 return; 722 } 723 724 auto& slot = m_storageAreasByConnection.add({ connection.uniqueID(), storageMapID }, nullptr).iterator->value; 717 725 718 726 // FIXME: This should be a message check. … … 722 730 723 731 auto storageArea = transientLocalStorageNamespace->getOrCreateStorageArea(WTFMove(origin)); 724 storageArea->addListener(connection , storageMapID);732 storageArea->addListener(connection.uniqueID(), storageMapID); 725 733 726 734 slot = WTFMove(storageArea); … … 740 748 741 749 // FIXME: This should be a message check. 742 ASSERT(m_storageAreasByConnection.isValidKey({ &connection, storageMapID }));743 744 auto& slot = m_storageAreasByConnection.add({ &connection, storageMapID }, nullptr).iterator->value;750 ASSERT(m_storageAreasByConnection.isValidKey({ connection.uniqueID(), storageMapID })); 751 752 auto& slot = m_storageAreasByConnection.add({ connection.uniqueID(), storageMapID }, nullptr).iterator->value; 745 753 746 754 // FIXME: This should be a message check. … … 748 756 749 757 // FIXME: This should be a message check. 750 ASSERT( &connection== sessionStorageNamespace->allowedConnection());758 ASSERT(connection.uniqueID() == sessionStorageNamespace->allowedConnection()); 751 759 752 760 auto storageArea = sessionStorageNamespace->getOrCreateStorageArea(WTFMove(securityOriginData)); 753 storageArea->addListener(connection , storageMapID);761 storageArea->addListener(connection.uniqueID(), storageMapID); 754 762 755 763 slot = WTFMove(storageArea); … … 758 766 void StorageManager::destroyStorageMap(IPC::Connection& connection, uint64_t storageMapID) 759 767 { 760 std::pair< RefPtr<IPC::Connection>, uint64_t> connectionAndStorageMapIDPair(&connection, storageMapID);768 std::pair<IPC::Connection::UniqueID, uint64_t> connectionAndStorageMapIDPair(connection.uniqueID(), storageMapID); 761 769 762 770 // FIXME: This should be a message check. … … 769 777 } 770 778 771 it->value->removeListener(connection , storageMapID);779 it->value->removeListener(connection.uniqueID(), storageMapID); 772 780 773 781 // Don't remove session storage maps. The web process may reconnect and expect the data to still be around. … … 799 807 800 808 bool quotaError; 801 storageArea->setItem( &connection, sourceStorageAreaID, key, value, urlString, quotaError);809 storageArea->setItem(connection.uniqueID(), sourceStorageAreaID, key, value, urlString, quotaError); 802 810 connection.send(Messages::StorageAreaMap::DidSetItem(storageMapSeed, key, quotaError), storageMapID); 803 811 } … … 811 819 } 812 820 813 storageArea->removeItem( &connection, sourceStorageAreaID, key, urlString);821 storageArea->removeItem(connection.uniqueID(), sourceStorageAreaID, key, urlString); 814 822 connection.send(Messages::StorageAreaMap::DidRemoveItem(storageMapSeed, key), storageMapID); 815 823 } … … 823 831 } 824 832 825 storageArea->clear( &connection, sourceStorageAreaID, urlString);833 storageArea->clear(connection.uniqueID(), sourceStorageAreaID, urlString); 826 834 connection.send(Messages::StorageAreaMap::DidClear(storageMapSeed), storageMapID); 827 835 } … … 831 839 BinarySemaphore semaphore; 832 840 m_queue->dispatch([this, &semaphore] { 833 Vector<std::pair< RefPtr<IPC::Connection>, uint64_t>> connectionAndStorageMapIDPairsToRemove;841 Vector<std::pair<IPC::Connection::UniqueID, uint64_t>> connectionAndStorageMapIDPairsToRemove; 834 842 for (auto& connectionStorageAreaPair : m_storageAreasByConnection) { 835 connectionStorageAreaPair.value->removeListener( *connectionStorageAreaPair.key.first, connectionStorageAreaPair.key.second);843 connectionStorageAreaPair.value->removeListener(connectionStorageAreaPair.key.first, connectionStorageAreaPair.key.second); 836 844 connectionAndStorageMapIDPairsToRemove.append(connectionStorageAreaPair.key); 837 845 } … … 847 855 StorageManager::StorageArea* StorageManager::findStorageArea(IPC::Connection& connection, uint64_t storageMapID) const 848 856 { 849 std::pair<IPC::Connection *, uint64_t> connectionAndStorageMapIDPair(&connection, storageMapID);857 std::pair<IPC::Connection::UniqueID, uint64_t> connectionAndStorageMapIDPair(connection.uniqueID(), storageMapID); 850 858 851 859 if (!m_storageAreasByConnection.isValidKey(connectionAndStorageMapIDPair)) -
trunk/Source/WebKit/UIProcess/WebStorage/StorageManager.h
r232410 r234664 106 106 HashMap<uint64_t, RefPtr<SessionStorageNamespace>> m_sessionStorageNamespaces; 107 107 108 HashMap<std::pair< RefPtr<IPC::Connection>, uint64_t>, RefPtr<StorageArea>> m_storageAreasByConnection;108 HashMap<std::pair<IPC::Connection::UniqueID, uint64_t>, RefPtr<StorageArea>> m_storageAreasByConnection; 109 109 }; 110 110
Note: See TracChangeset
for help on using the changeset viewer.