[SCM] WebKit Debian packaging branch, webkit-1.1, updated. upstream/1.1.19-706-ge5415e9
ggaren at apple.com
ggaren at apple.com
Thu Feb 4 21:24:13 UTC 2010
The following commit has been merged in the webkit-1.1 branch:
commit d2d8815199de6f97fefb0cd89c671fcb35a9d2bc
Author: ggaren at apple.com <ggaren at apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date: Fri Jan 22 03:25:16 2010 +0000
2010-01-21 Geoffrey Garen <ggaren at apple.com>
Reviewed by Sam Weinig.
Fixed <rdar://problem/7562574> ASSERT in WebCore::removeWrapper() at the
end of run-webkit-tests
This was an ASSERT-only bug, introduced by isolated worlds, which
created the novelty of a wrapper that might outlive its wrapper cache.
When a wrapper outlived its wrapper cache, both the wrapper's destructor
and the wrapper cache's destructor would claim to have uncached the wrapper,
causing an ASSERT to fire.
The solution is to distinguish between operations that logically add and
remove cache entries, and operations that delete whole caches. We track
when a cache entry is logically added, and when it's logically removed,
independent of whether the actual cache still exists.
* bindings/js/JSDOMBinding.cpp:
(WebCore::willCacheWrapper):
(WebCore::didUncacheWrapper): New names for these functions to help
explain what they track.
(WebCore::DOMWrapperWorld::~DOMWrapperWorld): Don't claim to uncache
all the wrappers in the world; we're deleting the cache, not managing its
entries.
(WebCore::cacheDOMObjectWrapper):
(WebCore::forgetDOMObject):
(WebCore::forgetDOMNode):
(WebCore::cacheDOMNodeWrapper):
(WebCore::forgetAllDOMNodesForDocument):
(WebCore::forgetWorldOfDOMNodesForDocument):
(WebCore::takeWrappers):
(WebCore::updateDOMNodeDocument): Updated for renames.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@53668 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
index f013b7c..34dfe47 100644
--- a/WebCore/ChangeLog
+++ b/WebCore/ChangeLog
@@ -1,3 +1,40 @@
+2010-01-21 Geoffrey Garen <ggaren at apple.com>
+
+ Reviewed by Sam Weinig.
+
+ Fixed <rdar://problem/7562574> ASSERT in WebCore::removeWrapper() at the
+ end of run-webkit-tests
+
+ This was an ASSERT-only bug, introduced by isolated worlds, which
+ created the novelty of a wrapper that might outlive its wrapper cache.
+
+ When a wrapper outlived its wrapper cache, both the wrapper's destructor
+ and the wrapper cache's destructor would claim to have uncached the wrapper,
+ causing an ASSERT to fire.
+
+ The solution is to distinguish between operations that logically add and
+ remove cache entries, and operations that delete whole caches. We track
+ when a cache entry is logically added, and when it's logically removed,
+ independent of whether the actual cache still exists.
+
+ * bindings/js/JSDOMBinding.cpp:
+ (WebCore::willCacheWrapper):
+ (WebCore::didUncacheWrapper): New names for these functions to help
+ explain what they track.
+
+ (WebCore::DOMWrapperWorld::~DOMWrapperWorld): Don't claim to uncache
+ all the wrappers in the world; we're deleting the cache, not managing its
+ entries.
+
+ (WebCore::cacheDOMObjectWrapper):
+ (WebCore::forgetDOMObject):
+ (WebCore::forgetDOMNode):
+ (WebCore::cacheDOMNodeWrapper):
+ (WebCore::forgetAllDOMNodesForDocument):
+ (WebCore::forgetWorldOfDOMNodesForDocument):
+ (WebCore::takeWrappers):
+ (WebCore::updateDOMNodeDocument): Updated for renames.
+
2010-01-21 Nikolas Zimmermann <nzimmermann at rim.com>
Reviewed by Sam Weinig.
diff --git a/WebCore/bindings/js/JSDOMBinding.cpp b/WebCore/bindings/js/JSDOMBinding.cpp
index cfa6761..531ade7 100644
--- a/WebCore/bindings/js/JSDOMBinding.cpp
+++ b/WebCore/bindings/js/JSDOMBinding.cpp
@@ -91,30 +91,21 @@ inline JSWrapperCache* Document::getWrapperCache(DOMWrapperWorld* world)
return createWrapperCache(world);
}
-// For debugging, keep a set of wrappers currently registered, and check that
-// all are unregistered before they are destroyed. This has helped us fix at
-// least one bug.
+// For debugging, keep a set of wrappers currently cached, and check that
+// all are uncached before they are destroyed. This helps us catch bugs like:
+// - wrappers being deleted without being removed from the cache
+// - wrappers being cached twice
-static void addWrapper(DOMObject* wrapper);
-static void removeWrapper(DOMObject* wrapper);
-static void removeWrappers(const JSWrapperCache& wrappers);
-static void removeWrappers(const DOMObjectWrapperMap& wrappers);
+static void willCacheWrapper(DOMObject* wrapper);
+static void didUncacheWrapper(DOMObject* wrapper);
#ifdef NDEBUG
-static inline void addWrapper(DOMObject*)
+static inline void willCacheWrapper(DOMObject*)
{
}
-static inline void removeWrapper(DOMObject*)
-{
-}
-
-static inline void removeWrappers(const JSWrapperCache&)
-{
-}
-
-static inline void removeWrappers(const DOMObjectWrapperMap&)
+static inline void didUncacheWrapper(DOMObject*)
{
}
@@ -131,13 +122,13 @@ static HashSet<DOMObject*>& wrapperSet()
#endif
}
-static void addWrapper(DOMObject* wrapper)
+static void willCacheWrapper(DOMObject* wrapper)
{
ASSERT(!wrapperSet().contains(wrapper));
wrapperSet().add(wrapper);
}
-static void removeWrapper(DOMObject* wrapper)
+static void didUncacheWrapper(DOMObject* wrapper)
{
if (!wrapper)
return;
@@ -145,20 +136,6 @@ static void removeWrapper(DOMObject* wrapper)
wrapperSet().remove(wrapper);
}
-static void removeWrappers(const JSWrapperCache& wrappers)
-{
- JSWrapperCache::const_iterator wrappersEnd = wrappers.uncheckedEnd();
- for (JSWrapperCache::const_iterator it = wrappers.uncheckedBegin(); it != wrappersEnd; ++it)
- removeWrapper(it->second);
-}
-
-static inline void removeWrappers(const DOMObjectWrapperMap& wrappers)
-{
- DOMObjectWrapperMap::const_iterator wrappersEnd = wrappers.uncheckedEnd();
- for (DOMObjectWrapperMap::const_iterator it = wrappers.uncheckedBegin(); it != wrappersEnd; ++it)
- removeWrapper(it->second);
-}
-
DOMObject::~DOMObject()
{
ASSERT(!wrapperSet().contains(this));
@@ -178,8 +155,6 @@ DOMWrapperWorld::~DOMWrapperWorld()
ASSERT(clientData);
static_cast<WebCoreJSClientData*>(clientData)->forgetWorld(this);
- removeWrappers(m_wrappers);
-
for (HashSet<Document*>::iterator iter = documentsWithWrappers.begin(); iter != documentsWithWrappers.end(); ++iter)
forgetWorldOfDOMNodesForDocument(*iter, this);
}
@@ -276,7 +251,7 @@ DOMObject* getCachedDOMObjectWrapper(JSC::ExecState* exec, void* objectHandle)
void cacheDOMObjectWrapper(JSC::ExecState* exec, void* objectHandle, DOMObject* wrapper)
{
- addWrapper(wrapper);
+ willCacheWrapper(wrapper);
DOMObjectWrapperMapFor(exec).set(objectHandle, wrapper);
}
@@ -309,15 +284,17 @@ void forgetDOMObject(DOMObject* wrapper, void* objectHandle)
ASSERT(clientData);
DOMObjectWrapperMap& wrappers = static_cast<WebCoreJSClientData*>(clientData)->normalWorld()->m_wrappers;
if (wrappers.uncheckedRemove(objectHandle, wrapper)) {
- removeWrapper(wrapper);
+ didUncacheWrapper(wrapper);
return;
}
+ // We can't guarantee that an wrapper is in the cache when it uncaches itself,
+ // since a new wrapper may have been allocated before the object's destrcutor ran.
for (JSGlobalDataWorldIterator worldIter(globalData); worldIter; ++worldIter) {
if (worldIter->m_wrappers.uncheckedRemove(objectHandle, wrapper))
break;
}
- removeWrapper(wrapper);
+ didUncacheWrapper(wrapper);
}
void forgetDOMNode(JSNode* wrapper, Node* node, Document* document)
@@ -327,22 +304,24 @@ void forgetDOMNode(JSNode* wrapper, Node* node, Document* document)
return;
}
+ // We can't guarantee that an wrapper is in the cache when it uncaches itself,
+ // since a new wrapper may have been allocated before the object's destrcutor ran.
JSWrapperCacheMap& wrapperCacheMap = document->wrapperCacheMap();
for (JSWrapperCacheMap::iterator wrappersIter = wrapperCacheMap.begin(); wrappersIter != wrapperCacheMap.end(); ++wrappersIter) {
if (wrappersIter->second->uncheckedRemove(node, wrapper))
break;
}
- removeWrapper(wrapper);
+ didUncacheWrapper(wrapper);
}
void cacheDOMNodeWrapper(JSC::ExecState* exec, Document* document, Node* node, JSNode* wrapper)
{
if (!document) {
- addWrapper(wrapper);
+ willCacheWrapper(wrapper);
DOMObjectWrapperMapFor(exec).set(node, wrapper);
return;
}
- addWrapper(wrapper);
+ willCacheWrapper(wrapper);
document->getWrapperCache(currentWorld(exec))->set(node, wrapper);
}
@@ -352,9 +331,7 @@ void forgetAllDOMNodesForDocument(Document* document)
JSWrapperCacheMap& wrapperCacheMap = document->wrapperCacheMap();
JSWrapperCacheMap::const_iterator wrappersMapEnd = wrapperCacheMap.end();
for (JSWrapperCacheMap::const_iterator wrappersMapIter = wrapperCacheMap.begin(); wrappersMapIter != wrappersMapEnd; ++wrappersMapIter) {
- JSWrapperCache* wrappers = wrappersMapIter->second;
- removeWrappers(*wrappers);
- delete wrappers;
+ delete wrappersMapIter->second;
wrappersMapIter->first->forgetDocument(document);
}
}
@@ -363,7 +340,6 @@ void forgetWorldOfDOMNodesForDocument(Document* document, DOMWrapperWorld* world
{
JSWrapperCache* wrappers = document->wrapperCacheMap().take(world);
ASSERT(wrappers); // 'world' should only know about 'document' if 'document' knows about 'world'!
- removeWrappers(*wrappers);
delete wrappers;
}
@@ -488,7 +464,7 @@ static inline void takeWrappers(Node* node, Document* document, WrapperSet& wrap
JSWrapperCacheMap& wrapperCacheMap = document->wrapperCacheMap();
for (JSWrapperCacheMap::iterator iter = wrapperCacheMap.begin(); iter != wrapperCacheMap.end(); ++iter) {
if (JSNode* wrapper = iter->second->take(node)) {
- removeWrapper(wrapper);
+ didUncacheWrapper(wrapper);
wrapperSet.append(WrapperAndWorld(wrapper, iter->first));
}
}
@@ -496,7 +472,7 @@ static inline void takeWrappers(Node* node, Document* document, WrapperSet& wrap
for (JSGlobalDataWorldIterator worldIter(JSDOMWindow::commonJSGlobalData()); worldIter; ++worldIter) {
DOMWrapperWorld* world = *worldIter;
if (JSNode* wrapper = static_cast<JSNode*>(world->m_wrappers.take(node))) {
- removeWrapper(wrapper);
+ didUncacheWrapper(wrapper);
wrapperSet.append(WrapperAndWorld(wrapper, world));
}
}
@@ -512,11 +488,11 @@ void updateDOMNodeDocument(Node* node, Document* oldDocument, Document* newDocum
for (unsigned i = 0; i < wrapperSet.size(); ++i) {
JSNode* wrapper = wrapperSet[i].first;
+ willCacheWrapper(wrapper);
if (newDocument)
newDocument->getWrapperCache(wrapperSet[i].second)->set(node, wrapper);
else
wrapperSet[i].second->m_wrappers.set(node, wrapper);
- addWrapper(wrapper);
}
}
--
WebKit Debian packaging
More information about the Pkg-webkit-commits
mailing list