Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1923003002:
Clear Resource::m_loader in Resource::finish() and Resource::error() (Closed)
|
Created:
4 years, 8 months ago by Nate Chapin Modified:
4 years, 7 months ago CC:
chromium-reviews, caseq+blink_chromium.org, Yoav Weiss, tyoshino+watch_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, gavinp+loader_chromium.org, devtools-reviews_chromium.org, blink-reviews, loading-reviews+fetch_chromium.org, sergeyv+blink_chromium.org, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClear Resource::m_loader in Resource::finish() and Resource::error()
Ensure that m_loader is cleared at the same time Resource
starts return false for isLoading(). This allows for the
removal of a hack in ResourceFetcher::determineRevalidationPolicy.
It also allows us to remove all cases of reentrancy during
ResourceLoader completion, greatly simplifying its logic.
This requires rewriting a couple of tests that baked invalid
assumptions into how Resource load completion works. One of
those assumptions is related to removing a Resource from
MemoryCache when it is cancelled during completion, which is
strange and unnecessary outside of a couple of tests.
This also changes failure notification behavior for multipart
responses. Multipart responses have historically sent a
finish notification when the first payload part completes,
then no notification when the final part completes. This
ensures that end-of-request bookkeeping happens when the
load fails after the first part completes, which may be a
visible changes in the inspector.
BUG=
Committed: https://crrev.com/33d4038b62d5b15b216d34ee42e1e03c49e2ef99
Cr-Commit-Position: refs/heads/master@{#393042}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 13
Patch Set 6 : Fix ResourceFetchertest.RevalidateWhileFinishingLoading memory leak #
Total comments: 21
Patch Set 7 : Address comments #
Total comments: 4
Patch Set 8 : ASSERT clenaup in releaseResources() #Patch Set 9 : Rebase #Patch Set 10 : Rebase, comment in background.js #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 31 (11 generated)
Description was changed from ========== Clear Resource::m_loader in Resource::finish() and Resource::error() BUG= ========== to ========== Clear Resource::m_loader in Resource::finish() and Resource::error() Ensure that m_loader is cleared at the same time Resource starts return false for isLoading(). This allows for the removal of a hack in ResourceFetcher::determineRevalidationPolicy. It also allows us to remove all cases of reentrancy during ResourceLoader completion, greatly simplifying its logic. This requires rewriting a couple of tests that baked invalid assumptions into how Resource load completion works. BUG= ==========
japhet@chromium.org changed reviewers: + hiroshige@chromium.org, yhirano@chromium.org
PTAL! https://codereview.chromium.org/1923003002/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/extension_throttle/background.js (right): https://codereview.chromium.org/1923003002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/extension_throttle/background.js:9: xhr.setRequestHeader("X-Bust-Blink-MemoryCache", Math.random()); This test depends on the memoryCache()->remove() quirk in Resource::cancelTimerFired() that this patch made inert and removes. The test doesn't want requests served from blink's MemoryCache, so add a bogus header to circumvent blink's overly cautious XHR caching policy. https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/network/network-xhr-same-url-as-main-resource.html (right): https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/network-xhr-same-url-as-main-resource.html:18: InspectorTest.evaluateInPage("loadIframe()"); See the comment thread in https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour... for the problems with how this test was written. https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp (right): https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp:169: cachedImage->loader()->didFinishLoading(nullptr, 0.0, 0); ResourceLoader now gets made if you initialize it but don't finish, fail or cancel. The tests in this file that have changes indirectly created a ResourceLoader, and this change ensures the ResourceLoader is included in the loading process to emulate how it works outside of tests. https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.cpp (left): https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:718: m_loader->cancelIfNotFinishing(); Since m_loader is nulled before sending load completion notifications, it should now be impossible for m_loader to be non-null while finishing. https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:719: memoryCache()->remove(this); This was a dumb hack to work around tests with unstated assumptions. Those tests are fixed in this patch. https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp (right): https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:241: TEST_F(ResourceFetcherTest, RevalidateWhileFinishingLoading) This is probably how this test should have been written from the beginning. It was designed to test the special case that this patch removes from ResourceFetcher::determineRevalidationPolicy, but it's still worthwhile to test resource reuse during load completion. https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (left): https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:297: m_resource->error(Resource::LoadError); This block is also redundant with cancel/didFail. https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:331: WTF_LOG(ResourceLoading, "Received '%s'.", m_resource->url().getString().latin1().data()); I don't even know where these log to, or if anyone has used them post-fork. https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:177: didFail(nullptr, error.isNull() ? ResourceError::cancelledError(m_resource->lastResourceRequest().url()) : error); Most of cancel() is the same as didFail(). Delegate to didFail() instead. https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:308: m_fetcher->didFailLoading(m_resource.get(), error); Unconditionally call didFailLoading here. The only case where m_notifiedLoadComplete will be set to true before finishing is for multipart loads, which set it when the first payload part completes. didFinishLoadingOnePart() ensures ResourceFetcher is properly notified if the load completes successfully even when m_notifiedLoadComplete is true. However, if the load fails or cancels after m_notifiedLoadComplete is set to true, we still need to call didFailLoading to ensure that ResourceFetcher drops its reference to this ResourceLoader. https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceLoader.h (left): https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceLoader.h:103: ConnectionStateFailed, Especially in a world where finish/fail/cancel can't reenter each other, there's no particular need to have separate enum values.
On 2016/04/27 19:48:30, Nate Chapin wrote: > PTAL! > > https://codereview.chromium.org/1923003002/diff/80001/chrome/test/data/extens... > File chrome/test/data/extensions/extension_throttle/background.js (right): > > https://codereview.chromium.org/1923003002/diff/80001/chrome/test/data/extens... > chrome/test/data/extensions/extension_throttle/background.js:9: > xhr.setRequestHeader("X-Bust-Blink-MemoryCache", Math.random()); > This test depends on the memoryCache()->remove() quirk in > Resource::cancelTimerFired() that this patch made inert and removes. The test > doesn't want requests served from blink's MemoryCache, so add a bogus header to > circumvent blink's overly cautious XHR caching policy. > > https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/http/tests/inspector/network/network-xhr-same-url-as-main-resource.html > (right): > > https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/http/tests/inspector/network/network-xhr-same-url-as-main-resource.html:18: > InspectorTest.evaluateInPage("loadIframe()"); > See the comment thread in > https://codereview.chromium.org/1802123002/diff/80001/third_party/WebKit/Sour... > for the problems with how this test was written. > > https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp (right): > > https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp:169: > cachedImage->loader()->didFinishLoading(nullptr, 0.0, 0); > ResourceLoader now gets made if you initialize it but don't finish, fail or > cancel. The tests in this file that have changes indirectly created a > ResourceLoader, and this change ensures the ResourceLoader is included in the > loading process to emulate how it works outside of tests. > > https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/fetch/Resource.cpp (left): > > https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/fetch/Resource.cpp:718: > m_loader->cancelIfNotFinishing(); > Since m_loader is nulled before sending load completion notifications, it should > now be impossible for m_loader to be non-null while finishing. > > https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/fetch/Resource.cpp:719: > memoryCache()->remove(this); > This was a dumb hack to work around tests with unstated assumptions. Those tests > are fixed in this patch. > > https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp (right): > > https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:241: > TEST_F(ResourceFetcherTest, RevalidateWhileFinishingLoading) > This is probably how this test should have been written from the beginning. It > was designed to test the special case that this patch removes from > ResourceFetcher::determineRevalidationPolicy, but it's still worthwhile to test > resource reuse during load completion. > > https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (left): > > https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:297: > m_resource->error(Resource::LoadError); > This block is also redundant with cancel/didFail. > > https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:331: > WTF_LOG(ResourceLoading, "Received '%s'.", > m_resource->url().getString().latin1().data()); > I don't even know where these log to, or if anyone has used them post-fork. > > https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): > > https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:177: didFail(nullptr, > error.isNull() ? > ResourceError::cancelledError(m_resource->lastResourceRequest().url()) : error); > Most of cancel() is the same as didFail(). Delegate to didFail() instead. > > https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:308: > m_fetcher->didFailLoading(m_resource.get(), error); > Unconditionally call didFailLoading here. The only case where > m_notifiedLoadComplete will be set to true before finishing is for multipart > loads, which set it when the first payload part completes. > didFinishLoadingOnePart() ensures ResourceFetcher is properly notified if the > load completes successfully even when m_notifiedLoadComplete is true. However, > if the load fails or cancels after m_notifiedLoadComplete is set to true, we > still need to call didFailLoading to ensure that ResourceFetcher drops its > reference to this ResourceLoader. > > https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/fetch/ResourceLoader.h (left): > > https://codereview.chromium.org/1923003002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/fetch/ResourceLoader.h:103: > ConnectionStateFailed, > Especially in a world where finish/fail/cancel can't reenter each other, there's > no particular need to have separate enum values. Friendly ping :)
https://codereview.chromium.org/1923003002/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/extension_throttle/background.js (right): https://codereview.chromium.org/1923003002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/extension_throttle/background.js:9: xhr.setRequestHeader("X-Bust-Blink-MemoryCache", Math.random()); On 2016/04/27 19:48:30, Nate Chapin wrote: > This test depends on the memoryCache()->remove() quirk in > Resource::cancelTimerFired() that this patch made inert and removes. The test > doesn't want requests served from blink's MemoryCache, so add a bogus header to > circumvent blink's overly cautious XHR caching policy. Could you add a comment that this header is added just to prevent the XHR from being served by MemoryCache? (to clarify the intent of this header and that the header name 'X-Bust-Blink-MemoryCache' itself has no meaning) https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:297: m_resource->finish(); How about adding ASSERT(m_state != ConnectionStateReleased) before and after this line? Such ASSERT()s might be helpful for preventing the reentrancy from occurring again. https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:307: m_notifiedLoadComplete = true; How about adding ASSERT(!m_notifiedLoadComplete) before this line? https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:309: m_resource->error(Resource::LoadError); How about adding ASSERT(m_state != ConnectionStateReleased) before and after this line? https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceLoaderSet.cpp (right): https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoaderSet.cpp:52: // cancelAll() can reenter. Don't cancel the same ResourceLoader twice. IIUC, cancelAll() has been reentering even before this CL but ResourceLoader::cancel() just ignored the second cancel() call; this CL adds stricter ASSERT() to ResourceLoader::cancel() and thus we have to avoid second cancel() call here. Correct?
https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp (right): https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:220: RequestSameResourceOnComplete(Resource* resource) +explicit https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:258: Platform::current()->getURLLoaderMockFactory()->serveAsynchronousRequests(); +resource1->removeClient(&client); https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:258: Platform::current()->getURLLoaderMockFactory()->serveAsynchronousRequests(); Can you verify that RequestSameResourceOnComplete::notifyFinished is called? https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:246: m_fetcher->didReceiveResponse(m_resource.get(), resourceResponse); Do we need |if (m_state != Released) | here?
https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp (right): https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:220: RequestSameResourceOnComplete(Resource* resource) On 2016/05/02 12:38:45, yhirano (ooo until May 6) wrote: > +explicit Done. https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:258: Platform::current()->getURLLoaderMockFactory()->serveAsynchronousRequests(); On 2016/05/02 12:38:45, yhirano (ooo until May 6) wrote: > +resource1->removeClient(&client); Done. https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:258: Platform::current()->getURLLoaderMockFactory()->serveAsynchronousRequests(); On 2016/05/02 12:38:45, yhirano (ooo until May 6) wrote: > Can you verify that RequestSameResourceOnComplete::notifyFinished is called? Done. https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:246: m_fetcher->didReceiveResponse(m_resource.get(), resourceResponse); On 2016/05/02 12:38:45, yhirano (ooo until May 6) wrote: > Do we need |if (m_state != Released) | here? canAccessResource() doesn't cancel() the ResourceLoader, so it's currently safe without the Released check. https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:297: m_resource->finish(); On 2016/05/02 06:39:27, hiroshige (OOO May3-5 holiday) wrote: > How about adding ASSERT(m_state != ConnectionStateReleased) before and after > this line? Such ASSERT()s might be helpful for preventing the reentrancy from > occurring again. Done. https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:307: m_notifiedLoadComplete = true; On 2016/05/02 06:39:27, hiroshige (OOO May3-5 holiday) wrote: > How about adding ASSERT(!m_notifiedLoadComplete) before this line? This isn't always the case as-is. It can be true here if a cancel() occurs after the first part in a multipart load. I want to get rid of this bit eventually :/ https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:309: m_resource->error(Resource::LoadError); On 2016/05/02 06:39:27, hiroshige (OOO May3-5 holiday) wrote: > How about adding ASSERT(m_state != ConnectionStateReleased) before and after > this line? Done. https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceLoaderSet.cpp (right): https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoaderSet.cpp:52: // cancelAll() can reenter. Don't cancel the same ResourceLoader twice. On 2016/05/02 06:39:27, hiroshige (OOO May3-5 holiday) wrote: > IIUC, cancelAll() has been reentering even before this CL but > ResourceLoader::cancel() just ignored the second cancel() call; this CL adds > stricter ASSERT() to ResourceLoader::cancel() and thus we have to avoid second > cancel() call here. Correct? Correct.
https://codereview.chromium.org/1923003002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): https://codereview.chromium.org/1923003002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:81: if (m_state == ConnectionStateReleased) Can we replace this branch with an assertion? https://codereview.chromium.org/1923003002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:85: ASSERT(m_state != ConnectionStateReleased); This is no longer needed.
https://codereview.chromium.org/1923003002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): https://codereview.chromium.org/1923003002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:81: if (m_state == ConnectionStateReleased) On 2016/05/06 10:20:09, yhirano wrote: > Can we replace this branch with an assertion? Yes, we should be able to, since Resource and ResourceFetcher should have already dropped references to this. https://codereview.chromium.org/1923003002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:85: ASSERT(m_state != ConnectionStateReleased); On 2016/05/06 10:20:09, yhirano wrote: > This is no longer needed. Done.
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923003002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923003002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923003002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923003002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:297: m_resource->finish(); On 2016/05/02 20:07:37, Nate Chapin wrote: > On 2016/05/02 06:39:27, hiroshige (OOO May3-5 holiday) wrote: > > How about adding ASSERT(m_state != ConnectionStateReleased) before and after > > this line? Such ASSERT()s might be helpful for preventing the reentrancy from > > occurring again. > > Done. Ack. (memo: ASSERT() after this line is not needed (and not added) because releaseResources() already has one.) https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:307: m_notifiedLoadComplete = true; On 2016/05/02 20:07:37, Nate Chapin wrote: > On 2016/05/02 06:39:27, hiroshige (OOO May3-5 holiday) wrote: > > How about adding ASSERT(!m_notifiedLoadComplete) before this line? > > This isn't always the case as-is. It can be true here if a cancel() occurs after > the first part in a multipart load. I want to get rid of this bit eventually :/ Hmm, so this CL adds ResourceFetcher::didFailLoading() in such cases. Is it OK? I feel we should call didFailLoading() because removeResourceLoader() in didFailLoading() should be called in order to clear ResourceLoader from ResourceFetcher's loaders list (I made ResourceLoader to remain in ResourceFetcher::m_nonBlockingLoaders after the first part loading of a multipart load). But not so sure about other parts of didFailLoading().
Could you also address my comment here? https://codereview.chromium.org/1923003002/diff/80001/chrome/test/data/extens...
https://codereview.chromium.org/1923003002/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/extension_throttle/background.js (right): https://codereview.chromium.org/1923003002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/extension_throttle/background.js:9: xhr.setRequestHeader("X-Bust-Blink-MemoryCache", Math.random()); On 2016/05/02 06:39:27, hiroshige wrote: > On 2016/04/27 19:48:30, Nate Chapin wrote: > > This test depends on the memoryCache()->remove() quirk in > > Resource::cancelTimerFired() that this patch made inert and removes. The test > > doesn't want requests served from blink's MemoryCache, so add a bogus header > to > > circumvent blink's overly cautious XHR caching policy. > > Could you add a comment that this header is added just to prevent the XHR from > being served by MemoryCache? > (to clarify the intent of this header and that the header name > 'X-Bust-Blink-MemoryCache' itself has no meaning) Yeah, sorry. I was hoping the header-name was clear enough, but overdocumenting is better than underdocumenting for things like this :) https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:307: m_notifiedLoadComplete = true; On 2016/05/10 10:22:48, hiroshige wrote: > On 2016/05/02 20:07:37, Nate Chapin wrote: > > On 2016/05/02 06:39:27, hiroshige (OOO May3-5 holiday) wrote: > > > How about adding ASSERT(!m_notifiedLoadComplete) before this line? > > > > This isn't always the case as-is. It can be true here if a cancel() occurs > after > > the first part in a multipart load. I want to get rid of this bit eventually > :/ > > Hmm, so this CL adds ResourceFetcher::didFailLoading() in such cases. Is it OK? > > I feel we should call didFailLoading() because removeResourceLoader() in > didFailLoading() should be called in order to clear ResourceLoader from > ResourceFetcher's loaders list (I made ResourceLoader to remain > in ResourceFetcher::m_nonBlockingLoaders after the first part loading of a > multipart load). > But not so sure about other parts of didFailLoading(). The only non-test difference I see will be that the failure will appear in console (if the failure is not a cancellation) and inspector. It seems right to report a load failure in the console, even if it's after the first part completes. I'm not absolutely certain it's benign to report the failure to inspector, but my best guess is yes: the message goes straight to the frontend, no state is maintained in core/inspector based on it.
lgtm
lgtm with comments about CL description. https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:307: m_notifiedLoadComplete = true; On 2016/05/10 18:26:17, Nate Chapin wrote: > On 2016/05/10 10:22:48, hiroshige wrote: > > On 2016/05/02 20:07:37, Nate Chapin wrote: > > > On 2016/05/02 06:39:27, hiroshige (OOO May3-5 holiday) wrote: > > > > How about adding ASSERT(!m_notifiedLoadComplete) before this line? > > > > > > This isn't always the case as-is. It can be true here if a cancel() occurs > > after > > > the first part in a multipart load. I want to get rid of this bit eventually > > :/ > > > > Hmm, so this CL adds ResourceFetcher::didFailLoading() in such cases. Is it > OK? > > > > I feel we should call didFailLoading() because removeResourceLoader() in > > didFailLoading() should be called in order to clear ResourceLoader from > > ResourceFetcher's loaders list (I made ResourceLoader to remain > > in ResourceFetcher::m_nonBlockingLoaders after the first part loading of a > > multipart load). > > But not so sure about other parts of didFailLoading(). > > The only non-test difference I see will be that the failure will appear in > console (if the failure is not a cancellation) and inspector. It seems right to > report a load failure in the console, even if it's after the first part > completes. I'm not absolutely certain it's benign to report the failure to > inspector, but my best guess is yes: the message goes straight to the frontend, > no state is maintained in core/inspector based on it. Sounds reasonable. Please mention briefly about this behavior change in the CL description. https://codereview.chromium.org/1923003002/diff/170001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/Resource.cpp (left): https://codereview.chromium.org/1923003002/diff/170001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/Resource.cpp:780: memoryCache()->remove(this); Could you mention in the CL description that this CL removes the MemoryCache::remove() call here? (because I think this remove() call blocks Issue 606651 and thus removing this call is worth mentioning.)
Description was changed from ========== Clear Resource::m_loader in Resource::finish() and Resource::error() Ensure that m_loader is cleared at the same time Resource starts return false for isLoading(). This allows for the removal of a hack in ResourceFetcher::determineRevalidationPolicy. It also allows us to remove all cases of reentrancy during ResourceLoader completion, greatly simplifying its logic. This requires rewriting a couple of tests that baked invalid assumptions into how Resource load completion works. BUG= ========== to ========== Clear Resource::m_loader in Resource::finish() and Resource::error() Ensure that m_loader is cleared at the same time Resource starts return false for isLoading(). This allows for the removal of a hack in ResourceFetcher::determineRevalidationPolicy. It also allows us to remove all cases of reentrancy during ResourceLoader completion, greatly simplifying its logic. This requires rewriting a couple of tests that baked invalid assumptions into how Resource load completion works. One of those assumptions is related to removing a Resource from MemoryCache when it is cancelled during completion, which is strange and unnecessary outside of a couple of tests. This also changes failure notification behavior for multipart responses. Multipart responses have historically sent a finish notification when the first payload part completes, then no notification when the final part completes. This ensures that end-of-request bookkeeping happens when the load fails after the first part completes, which may be a visible changes in the inspector. BUG= ==========
Description was changed from ========== Clear Resource::m_loader in Resource::finish() and Resource::error() Ensure that m_loader is cleared at the same time Resource starts return false for isLoading(). This allows for the removal of a hack in ResourceFetcher::determineRevalidationPolicy. It also allows us to remove all cases of reentrancy during ResourceLoader completion, greatly simplifying its logic. This requires rewriting a couple of tests that baked invalid assumptions into how Resource load completion works. One of those assumptions is related to removing a Resource from MemoryCache when it is cancelled during completion, which is strange and unnecessary outside of a couple of tests. This also changes failure notification behavior for multipart responses. Multipart responses have historically sent a finish notification when the first payload part completes, then no notification when the final part completes. This ensures that end-of-request bookkeeping happens when the load fails after the first part completes, which may be a visible changes in the inspector. BUG= ========== to ========== Clear Resource::m_loader in Resource::finish() and Resource::error() Ensure that m_loader is cleared at the same time Resource starts return false for isLoading(). This allows for the removal of a hack in ResourceFetcher::determineRevalidationPolicy. It also allows us to remove all cases of reentrancy during ResourceLoader completion, greatly simplifying its logic. This requires rewriting a couple of tests that baked invalid assumptions into how Resource load completion works. One of those assumptions is related to removing a Resource from MemoryCache when it is cancelled during completion, which is strange and unnecessary outside of a couple of tests. This also changes failure notification behavior for multipart responses. Multipart responses have historically sent a finish notification when the first payload part completes, then no notification when the final part completes. This ensures that end-of-request bookkeeping happens when the load fails after the first part completes, which may be a visible changes in the inspector. BUG= ==========
https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): https://codereview.chromium.org/1923003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:307: m_notifiedLoadComplete = true; On 2016/05/11 09:28:38, hiroshige wrote: > On 2016/05/10 18:26:17, Nate Chapin wrote: > > On 2016/05/10 10:22:48, hiroshige wrote: > > > On 2016/05/02 20:07:37, Nate Chapin wrote: > > > > On 2016/05/02 06:39:27, hiroshige (OOO May3-5 holiday) wrote: > > > > > How about adding ASSERT(!m_notifiedLoadComplete) before this line? > > > > > > > > This isn't always the case as-is. It can be true here if a cancel() occurs > > > after > > > > the first part in a multipart load. I want to get rid of this bit > eventually > > > :/ > > > > > > Hmm, so this CL adds ResourceFetcher::didFailLoading() in such cases. Is it > > OK? > > > > > > I feel we should call didFailLoading() because removeResourceLoader() in > > > didFailLoading() should be called in order to clear ResourceLoader from > > > ResourceFetcher's loaders list (I made ResourceLoader to remain > > > in ResourceFetcher::m_nonBlockingLoaders after the first part loading of a > > > multipart load). > > > But not so sure about other parts of didFailLoading(). > > > > The only non-test difference I see will be that the failure will appear in > > console (if the failure is not a cancellation) and inspector. It seems right > to > > report a load failure in the console, even if it's after the first part > > completes. I'm not absolutely certain it's benign to report the failure to > > inspector, but my best guess is yes: the message goes straight to the > frontend, > > no state is maintained in core/inspector based on it. > > Sounds reasonable. > Please mention briefly about this behavior change in the CL description. Done. https://codereview.chromium.org/1923003002/diff/170001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/Resource.cpp (left): https://codereview.chromium.org/1923003002/diff/170001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/Resource.cpp:780: memoryCache()->remove(this); On 2016/05/11 09:28:38, hiroshige wrote: > Could you mention in the CL description that this CL removes the > MemoryCache::remove() call here? > (because I think this remove() call blocks Issue 606651 and thus removing this > call is worth mentioning.) Done.
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923003002/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923003002/170001
Message was sent while issue was closed.
Description was changed from ========== Clear Resource::m_loader in Resource::finish() and Resource::error() Ensure that m_loader is cleared at the same time Resource starts return false for isLoading(). This allows for the removal of a hack in ResourceFetcher::determineRevalidationPolicy. It also allows us to remove all cases of reentrancy during ResourceLoader completion, greatly simplifying its logic. This requires rewriting a couple of tests that baked invalid assumptions into how Resource load completion works. One of those assumptions is related to removing a Resource from MemoryCache when it is cancelled during completion, which is strange and unnecessary outside of a couple of tests. This also changes failure notification behavior for multipart responses. Multipart responses have historically sent a finish notification when the first payload part completes, then no notification when the final part completes. This ensures that end-of-request bookkeeping happens when the load fails after the first part completes, which may be a visible changes in the inspector. BUG= ========== to ========== Clear Resource::m_loader in Resource::finish() and Resource::error() Ensure that m_loader is cleared at the same time Resource starts return false for isLoading(). This allows for the removal of a hack in ResourceFetcher::determineRevalidationPolicy. It also allows us to remove all cases of reentrancy during ResourceLoader completion, greatly simplifying its logic. This requires rewriting a couple of tests that baked invalid assumptions into how Resource load completion works. One of those assumptions is related to removing a Resource from MemoryCache when it is cancelled during completion, which is strange and unnecessary outside of a couple of tests. This also changes failure notification behavior for multipart responses. Multipart responses have historically sent a finish notification when the first payload part completes, then no notification when the final part completes. This ensures that end-of-request bookkeeping happens when the load fails after the first part completes, which may be a visible changes in the inspector. BUG= ==========
Message was sent while issue was closed.
Committed patchset #10 (id:170001)
Message was sent while issue was closed.
Description was changed from ========== Clear Resource::m_loader in Resource::finish() and Resource::error() Ensure that m_loader is cleared at the same time Resource starts return false for isLoading(). This allows for the removal of a hack in ResourceFetcher::determineRevalidationPolicy. It also allows us to remove all cases of reentrancy during ResourceLoader completion, greatly simplifying its logic. This requires rewriting a couple of tests that baked invalid assumptions into how Resource load completion works. One of those assumptions is related to removing a Resource from MemoryCache when it is cancelled during completion, which is strange and unnecessary outside of a couple of tests. This also changes failure notification behavior for multipart responses. Multipart responses have historically sent a finish notification when the first payload part completes, then no notification when the final part completes. This ensures that end-of-request bookkeeping happens when the load fails after the first part completes, which may be a visible changes in the inspector. BUG= ========== to ========== Clear Resource::m_loader in Resource::finish() and Resource::error() Ensure that m_loader is cleared at the same time Resource starts return false for isLoading(). This allows for the removal of a hack in ResourceFetcher::determineRevalidationPolicy. It also allows us to remove all cases of reentrancy during ResourceLoader completion, greatly simplifying its logic. This requires rewriting a couple of tests that baked invalid assumptions into how Resource load completion works. One of those assumptions is related to removing a Resource from MemoryCache when it is cancelled during completion, which is strange and unnecessary outside of a couple of tests. This also changes failure notification behavior for multipart responses. Multipart responses have historically sent a finish notification when the first payload part completes, then no notification when the final part completes. This ensures that end-of-request bookkeeping happens when the load fails after the first part completes, which may be a visible changes in the inspector. BUG= Committed: https://crrev.com/33d4038b62d5b15b216d34ee42e1e03c49e2ef99 Cr-Commit-Position: refs/heads/master@{#393042} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/33d4038b62d5b15b216d34ee42e1e03c49e2ef99 Cr-Commit-Position: refs/heads/master@{#393042} |
