- Revision
- 119331
- Author
- [email protected]
- Date
- 2012-06-02 13:29:47 -0700 (Sat, 02 Jun 2012)
Log Message
Source/WebCore: Prevent CachedRawResource from sending the same data
chunk multiple times.
https://bugs.webkit.org/show_bug.cgi?id=78810
Reviewed by Adam Barth.
If a CachedRawResource receives data while a CachedRawResourceCallback
timer is active, the incremental data will be sent to the client, followed
but all data received so far, resulting in invalid data. Resolving this adds
complexity to CachedRawResource and requires making a few more CachedResource
functions virtual, so push the callback logic into CachedResource where it can
be implemented more cleanly.
Test: inspector/debugger/script-formatter-console.html
should no longer be flaky.
* loader/cache/CachedRawResource.cpp: CachedRawResourceCallback renamed and moved to CachedResource.
(WebCore::CachedRawResource::didAddClient): More or less the same as sendCallbacks() was.
* loader/cache/CachedRawResource.h:
* loader/cache/CachedResource.cpp:
(WebCore::CachedResource::addClient): Check the return value of addClientToSet() to determine whether
or not to call didAddClient.
(WebCore::CachedResource::didAddClient): May be called during addClient(), or may be called on a timer.
If called on a timer, move the client between sets.
(WebCore::CachedResource::addClientToSet): Determine whether didAddClient() can be called synchronously and
return true if it can.
(WebCore::CachedResource::removeClient): Ensure we cancel pending callbacks if necessary.
(WebCore::CachedResource::CachedResourceCallback::CachedResourceCallback): Renamed and moved from CachedRawResource.
* loader/cache/CachedResource.h:
(WebCore::CachedResource::hasClients): Check m_clientsAwaitingCallback as well as m_clients.
(WebCore::CachedResource::CachedResourceCallback::schedule):
(WebCore::CachedResource::hasClient): Helper for calling contains() on both m_clientsAwaitingCallback and m_clients.
LayoutTests: inspector/debugger/script-formatter-console.html should
no longer be flaky.
https://bugs.webkit.org/show_bug.cgi?id=78810
Reviewed by Adam Barth.
* platform/chromium/test_expectations.txt:
Modified Paths
Diff
Modified: releases/WebKitGTK/webkit-1.8/LayoutTests/ChangeLog (119330 => 119331)
--- releases/WebKitGTK/webkit-1.8/LayoutTests/ChangeLog 2012-06-02 19:23:25 UTC (rev 119330)
+++ releases/WebKitGTK/webkit-1.8/LayoutTests/ChangeLog 2012-06-02 20:29:47 UTC (rev 119331)
@@ -1,3 +1,13 @@
+2012-02-22 Nate Chapin <[email protected]>
+
+ inspector/debugger/script-formatter-console.html should
+ no longer be flaky.
+ https://bugs.webkit.org/show_bug.cgi?id=78810
+
+ Reviewed by Adam Barth.
+
+ * platform/chromium/test_expectations.txt:
+
2012-04-05 Adam Klein <[email protected]>
Crash in MutationObservers due to an invalid HashSet iterator
Modified: releases/WebKitGTK/webkit-1.8/LayoutTests/platform/chromium/test_expectations.txt (119330 => 119331)
--- releases/WebKitGTK/webkit-1.8/LayoutTests/platform/chromium/test_expectations.txt 2012-06-02 19:23:25 UTC (rev 119330)
+++ releases/WebKitGTK/webkit-1.8/LayoutTests/platform/chromium/test_expectations.txt 2012-06-02 20:29:47 UTC (rev 119331)
@@ -715,8 +715,7 @@
BUGWK75916 SKIP LINUX : http/tests/inspector/resource-tree/resource-tree-document-url.html = PASS TEXT TIMEOUT
-BUGWK78810 LINUX WIN RELEASE : inspector/debugger/script-formatter-console.html = PASS TEXT
-BUGWK78810 LINUX WIN DEBUG SLOW : inspector/debugger/script-formatter-console.html = PASS TEXT
+BUGWK78810 LINUX WIN DEBUG SLOW : inspector/debugger/script-formatter-console.html = PASS
// -----------------------------------------------------------------
// Editing tests
Modified: releases/WebKitGTK/webkit-1.8/Source/WebCore/ChangeLog (119330 => 119331)
--- releases/WebKitGTK/webkit-1.8/Source/WebCore/ChangeLog 2012-06-02 19:23:25 UTC (rev 119330)
+++ releases/WebKitGTK/webkit-1.8/Source/WebCore/ChangeLog 2012-06-02 20:29:47 UTC (rev 119331)
@@ -1,3 +1,38 @@
+2012-02-22 Nate Chapin <[email protected]>
+
+ Prevent CachedRawResource from sending the same data
+ chunk multiple times.
+ https://bugs.webkit.org/show_bug.cgi?id=78810
+
+ Reviewed by Adam Barth.
+
+ If a CachedRawResource receives data while a CachedRawResourceCallback
+ timer is active, the incremental data will be sent to the client, followed
+ but all data received so far, resulting in invalid data. Resolving this adds
+ complexity to CachedRawResource and requires making a few more CachedResource
+ functions virtual, so push the callback logic into CachedResource where it can
+ be implemented more cleanly.
+
+ Test: inspector/debugger/script-formatter-console.html
+ should no longer be flaky.
+
+ * loader/cache/CachedRawResource.cpp: CachedRawResourceCallback renamed and moved to CachedResource.
+ (WebCore::CachedRawResource::didAddClient): More or less the same as sendCallbacks() was.
+ * loader/cache/CachedRawResource.h:
+ * loader/cache/CachedResource.cpp:
+ (WebCore::CachedResource::addClient): Check the return value of addClientToSet() to determine whether
+ or not to call didAddClient.
+ (WebCore::CachedResource::didAddClient): May be called during addClient(), or may be called on a timer.
+ If called on a timer, move the client between sets.
+ (WebCore::CachedResource::addClientToSet): Determine whether didAddClient() can be called synchronously and
+ return true if it can.
+ (WebCore::CachedResource::removeClient): Ensure we cancel pending callbacks if necessary.
+ (WebCore::CachedResource::CachedResourceCallback::CachedResourceCallback): Renamed and moved from CachedRawResource.
+ * loader/cache/CachedResource.h:
+ (WebCore::CachedResource::hasClients): Check m_clientsAwaitingCallback as well as m_clients.
+ (WebCore::CachedResource::CachedResourceCallback::schedule):
+ (WebCore::CachedResource::hasClient): Helper for calling contains() on both m_clientsAwaitingCallback and m_clients.
+
2012-04-05 Adam Klein <[email protected]>
Crash in MutationObservers due to an invalid HashSet iterator
Modified: releases/WebKitGTK/webkit-1.8/Source/WebCore/loader/cache/CachedRawResource.cpp (119330 => 119331)
--- releases/WebKitGTK/webkit-1.8/Source/WebCore/loader/cache/CachedRawResource.cpp 2012-06-02 19:23:25 UTC (rev 119330)
+++ releases/WebKitGTK/webkit-1.8/Source/WebCore/loader/cache/CachedRawResource.cpp 2012-06-02 20:29:47 UTC (rev 119331)
@@ -68,77 +68,20 @@
CachedResource::data(m_data, allDataReceived);
}
-class CachedRawResourceCallback {
-public:
- static CachedRawResourceCallback* schedule(CachedRawResource* resource, CachedRawResourceClient* client)
- {
- return new CachedRawResourceCallback(resource, client);
- }
-
- void cancel()
- {
- if (m_callbackTimer.isActive())
- m_callbackTimer.stop();
- }
-
-private:
- CachedRawResourceCallback(CachedRawResource* resource, CachedRawResourceClient* client)
- : m_resource(resource)
- , m_client(client)
- , m_callbackTimer(this, &CachedRawResourceCallback::timerFired)
- {
- m_callbackTimer.startOneShot(0);
- }
-
- void timerFired(Timer<CachedRawResourceCallback>*)
- {
- m_resource->sendCallbacks(m_client);
- }
- CachedResourceHandle<CachedRawResource> m_resource;
- CachedRawResourceClient* m_client;
- Timer<CachedRawResourceCallback> m_callbackTimer;
-};
-
-void CachedRawResource::sendCallbacks(CachedRawResourceClient* c)
+void CachedRawResource::didAddClient(CachedResourceClient* c)
{
- if (!m_clientsAwaitingCallback.contains(c))
+ if (m_response.isNull() || !hasClient(c))
return;
- m_clientsAwaitingCallback.remove(c);
- c->responseReceived(this, m_response);
- if (!m_clients.contains(c) || !m_data)
+ CachedRawResourceClient* client = static_cast<CachedRawResourceClient*>(c);
+ client->responseReceived(this, m_response);
+ if (!hasClient(c) || !m_data)
return;
- c->dataReceived(this, m_data->data(), m_data->size());
- if (!m_clients.contains(c) || isLoading())
+ client->dataReceived(this, m_data->data(), m_data->size());
+ if (!hasClient(c))
return;
- c->notifyFinished(this);
+ CachedResource::didAddClient(client);
}
-void CachedRawResource::didAddClient(CachedResourceClient* c)
-{
- if (m_response.isNull())
- return;
-
- // CachedRawResourceClients (especially XHRs) do crazy things if an asynchronous load returns
- // synchronously (e.g., scripts may not have set all the state they need to handle the load).
- // Therefore, rather than immediately sending callbacks on a cache hit like other CachedResources,
- // we schedule the callbacks and ensure we never finish synchronously.
- // FIXME: We also use an async callback on 304 because we don't have sufficient information to
- // know whether we are receiving new clients because of revalidation or pure reuse. Perhaps move
- // this logic to CachedResource?
- CachedRawResourceClient* client = static_cast<CachedRawResourceClient*>(c);
- ASSERT(!m_clientsAwaitingCallback.contains(client));
- m_clientsAwaitingCallback.add(client, adoptPtr(CachedRawResourceCallback::schedule(this, client)));
-}
-
-void CachedRawResource::removeClient(CachedResourceClient* c)
-{
- OwnPtr<CachedRawResourceCallback> callback = m_clientsAwaitingCallback.take(static_cast<CachedRawResourceClient*>(c));
- if (callback)
- callback->cancel();
- callback.clear();
- CachedResource::removeClient(c);
-}
-
void CachedRawResource::allClientsRemoved()
{
if (m_loader)
Modified: releases/WebKitGTK/webkit-1.8/Source/WebCore/loader/cache/CachedRawResource.h (119330 => 119331)
--- releases/WebKitGTK/webkit-1.8/Source/WebCore/loader/cache/CachedRawResource.h 2012-06-02 19:23:25 UTC (rev 119330)
+++ releases/WebKitGTK/webkit-1.8/Source/WebCore/loader/cache/CachedRawResource.h 2012-06-02 20:29:47 UTC (rev 119331)
@@ -43,10 +43,7 @@
unsigned long identifier() const { return m_identifier; }
bool canReuse() const;
- void sendCallbacks(CachedRawResourceClient*);
- virtual void removeClient(CachedResourceClient*);
-
private:
virtual void didAddClient(CachedResourceClient*);
virtual void data(PassRefPtr<SharedBuffer> data, bool allDataReceived);
@@ -62,7 +59,6 @@
#endif
unsigned long m_identifier;
- HashMap<CachedRawResourceClient*, OwnPtr<CachedRawResourceCallback> > m_clientsAwaitingCallback;
};
Modified: releases/WebKitGTK/webkit-1.8/Source/WebCore/loader/cache/CachedResource.cpp (119330 => 119331)
--- releases/WebKitGTK/webkit-1.8/Source/WebCore/loader/cache/CachedResource.cpp 2012-06-02 19:23:25 UTC (rev 119330)
+++ releases/WebKitGTK/webkit-1.8/Source/WebCore/loader/cache/CachedResource.cpp 2012-06-02 20:29:47 UTC (rev 119331)
@@ -366,17 +366,21 @@
void CachedResource::addClient(CachedResourceClient* client)
{
- addClientToSet(client);
- didAddClient(client);
+ if (addClientToSet(client))
+ didAddClient(client);
}
void CachedResource::didAddClient(CachedResourceClient* c)
{
+ if (m_clientsAwaitingCallback.contains(c)) {
+ m_clients.add(c);
+ m_clientsAwaitingCallback.remove(c);
+ }
if (!isLoading())
c->notifyFinished(this);
}
-void CachedResource::addClientToSet(CachedResourceClient* client)
+bool CachedResource::addClientToSet(CachedResourceClient* client)
{
ASSERT(!isPurgeable());
@@ -390,13 +394,32 @@
}
if (!hasClients() && inCache())
memoryCache()->addToLiveResourcesSize(this);
+
+ if (m_type == RawResource && !m_response.isNull() && !m_proxyResource) {
+ // Certain resources (especially XHRs) do crazy things if an asynchronous load returns
+ // synchronously (e.g., scripts may not have set all the state they need to handle the load).
+ // Therefore, rather than immediately sending callbacks on a cache hit like other CachedResources,
+ // we schedule the callbacks and ensure we never finish synchronously.
+ ASSERT(!m_clientsAwaitingCallback.contains(client));
+ m_clientsAwaitingCallback.add(client, CachedResourceCallback::schedule(this, client));
+ return false;
+ }
+
m_clients.add(client);
+ return true;
}
void CachedResource::removeClient(CachedResourceClient* client)
{
- ASSERT(m_clients.contains(client));
- m_clients.remove(client);
+ OwnPtr<CachedResourceCallback> callback = m_clientsAwaitingCallback.take(client);
+ if (callback) {
+ ASSERT(!m_clients.contains(client));
+ callback->cancel();
+ callback.clear();
+ } else {
+ ASSERT(m_clients.contains(client));
+ m_clients.remove(client);
+ }
if (canDelete() && !inCache())
delete this;
@@ -725,4 +748,24 @@
m_loadPriority = loadPriority;
}
+
+CachedResource::CachedResourceCallback::CachedResourceCallback(CachedResource* resource, CachedResourceClient* client)
+ : m_resource(resource)
+ , m_client(client)
+ , m_callbackTimer(this, &CachedResourceCallback::timerFired)
+{
+ m_callbackTimer.startOneShot(0);
}
+
+void CachedResource::CachedResourceCallback::cancel()
+{
+ if (m_callbackTimer.isActive())
+ m_callbackTimer.stop();
+}
+
+void CachedResource::CachedResourceCallback::timerFired(Timer<CachedResourceCallback>*)
+{
+ m_resource->didAddClient(m_client);
+}
+
+}
Modified: releases/WebKitGTK/webkit-1.8/Source/WebCore/loader/cache/CachedResource.h (119330 => 119331)
--- releases/WebKitGTK/webkit-1.8/Source/WebCore/loader/cache/CachedResource.h 2012-06-02 19:23:25 UTC (rev 119330)
+++ releases/WebKitGTK/webkit-1.8/Source/WebCore/loader/cache/CachedResource.h 2012-06-02 20:29:47 UTC (rev 119331)
@@ -31,6 +31,7 @@
#include "ResourceLoadPriority.h"
#include "ResourceRequest.h"
#include "ResourceResponse.h"
+#include "Timer.h"
#include <wtf/HashCountedSet.h>
#include <wtf/HashSet.h>
#include <wtf/OwnPtr.h>
@@ -110,8 +111,8 @@
void setLoadPriority(ResourceLoadPriority);
void addClient(CachedResourceClient*);
- virtual void removeClient(CachedResourceClient*);
- bool hasClients() const { return !m_clients.isEmpty(); }
+ void removeClient(CachedResourceClient*);
+ bool hasClients() const { return !m_clients.isEmpty() || !m_clientsAwaitingCallback.isEmpty(); }
void deleteIfPossible();
enum PreloadResult {
@@ -254,6 +255,22 @@
HashCountedSet<CachedResourceClient*> m_clients;
+ class CachedResourceCallback {
+ public:
+ static PassOwnPtr<CachedResourceCallback> schedule(CachedResource* resource, CachedResourceClient* client) { return adoptPtr(new CachedResourceCallback(resource, client)); }
+ void cancel();
+ private:
+ CachedResourceCallback(CachedResource*, CachedResourceClient*);
+ void timerFired(Timer<CachedResourceCallback>*);
+
+ CachedResource* m_resource;
+ CachedResourceClient* m_client;
+ Timer<CachedResourceCallback> m_callbackTimer;
+ };
+ HashMap<CachedResourceClient*, OwnPtr<CachedResourceCallback> > m_clientsAwaitingCallback;
+
+ bool hasClient(CachedResourceClient* client) { return m_clients.contains(client) || m_clientsAwaitingCallback.contains(client); }
+
ResourceRequest m_resourceRequest;
String m_accept;
RefPtr<SubresourceLoader> m_loader;
@@ -267,7 +284,7 @@
OwnPtr<PurgeableBuffer> m_purgeableData;
private:
- void addClientToSet(CachedResourceClient*);
+ bool addClientToSet(CachedResourceClient*);
virtual PurgePriority purgePriority() const { return PurgeDefault; }
