REGRESSION(r157081): Fix multipart handling of ResourceFetcher (issue 242373002)

0 views
Skip to first unread message

mor...@chromium.org

unread,
Apr 17, 2014, 8:36:22 PM4/17/14
to aba...@chromium.org, jap...@chromium.org, blink-...@chromium.org, jap...@chromium.org, gavinp...@chromium.org
Reviewers: abarth, Nate Chapin,

Message:
PTAL?

Description:
REGRESSION(r157081): Fix multipart handling of ResourceFetcher

r157081 broke multipart handling in ResourceFetcher.
The breakage was harmless except Motion JPEG case - ResourceFetcher
lost the reference to Motion JPEG resource so it was no longer
able to cancel the ongoing resource fetching.

This change makes the tracking working again.
Also, the change adds a stopFetching() call to DocumentLoder::stopLoading()
that is used to be there but was lost at some point.

TEST=stop-loading.html
R=jap...@chromium.org,aba...@chromium.org
BUG=309641

Please review this at https://codereview.chromium.org/242373002/

SVN Base: svn://svn.chromium.org/blink/trunk

Affected files (+36, -7 lines):
A LayoutTests/http/tests/multipart/stop-loading.html
A + LayoutTests/http/tests/multipart/stop-loading-expected.txt
M Source/core/fetch/ResourceFetcher.cpp
M Source/core/loader/DocumentLoader.cpp


Index: LayoutTests/http/tests/multipart/stop-loading-expected.txt
diff --git
a/LayoutTests/http/tests/security/calling-versus-current-expected.txt
b/LayoutTests/http/tests/multipart/stop-loading-expected.txt
similarity index 100%
copy from
LayoutTests/http/tests/security/calling-versus-current-expected.txt
copy to LayoutTests/http/tests/multipart/stop-loading-expected.txt
Index: LayoutTests/http/tests/multipart/stop-loading.html
diff --git a/LayoutTests/http/tests/multipart/stop-loading.html
b/LayoutTests/http/tests/multipart/stop-loading.html
new file mode 100644
index
0000000000000000000000000000000000000000..c5665aab692ced021d82911b43ad40d83f0ed609
--- /dev/null
+++ b/LayoutTests/http/tests/multipart/stop-loading.html
@@ -0,0 +1,25 @@
+<html>
+<head>
+<script>
+if (window.testRunner) {
+ testRunner.dumpAsText();
+ testRunner.waitUntilDone();
+}
+
+function firstPartLoaded()
+{
+ window.stop();
+ window.setTimeout(function() {
+ var broken = (testingImage.width != 2 && testingImage != 76);
+ document.getElementById("results").innerHTML =
broken ? "PASS" : "FAIL";
+ if (window.testRunner)
+ testRunner.notifyDone();
+ }, 100);
+}
+</script>
+</head>
+<body>
+<img id=testingImage
src="resources/multipart.php?interval=1&loop=1&img1=2x2-green.png&img2=abe.png"
onload="firstPartLoaded()">
+<p id="results"></p>
+</body>
+</html>
Index: Source/core/fetch/ResourceFetcher.cpp
diff --git a/Source/core/fetch/ResourceFetcher.cpp
b/Source/core/fetch/ResourceFetcher.cpp
index
48ef9d71fca50d00a5308b29ee33d42409e5173d..d90fbdec68168b13bc0030d4d9e8faa4888ef188
100644
--- a/Source/core/fetch/ResourceFetcher.cpp
+++ b/Source/core/fetch/ResourceFetcher.cpp
@@ -1250,10 +1250,10 @@ void ResourceFetcher::didDownloadData(const
Resource* resource, int dataLength,

void
ResourceFetcher::subresourceLoaderFinishedLoadingOnePart(ResourceLoader*
loader)
{
- if (m_multipartLoaders)
- m_multipartLoaders->add(loader);
- if (m_loaders)
- m_loaders->remove(loader);
+ if (!m_multipartLoaders)
+ m_multipartLoaders = adoptPtr(new ResourceLoaderSet());
+ m_multipartLoaders->add(loader);
+ m_loaders->remove(loader);
if (LocalFrame* frame = this->frame())
return frame->loader().checkLoadComplete(m_documentLoader);
}
@@ -1270,9 +1270,10 @@ void
ResourceFetcher::didInitializeResourceLoader(ResourceLoader* loader)

void ResourceFetcher::willTerminateResourceLoader(ResourceLoader* loader)
{
- if (!m_loaders || !m_loaders->contains(loader))
- return;
- m_loaders->remove(loader);
+ if (m_loaders && m_loaders->contains(loader))
+ m_loaders->remove(loader);
+ if (m_multipartLoaders && m_multipartLoaders->contains(loader))
+ m_multipartLoaders->remove(loader);
if (LocalFrame* frame = this->frame())
frame->loader().checkLoadComplete(m_documentLoader);
}
Index: Source/core/loader/DocumentLoader.cpp
diff --git a/Source/core/loader/DocumentLoader.cpp
b/Source/core/loader/DocumentLoader.cpp
index
e57453a91dbb40af78a00c064845f893fed506fb..9363f43c5e4d81f6ce3a4f43e43e3d7fbe158a2c
100644
--- a/Source/core/loader/DocumentLoader.cpp
+++ b/Source/core/loader/DocumentLoader.cpp
@@ -188,8 +188,11 @@ void DocumentLoader::stopLoading()
m_frame->loader().stopLoading();
}

