Commit 59d47873 authored by andersca@apple.com's avatar andersca@apple.com

[WK2] Crash in WebKit::StorageAreaMap::didSetItem()

https://bugs.webkit.org/show_bug.cgi?id=116026

Reviewed by Andreas Kling.

Make sure that we ignore any leftover messages from the UI process after we've reset
the storage map. Achieve this by keeping a seed count in the StorageAreaMap object that's incremented
everytime the map is reset. Associate every storage area change with the seed and ignore any incoming
notification messages from the UI process if the seeds are different.

* Platform/CoreIPC/HandleMessage.h:
(CoreIPC):
(CoreIPC::callMemberFunction):
* UIProcess/Storage/StorageManager.cpp:
(WebKit::StorageManager::getValues):
(WebKit::StorageManager::setItem):
(WebKit::StorageManager::removeItem):
(WebKit::StorageManager::clear):
* UIProcess/Storage/StorageManager.h:
(StorageManager):
* UIProcess/Storage/StorageManager.messages.in:
* WebProcess/Storage/StorageAreaMap.cpp:
(WebKit::StorageAreaMap::StorageAreaMap):
(WebKit::StorageAreaMap::setItem):
(WebKit::StorageAreaMap::removeItem):
(WebKit::StorageAreaMap::clear):
(WebKit::StorageAreaMap::resetValues):
(WebKit::StorageAreaMap::loadValuesIfNeeded):
(WebKit::StorageAreaMap::didGetValues):
(WebKit::StorageAreaMap::didSetItem):
(WebKit::StorageAreaMap::didRemoveItem):
(WebKit::StorageAreaMap::didClear):
(WebKit::StorageAreaMap::applyChange):
* WebProcess/Storage/StorageAreaMap.h:
(StorageAreaMap):
* WebProcess/Storage/StorageAreaMap.messages.in:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@150030 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent 24a5e474
2013-05-13 Anders Carlsson <andersca@apple.com>
[WK2] Crash in WebKit::StorageAreaMap::didSetItem()
https://bugs.webkit.org/show_bug.cgi?id=116026
Reviewed by Andreas Kling.
Make sure that we ignore any leftover messages from the UI process after we've reset
the storage map. Achieve this by keeping a seed count in the StorageAreaMap object that's incremented
everytime the map is reset. Associate every storage area change with the seed and ignore any incoming
notification messages from the UI process if the seeds are different.
* Platform/CoreIPC/HandleMessage.h:
(CoreIPC):
(CoreIPC::callMemberFunction):
* UIProcess/Storage/StorageManager.cpp:
(WebKit::StorageManager::getValues):
(WebKit::StorageManager::setItem):
(WebKit::StorageManager::removeItem):
(WebKit::StorageManager::clear):
* UIProcess/Storage/StorageManager.h:
(StorageManager):
* UIProcess/Storage/StorageManager.messages.in:
* WebProcess/Storage/StorageAreaMap.cpp:
(WebKit::StorageAreaMap::StorageAreaMap):
(WebKit::StorageAreaMap::setItem):
(WebKit::StorageAreaMap::removeItem):
(WebKit::StorageAreaMap::clear):
(WebKit::StorageAreaMap::resetValues):
(WebKit::StorageAreaMap::loadValuesIfNeeded):
(WebKit::StorageAreaMap::didGetValues):
(WebKit::StorageAreaMap::didSetItem):
(WebKit::StorageAreaMap::didRemoveItem):
(WebKit::StorageAreaMap::didClear):
(WebKit::StorageAreaMap::applyChange):
* WebProcess/Storage/StorageAreaMap.h:
(StorageAreaMap):
* WebProcess/Storage/StorageAreaMap.messages.in:
2013-05-13 Jesus Sanchez-Palencia <jesus.palencia@openbossa.org>
[WK2][CoordinatedGraphics] Avoid dispensable calls to WebView::updateViewportSize()
......
......@@ -259,6 +259,18 @@ void callMemberFunction(Connection* connection, const Arguments5<P1, P2, P3, P4,
(object->*function)(connection, args.argument1, args.argument2, args.argument3, args.argument4, args.argument5);
}
template<typename C, typename MF, typename P1, typename P2, typename P3, typename P4, typename P5, typename P6>
void callMemberFunction(Connection* connection, const Arguments6<P1, P2, P3, P4, P5, P6>& args, C* object, MF function)
{
(object->*function)(connection, args.argument1, args.argument2, args.argument3, args.argument4, args.argument5, args.argument6);
}
template<typename C, typename MF, typename P1, typename P2, typename R1>
void callMemberFunction(Connection* connection, const Arguments2<P1, P2>& args, Arguments1<R1>& replyArgs, C* object, MF function)
{
(object->*function)(connection, args.argument1, args.argument2, replyArgs.argument1);
}
template<typename C, typename MF, typename P1, typename R1>
void callMemberFunction(Connection* connection, const Arguments1<P1>& args, Arguments1<R1>& replyArgs, C* object, MF function)
{
......
......@@ -443,7 +443,7 @@ void StorageManager::destroyStorageMap(CoreIPC::Connection* connection, uint64_t
m_storageAreasByConnection.remove(connectionAndStorageMapIDPair);
}
void StorageManager::getValues(CoreIPC::Connection* connection, uint64_t storageMapID, HashMap<String, String>& values)
void StorageManager::getValues(CoreIPC::Connection* connection, uint64_t storageMapID, uint64_t storageMapSeed, HashMap<String, String>& values)
{
StorageArea* storageArea = findStorageArea(connection, storageMapID);
......@@ -452,10 +452,10 @@ void StorageManager::getValues(CoreIPC::Connection* connection, uint64_t storage
values = storageArea->items();
connection->send(Messages::StorageAreaMap::DidGetValues(), storageMapID);
connection->send(Messages::StorageAreaMap::DidGetValues(storageMapSeed), storageMapID);
}
void StorageManager::setItem(CoreIPC::Connection* connection, uint64_t storageMapID, uint64_t sourceStorageAreaID, const String& key, const String& value, const String& urlString)
void StorageManager::setItem(CoreIPC::Connection* connection, uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, const String& key, const String& value, const String& urlString)
{
StorageArea* storageArea = findStorageArea(connection, storageMapID);
......@@ -464,10 +464,10 @@ void StorageManager::setItem(CoreIPC::Connection* connection, uint64_t storageMa
bool quotaError;
storageArea->setItem(connection, sourceStorageAreaID, key, value, urlString, quotaError);
connection->send(Messages::StorageAreaMap::DidSetItem(key, quotaError), storageMapID);
connection->send(Messages::StorageAreaMap::DidSetItem(storageMapSeed, key, quotaError), storageMapID);
}
void StorageManager::removeItem(CoreIPC::Connection* connection, uint64_t storageMapID, uint64_t sourceStorageAreaID, const String& key, const String& urlString)
void StorageManager::removeItem(CoreIPC::Connection* connection, uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, const String& key, const String& urlString)
{
StorageArea* storageArea = findStorageArea(connection, storageMapID);
......@@ -475,10 +475,10 @@ void StorageManager::removeItem(CoreIPC::Connection* connection, uint64_t storag
ASSERT(storageArea);
storageArea->removeItem(connection, sourceStorageAreaID, key, urlString);
connection->send(Messages::StorageAreaMap::DidRemoveItem(key), storageMapID);
connection->send(Messages::StorageAreaMap::DidRemoveItem(storageMapSeed, key), storageMapID);
}
void StorageManager::clear(CoreIPC::Connection* connection, uint64_t storageMapID, uint64_t sourceStorageAreaID, const String& urlString)
void StorageManager::clear(CoreIPC::Connection* connection, uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, const String& urlString)
{
StorageArea* storageArea = findStorageArea(connection, storageMapID);
......@@ -486,7 +486,7 @@ void StorageManager::clear(CoreIPC::Connection* connection, uint64_t storageMapI
ASSERT(storageArea);
storageArea->clear(connection, sourceStorageAreaID, urlString);
connection->send(Messages::StorageAreaMap::DidClear(), storageMapID);
connection->send(Messages::StorageAreaMap::DidClear(storageMapSeed), storageMapID);
}
void StorageManager::createSessionStorageNamespaceInternal(uint64_t storageNamespaceID, CoreIPC::Connection* allowedConnection, unsigned quotaInBytes)
......
......@@ -65,10 +65,11 @@ private:
void createLocalStorageMap(CoreIPC::Connection*, uint64_t storageMapID, uint64_t storageNamespaceID, const SecurityOriginData&);
void createSessionStorageMap(CoreIPC::Connection*, uint64_t storageMapID, uint64_t storageNamespaceID, const SecurityOriginData&);
void destroyStorageMap(CoreIPC::Connection*, uint64_t storageMapID);
void getValues(CoreIPC::Connection*, uint64_t storageMapID, HashMap<String, String>& values);
void setItem(CoreIPC::Connection*, uint64_t storageAreaID, uint64_t sourceStorageAreaID, const String& key, const String& value, const String& urlString);
void removeItem(CoreIPC::Connection*, uint64_t storageMapID, uint64_t sourceStorageAreaID, const String& key, const String& urlString);
void clear(CoreIPC::Connection*, uint64_t storageMapID, uint64_t sourceStorageAreaID, const String& urlString);
void getValues(CoreIPC::Connection*, uint64_t storageMapID, uint64_t storageMapSeed, HashMap<String, String>& values);
void setItem(CoreIPC::Connection*, uint64_t storageAreaID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, const String& key, const String& value, const String& urlString);
void removeItem(CoreIPC::Connection*, uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, const String& key, const String& urlString);
void clear(CoreIPC::Connection*, uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, const String& urlString);
void createSessionStorageNamespaceInternal(uint64_t storageNamespaceID, CoreIPC::Connection* allowedConnection, unsigned quotaInBytes);
void destroySessionStorageNamespaceInternal(uint64_t storageNamespaceID);
......
......@@ -25,9 +25,9 @@ messages -> StorageManager {
CreateSessionStorageMap(uint64_t storageMapID, uint64_t storageNamespaceID, WebKit::SecurityOriginData securityOriginData) WantsConnection
DestroyStorageMap(uint64_t storageMapID) WantsConnection
GetValues(uint64_t storageMapID) -> (WTF::HashMap<WTF::String, WTF::String> values) WantsConnection
GetValues(uint64_t storageMapID, uint64_t storageMapSeed) -> (WTF::HashMap<WTF::String, WTF::String> values) WantsConnection
SetItem(uint64_t storageMapID, uint64_t sourceStorageAreaID, WTF::String key, WTF::String value, WTF::String urlString) WantsConnection
RemoveItem(uint64_t storageMapID, uint64_t sourceStorageAreaID, WTF::String key, WTF::String urlString) WantsConnection
Clear(uint64_t storageMapID, uint64_t sourceStorageAreaID, WTF::String urlString) WantsConnection
SetItem(uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, WTF::String key, WTF::String value, WTF::String urlString) WantsConnection
RemoveItem(uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, WTF::String key, WTF::String urlString) WantsConnection
Clear(uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, WTF::String urlString) WantsConnection
}
......@@ -63,7 +63,9 @@ StorageAreaMap::StorageAreaMap(StorageNamespaceImpl* storageNamespace, PassRefPt
, m_storageNamespaceID(storageNamespace->storageNamespaceID())
, m_quotaInBytes(storageNamespace->quotaInBytes())
, m_securityOrigin(securityOrigin)
, m_currentSeed(0)
, m_hasPendingClear(false)
, m_hasPendingGetValues(false)
{
if (m_storageType == LocalStorage)
WebProcess::shared().connection()->send(Messages::StorageManager::CreateLocalStorageMap(m_storageMapID, storageNamespace->storageNamespaceID(), SecurityOriginData::fromSecurityOrigin(m_securityOrigin.get())), 0);
......@@ -116,7 +118,7 @@ void StorageAreaMap::setItem(Frame* sourceFrame, StorageAreaImpl* sourceArea, co
m_pendingValueChanges.add(key);
WebProcess::shared().connection()->send(Messages::StorageManager::SetItem(m_storageMapID, sourceArea->storageAreaID(), key, value, sourceFrame->document()->url()), 0);
WebProcess::shared().connection()->send(Messages::StorageManager::SetItem(m_storageMapID, sourceArea->storageAreaID(), m_currentSeed, key, value, sourceFrame->document()->url()), 0);
}
void StorageAreaMap::removeItem(WebCore::Frame* sourceFrame, StorageAreaImpl* sourceArea, const String& key)
......@@ -132,7 +134,7 @@ void StorageAreaMap::removeItem(WebCore::Frame* sourceFrame, StorageAreaImpl* so
m_pendingValueChanges.add(key);
WebProcess::shared().connection()->send(Messages::StorageManager::RemoveItem(m_storageMapID, sourceArea->storageAreaID(), key, sourceFrame->document()->url()), 0);
WebProcess::shared().connection()->send(Messages::StorageManager::RemoveItem(m_storageMapID, sourceArea->storageAreaID(), m_currentSeed, key, sourceFrame->document()->url()), 0);
}
void StorageAreaMap::clear(WebCore::Frame* sourceFrame, StorageAreaImpl* sourceArea)
......@@ -141,7 +143,7 @@ void StorageAreaMap::clear(WebCore::Frame* sourceFrame, StorageAreaImpl* sourceA
m_hasPendingClear = true;
m_storageMap = StorageMap::create(m_quotaInBytes);
WebProcess::shared().connection()->send(Messages::StorageManager::Clear(m_storageMapID, sourceArea->storageAreaID(), sourceFrame->document()->url()), 0);
WebProcess::shared().connection()->send(Messages::StorageManager::Clear(m_storageMapID, sourceArea->storageAreaID(), m_currentSeed, sourceFrame->document()->url()), 0);
}
bool StorageAreaMap::contains(const String& key)
......@@ -154,8 +156,11 @@ bool StorageAreaMap::contains(const String& key)
void StorageAreaMap::resetValues()
{
m_storageMap = nullptr;
m_pendingValueChanges.clear();
m_hasPendingClear = false;
m_hasPendingGetValues = false;
m_currentSeed++;
}
void StorageAreaMap::loadValuesIfNeeded()
......@@ -167,22 +172,29 @@ void StorageAreaMap::loadValuesIfNeeded()
// FIXME: This should use a special sendSync flag to indicate that we don't want to process incoming messages while waiting for a reply.
// (This flag does not yet exist). Since loadValuesIfNeeded() ends up being called from within JavaScript code, processing incoming synchronous messages
// could lead to weird reentrency bugs otherwise.
WebProcess::shared().connection()->sendSync(Messages::StorageManager::GetValues(m_storageMapID), Messages::StorageManager::GetValues::Reply(values), 0);
WebProcess::shared().connection()->sendSync(Messages::StorageManager::GetValues(m_storageMapID, m_currentSeed), Messages::StorageManager::GetValues::Reply(values), 0);
m_storageMap = StorageMap::create(m_quotaInBytes);
m_storageMap->importItems(values);
// We want to ignore all changes until we get the DidGetValues message, so treat this as a pending clear.
m_hasPendingClear = true;
// We want to ignore all changes until we get the DidGetValues message.
m_hasPendingGetValues = true;
}
void StorageAreaMap::didGetValues()
void StorageAreaMap::didGetValues(uint64_t storageMapSeed)
{
m_hasPendingClear = false;
if (m_currentSeed != storageMapSeed)
return;
ASSERT(m_hasPendingGetValues);
m_hasPendingGetValues = false;
}
void StorageAreaMap::didSetItem(const String& key, bool quotaError)
void StorageAreaMap::didSetItem(uint64_t storageMapSeed, const String& key, bool quotaError)
{
if (m_currentSeed != storageMapSeed)
return;
ASSERT(m_pendingValueChanges.contains(key));
if (quotaError) {
......@@ -193,15 +205,21 @@ void StorageAreaMap::didSetItem(const String& key, bool quotaError)
m_pendingValueChanges.remove(key);
}
void StorageAreaMap::didRemoveItem(const String& key)
void StorageAreaMap::didRemoveItem(uint64_t storageMapSeed, const String& key)
{
ASSERT(m_pendingValueChanges.contains(key));
if (m_currentSeed != storageMapSeed)
return;
ASSERT(m_pendingValueChanges.contains(key));
m_pendingValueChanges.remove(key);
}
void StorageAreaMap::didClear()
void StorageAreaMap::didClear(uint64_t storageMapSeed)
{
if (m_currentSeed != storageMapSeed)
return;
ASSERT(m_hasPendingClear);
m_hasPendingClear = false;
}
......@@ -224,8 +242,8 @@ void StorageAreaMap::applyChange(const String& key, const String& newValue)
{
ASSERT(m_storageMap->hasOneRef());
// There's a clear pending, we don't want any changes until we've gotten the DidClear message.
if (m_hasPendingClear)
// There's a clear pending or getValues pending we don't want to apply any changes until we get the corresponding DidClear/DidGetValues messages.
if (m_hasPendingClear || m_hasPendingGetValues)
return;
if (!key) {
......
......@@ -65,10 +65,10 @@ private:
// CoreIPC::MessageReceiver
virtual void didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&) OVERRIDE;
void didGetValues();
void didSetItem(const String& key, bool quotaError);
void didRemoveItem(const String& key);
void didClear();
void didGetValues(uint64_t storageMapSeed);
void didSetItem(uint64_t storageMapSeed, const String& key, bool quotaError);
void didRemoveItem(uint64_t storageMapSeed, const String& key);
void didClear(uint64_t storageMapSeed);
void resetValues();
void loadValuesIfNeeded();
......@@ -89,7 +89,10 @@ private:
RefPtr<WebCore::SecurityOrigin> m_securityOrigin;
RefPtr<WebCore::StorageMap> m_storageMap;
uint64_t m_currentSeed;
bool m_hasPendingClear;
bool m_hasPendingGetValues;
HashCountedSet<String> m_pendingValueChanges;
};
......
......@@ -21,10 +21,10 @@
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
messages -> StorageAreaMap {
DidGetValues()
DidSetItem(WTF::String key, bool quotaException)
DidRemoveItem(WTF::String key)
DidClear()
DidGetValues(uint64_t storageMapSeed)
DidSetItem(uint64_t storageMapSeed, WTF::String key, bool quotaException)
DidRemoveItem(uint64_t storageMapSeed, WTF::String key)
DidClear(uint64_t storageMapSeed)
DispatchStorageEvent(uint64_t sourceStorageAreaID, WTF::String key, WTF::String oldValue, WTF::String newValue, WTF::String urlString)
}
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment