Skip to content

GitLab

T
tangled
Commit 62bb97b2 authored by hiroshige's avatar hiroshige Committed by Commit bot
Browse files
Options

Mark ResourceClient/ImageResourceObserver finished just before notifying 

Previously, all clients (except for awaiting ones) and observers are marked
finished just after checkNotify() is called.
However, it is possible that we have clients in |m_client| newly added during
ResourceClient::notifyFinished(), and such new clients shouldn't be marked
finished without calling notifyFinished().

This CL marks a client/observer just before notifyFinished()/
imageNotifyFinished() is called for the client/observer, and removes
markClientsAndObserversFinished() methods.

This CL also introduces MarkFinishedOption for checkNotify() for multipart
images: In multipart images we call notifyFinished()/imageNotifyFinished()
without marking clients/observers finished when the first part ends.

BUG=633696

Review-Url: https://codereview.chromium.org/2210473002
Cr-Commit-Position: refs/heads/master@{#414423}
parent c54188d2
Hide whitespace changes
Inline Side-by-side
Showing with 104 additions and 28 deletions
third_party/WebKit/LayoutTests/http/tests/cache/resources/etag-200.php 0 → 100644
View file @ 62bb97b2
<?
// Returns response headers that cause revalidation, and returns 200 for
// revalidating requests to test failed revalidation.
header('ETag: foo');
header('Cache-control: max-age=0');
?>
foo
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-failed-revalidation.html 0 → 100644
View file @ 62bb97b2
<html>
<head>
<script src="../resources/testharness.js"></script>
<script src="../resources/testharnessreport.js"></script>
<script>
async_test(function(t) {
const url = 'resources/etag-200.php?' +
Math.floor(100000000 * Math.random());
const xhr1 = new XMLHttpRequest();
xhr1.onload = function() {
assert_equals(xhr1.status, 200);
const xhr2 = new XMLHttpRequest();
xhr2.onload = function() {
assert_equals(xhr2.status, 200);
t.done();
};
xhr2.open("GET", url, true);
xhr2.send();
};
xhr1.open("GET", url, true);
xhr1.send();
}, "onload event must be invoked for failed revalidation when XHR's " +
"onload handler initiates a new XHR to the same URL.");
</script>
</head>
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-successful-revalidation.html 0 → 100644
View file @ 62bb97b2
<html>
<head>
<script src="../resources/testharness.js"></script>
<script src="../resources/testharnessreport.js"></script>
<script>
async_test(function(t) {
const url = 'resources/etag.php?' + Math.floor(100000000 * Math.random());
const xhr1 = new XMLHttpRequest();
xhr1.onload = function() {
assert_equals(xhr1.status, 200);
const xhr2 = new XMLHttpRequest();
xhr2.onload = function() {
assert_equals(xhr2.status, 200);
t.done();
};
xhr2.open("GET", url, true);
xhr2.send();
};
xhr1.open("GET", url, true);
xhr1.send();
}, "onload event must be invoked for successful revalidation when XHR's " +
"onload handler initiates a new XHR to the same URL.");
</script>
</head>
third_party/WebKit/Source/core/fetch/ImageResource.cpp
View file @ 62bb97b2
...@@ -94,26 +94,30 @@ DEFINE_TRACE(ImageResource) ...@@ -94,26 +94,30 @@ DEFINE_TRACE(ImageResource)
} }
void ImageResource::checkNotify() void ImageResource::checkNotify()
{
notifyObserversInternal(MarkFinishedOption::ShouldMarkFinished);
Resource::checkNotify();
}
void ImageResource::notifyObserversInternal(MarkFinishedOption markFinishedOption)
{ {
if (isLoading()) if (isLoading())
return; return;
ImageResourceObserverWalker walker(m_observers); ImageResourceObserverWalker walker(m_observers);
while (auto* observer = walker.next()) { while (auto* observer = walker.next()) {
if (markFinishedOption == MarkFinishedOption::ShouldMarkFinished)
markObserverFinished(observer);
observer->imageNotifyFinished(this); observer->imageNotifyFinished(this);
} }
Resource::checkNotify();
} }
void ImageResource::markClientsAndObserversFinished() void ImageResource::markObserverFinished(ImageResourceObserver* observer)
{ {
HashCountedSet<ImageResourceObserver*> observers; if (m_observers.contains(observer)) {
m_observers.swap(observers); m_finishedObservers.add(observer);
for (const auto& it : observers) m_observers.remove(observer);
m_finishedObservers.add(it.key, it.value); }
Resource::markClientsAndObserversFinished();
} }
void ImageResource::didAddClient(ResourceClient* client) void ImageResource::didAddClient(ResourceClient* client)
...@@ -146,11 +150,8 @@ void ImageResource::addObserver(ImageResourceObserver* observer) ...@@ -146,11 +150,8 @@ void ImageResource::addObserver(ImageResourceObserver* observer)
} }
if (isLoaded()) { if (isLoaded()) {
markObserverFinished(observer);
observer->imageNotifyFinished(this); observer->imageNotifyFinished(this);
if (m_observers.contains(observer)) {
m_finishedObservers.add(observer);
m_observers.remove(observer);
}
} }
} }
...@@ -571,7 +572,10 @@ void ImageResource::onePartInMultipartReceived(const ResourceResponse& response) ...@@ -571,7 +572,10 @@ void ImageResource::onePartInMultipartReceived(const ResourceResponse& response)
// Notify finished when the first part ends. // Notify finished when the first part ends.
if (!errorOccurred()) if (!errorOccurred())
setStatus(Cached); setStatus(Cached);
checkNotify(); // We will also notify clients/observers of the finish in
// Resource::finish()/error() so we don't mark them finished here.
notifyObserversInternal(MarkFinishedOption::DoNotMarkFinished);
notifyClientsInternal(MarkFinishedOption::DoNotMarkFinished);
if (loader()) if (loader())
loader()->didFinishLoadingFirstPartInMultipart(); loader()->didFinishLoadingFirstPartInMultipart();
} }
......
third_party/WebKit/Source/core/fetch/ImageResource.h
View file @ 62bb97b2
...@@ -163,7 +163,8 @@ private: ...@@ -163,7 +163,8 @@ private:
void ensureImage(); void ensureImage();
void checkNotify() override; void checkNotify() override;
void markClientsAndObserversFinished() override; void notifyObserversInternal(MarkFinishedOption);
void markObserverFinished(ImageResourceObserver*);
void doResetAnimation(); void doResetAnimation();
......
third_party/WebKit/Source/core/fetch/Resource.cpp
View file @ 62bb97b2
...@@ -341,13 +341,29 @@ void Resource::setLoader(ResourceLoader* loader) ...@@ -341,13 +341,29 @@ void Resource::setLoader(ResourceLoader* loader)
} }
void Resource::checkNotify() void Resource::checkNotify()
{
notifyClientsInternal(MarkFinishedOption::ShouldMarkFinished);
}
void Resource::notifyClientsInternal(MarkFinishedOption markFinishedOption)
{ {
if (isLoading()) if (isLoading())
return; return;
ResourceClientWalker<ResourceClient> w(m_clients); ResourceClientWalker<ResourceClient> w(m_clients);
while (ResourceClient* c = w.next()) while (ResourceClient* c = w.next()) {
if (markFinishedOption == MarkFinishedOption::ShouldMarkFinished)
markClientFinished(c);
c->notifyFinished(this); c->notifyFinished(this);
}
}
void Resource::markClientFinished(ResourceClient* client)
{
if (m_clients.contains(client)) {
m_finishedClients.add(client);
m_clients.remove(client);
}
} }
void Resource::appendData(const char* data, size_t length) void Resource::appendData(const char* data, size_t length)
...@@ -380,14 +396,6 @@ void Resource::setDataBufferingPolicy(DataBufferingPolicy dataBufferingPolicy) ...@@ -380,14 +396,6 @@ void Resource::setDataBufferingPolicy(DataBufferingPolicy dataBufferingPolicy)
setEncodedSize(0); setEncodedSize(0);
} }
void Resource::markClientsAndObserversFinished()
{
HashCountedSet<ResourceClient*> clients;
m_clients.swap(clients);
for (const auto& it : clients)
m_finishedClients.add(it.key, it.value);
}
void Resource::error(const ResourceError& error) void Resource::error(const ResourceError& error)
{ {
ASSERT(!error.isNull()); ASSERT(!error.isNull());
...@@ -403,7 +411,6 @@ void Resource::error(const ResourceError& error) ...@@ -403,7 +411,6 @@ void Resource::error(const ResourceError& error)
m_data.clear(); m_data.clear();
m_loader = nullptr; m_loader = nullptr;
checkNotify(); checkNotify();
markClientsAndObserversFinished();
} }
void Resource::finish(double loadFinishTime) void Resource::finish(double loadFinishTime)
...@@ -414,7 +421,6 @@ void Resource::finish(double loadFinishTime) ...@@ -414,7 +421,6 @@ void Resource::finish(double loadFinishTime)
m_status = Cached; m_status = Cached;
m_loader = nullptr; m_loader = nullptr;
checkNotify(); checkNotify();
markClientsAndObserversFinished();
} }
AtomicString Resource::httpContentType() const AtomicString Resource::httpContentType() const
......
third_party/WebKit/Source/core/fetch/Resource.h
View file @ 62bb97b2
...@@ -245,6 +245,13 @@ protected: ...@@ -245,6 +245,13 @@ protected:
virtual void checkNotify(); virtual void checkNotify();
enum class MarkFinishedOption {
ShouldMarkFinished,
DoNotMarkFinished
};
void notifyClientsInternal(MarkFinishedOption);
void markClientFinished(ResourceClient*);
virtual void destroyDecodedDataForFailedRevalidation() { } virtual void destroyDecodedDataForFailedRevalidation() { }
void setEncodedSize(size_t); void setEncodedSize(size_t);
...@@ -278,8 +285,6 @@ protected: ...@@ -278,8 +285,6 @@ protected:
virtual void destroyDecodedDataIfPossible() { } virtual void destroyDecodedDataIfPossible() { }
virtual void markClientsAndObserversFinished();
// Returns the memory dump name used for tracing. See Resource::onMemoryDump. // Returns the memory dump name used for tracing. See Resource::onMemoryDump.
String getMemoryDumpName() const; String getMemoryDumpName() const;
......
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