- if (!loading)
+ if (!loading) {
+ // This is needed to cancel multipart loading which can be
happening even after the frame is loaded.
+ m_fetcher->stopFetching();
return;
+ }

if (m_loadingMainResource) {
// Stop the main resource loader and let it send the cancelled
message.


mor...@chromium.org

unread,
Apr 18, 2014, 2:03:22 PM4/18/14
to aba...@chromium.org, jap...@chromium.org, blink-...@chromium.org, jap...@chromium.org, gavinp...@chromium.org
Hey Nate, could you take a look? This fixes a silly mistake I made in
ResourceFetcher :-/


https://codereview.chromium.org/242373002/

jap...@chromium.org

unread,
Apr 18, 2014, 2:10:29 PM4/18/14
to mor...@chromium.org, aba...@chromium.org, blink-...@chromium.org, gavinp...@chromium.org
LGTM with a question


https://codereview.chromium.org/242373002/diff/1/Source/core/loader/DocumentLoader.cpp
File Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/242373002/diff/1/Source/core/loader/DocumentLoader.cpp#newcode193
Source/core/loader/DocumentLoader.cpp:193: m_fetcher->stopFetching();
Is the ordering of this call important? If not, I'd favor moving this
stopFetching() call above this if() block and removing the
stopFetching() call at the end of the function.

https://codereview.chromium.org/242373002/

mor...@chromium.org

unread,
Apr 18, 2014, 2:15:27 PM4/18/14
to aba...@chromium.org, jap...@chromium.org, blink-...@chromium.org, jap...@chromium.org, gavinp...@chromium.org
Thanks for the review!


https://codereview.chromium.org/242373002/diff/1/Source/core/loader/DocumentLoader.cpp#newcode193
> Source/core/loader/DocumentLoader.cpp:193: m_fetcher->stopFetching();
> Is the ordering of this call important? If not, I'd favor moving this
> stopFetching() call above this if() block and removing the stopFetching()
> call
> at the end of the function.

I don't know. This change just tries to follow what the old code did.
Will try to move it and see if tests pass.


https://codereview.chromium.org/242373002/

commi...@chromium.org

unread,
Apr 18, 2014, 2:54:52 PM4/18/14
to mor...@chromium.org, aba...@chromium.org, jap...@chromium.org, blink-...@chromium.org, jap...@chromium.org, gavinp...@chromium.org
CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mor...@chromium.org/242373002/20001


https://codereview.chromium.org/242373002/

commi...@chromium.org

unread,
Apr 18, 2014, 4:06:40 PM4/18/14
to mor...@chromium.org, aba...@chromium.org, jap...@chromium.org, blink-...@chromium.org, jap...@chromium.org, gavinp...@chromium.org
Change committed as 171969

https://codereview.chromium.org/242373002/
Reply all
Reply to author
Forward
0 new messages