* Call willSendRequest() when the network load starts, rather than at the time of ResourceLoader creation (since those are no longer the same time) * Handle deferred request when the network load would have been started. This means ResourceLoader handles all logic related to deferred requests, and nothing needs to be re-inserted into ResourceLoadScheduler's queue. * Call CachedResource::setRequestedFromNetworkingLayer() at the time of willSendRequest() Note that we can no longer simply return 0 from SubresourceLoader::create() if willSendRequest() cancels the request (since willSendRequest() might run synchronously, but also might not be). Therefore, I made ResourceLoader::reachedTerminalState() public so that Loader can tell if the SubresourceLoader completed its life synchronously and avoid erroneously keeping it alive.
Created attachment 73554 [details] patch
Comment on attachment 73554 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=73554&action=review Looks good, r=me > WebCore/loader/loader.cpp:175 > + request->cachedResource()->setRequestedFromNetworkingLayer(); > + > +} Extra new line here.
Created attachment 73578 [details] Patch for landing
The commit-queue encountered the following flaky tests while processing attachment 73578 [details]: animations/suspend-resume-animation.html fast/text/atsui-spacing-features.html Please file bugs against the tests. These tests were authored by cmarrin@apple.com, ggaren@apple.com, hamaji@chromium.org, and mitz@webkit.org. The commit-queue is continuing to process your patch.
Comment on attachment 73578 [details] Patch for landing Seems I was hasty with r+. Browsing around on mac with this patch asserts very quickly. I think calling CachedResource::setDefersLoading(false) should call to ResourceLoadScheduler to schedule the load, instead of calling start() directly. I would be good if you could do some basic browsing using Mac debug build with this type of patches as that catches many issues not covered by the tests. Thread 0 Crashed: Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x00000001021badca WebCore::ResourceLoadScheduler::HostInformation::assertLoaderBeingCounted(WebCore::ResourceLoader*) + 218 (ResourceLoadScheduler.cpp:248) 1 com.apple.WebCore 0x00000001021bbc17 WebCore::ResourceLoadScheduler::assertLoaderBeingCounted(WebCore::ResourceLoader*) + 115 (ResourceLoadScheduler.cpp:235) 2 com.apple.WebCore 0x0000000101f85dd2 WebCore::ResourceLoader::start() + 280 (ResourceLoader.cpp:144) 3 com.apple.WebCore 0x0000000101f8616c WebCore::ResourceLoader::setDefersLoading(bool) + 290 (ResourceLoader.cpp:177) 4 com.apple.WebCore 0x000000010170aa0b WebCore::setAllDefersLoading(WTF::HashSet<WTF::RefPtr<WebCore::ResourceLoader>, WTF::PtrHash<WTF::RefPtr<WebCore::ResourceLoader> >, WTF::HashTraits<WTF::RefPtr<WebCore::ResourceLoader> > > const&, bool) + 135 (DocumentLoader.cpp:70) 5 com.apple.WebCore 0x000000010170ab8d WebCore::DocumentLoader::setDefersLoading(bool) + 91 (DocumentLoader.cpp:694) 6 com.apple.WebCore 0x00000001018711bc WebCore::FrameLoader::setDefersLoading(bool) + 74 (FrameLoader.cpp:251) 7 com.apple.WebCore 0x0000000101ddfb55 WebCore::Page::setDefersLoading(bool) + 113 (Page.cpp:579) 8 com.apple.WebKit 0x0000000100fefc9d -[WebView(WebPrivate) setDefersCallbacks:] + 115 (WebView.mm:2218) 9 com.apple.Safari 0x000000010008cee6 0x100000000 + 577254 10 com.apple.JavaScriptCore 0x000000010088f137 WTF::dispatchFunctionsFromMainThread() + 242 (MainThread.cpp:156) 11 com.apple.JavaScriptCore 0x0000000100890ad4 -[WTFMainThreadCaller call] + 21 (MainThreadMac.mm:48)
Does this mean that assertLoaderBeingCounted() is a useful assertion (see bug 49320 for context)?
(In reply to comment #5) > (From update of attachment 73578 [details]) > Seems I was hasty with r+. Browsing around on mac with this patch asserts very quickly. I think calling CachedResource::setDefersLoading(false) should call to ResourceLoadScheduler to schedule the load, instead of calling start() directly. > > I would be good if you could do some basic browsing using Mac debug build with this type of patches as that catches many issues not covered by the tests. > > Thread 0 Crashed: Dispatch queue: com.apple.main-thread > 0 com.apple.WebCore 0x00000001021badca WebCore::ResourceLoadScheduler::HostInformation::assertLoaderBeingCounted(WebCore::ResourceLoader*) + 218 (ResourceLoadScheduler.cpp:248) > 1 com.apple.WebCore 0x00000001021bbc17 WebCore::ResourceLoadScheduler::assertLoaderBeingCounted(WebCore::ResourceLoader*) + 115 (ResourceLoadScheduler.cpp:235) > 2 com.apple.WebCore 0x0000000101f85dd2 WebCore::ResourceLoader::start() + 280 (ResourceLoader.cpp:144) > 3 com.apple.WebCore 0x0000000101f8616c WebCore::ResourceLoader::setDefersLoading(bool) + 290 (ResourceLoader.cpp:177) > 4 com.apple.WebCore 0x000000010170aa0b WebCore::setAllDefersLoading(WTF::HashSet<WTF::RefPtr<WebCore::ResourceLoader>, WTF::PtrHash<WTF::RefPtr<WebCore::ResourceLoader> >, WTF::HashTraits<WTF::RefPtr<WebCore::ResourceLoader> > > const&, bool) + 135 (DocumentLoader.cpp:70) > 5 com.apple.WebCore 0x000000010170ab8d WebCore::DocumentLoader::setDefersLoading(bool) + 91 (DocumentLoader.cpp:694) > 6 com.apple.WebCore 0x00000001018711bc WebCore::FrameLoader::setDefersLoading(bool) + 74 (FrameLoader.cpp:251) > 7 com.apple.WebCore 0x0000000101ddfb55 WebCore::Page::setDefersLoading(bool) + 113 (Page.cpp:579) > 8 com.apple.WebKit 0x0000000100fefc9d -[WebView(WebPrivate) setDefersCallbacks:] + 115 (WebView.mm:2218) > 9 com.apple.Safari 0x000000010008cee6 0x100000000 + 577254 > 10 com.apple.JavaScriptCore 0x000000010088f137 WTF::dispatchFunctionsFromMainThread() + 242 (MainThread.cpp:156) > 11 com.apple.JavaScriptCore 0x0000000100890ad4 -[WTFMainThreadCaller call] + 21 (MainThreadMac.mm:48) The main problem with putting a deferred load back in the ResourceLoadScheduler queue is that I didn't expose a way to schedule an already existing load (you can create and schedule, but not just schedule). I'm happy to change it if you like, but that will add to the complexity of this change.
(In reply to comment #7) > The main problem with putting a deferred load back in the ResourceLoadScheduler queue is that I didn't expose a way to schedule an already existing load (you can create and schedule, but not just schedule). I'm happy to change it if you like, but that will add to the complexity of this change. I don't see the problem. If the ResourceLoader has not been start()ed yet then it should still be in a pending queue when the setDefersLoading(false) is called and scheduling it normally via the ResourceLoadScheduler is the right thing to do. In case the load was already start()ed when the setDefersLoading(true) was called the load will resume with m_handle->setDefersLoading(defers), start() is not called, and the code is correct.
(In reply to comment #6) > Does this mean that assertLoaderBeingCounted() is a useful assertion (see bug 49320 for context)? Yeah, probably. This patch simplifies the logic so that the current assert is correct.
Created attachment 73629 [details] Make ResourceLoader::start() protect and friend ResourceLoadScheduler Just saw your post. I was about to post this patch, so I figured I'd go ahead and get your opinion as to whether this is better or worse than leaving sending deferred loads back through ResourceLoadScheduler. We had talked in #webkit on Monday about making ResourceLoadScheduler a friend of ResourceLoader, so that start() no longer needed to be public. I believe that, if this is the case, it may remove one of the main needs for assertLoaderBeingCounted() (that the load might have been started without going through ResourceLoadScheduler). I don't have strong feelings on which technique we go with, but I figured I should post this one since the work was already done :)
Yeah, this is correct, I misread the code in the previous comment. Just calling start() directly when undeffering is the right thing to do. If the scheduler hasn't scheduled the load yet at that point m_deferredRequest will be null and nothing will happen. This also means that the assert is indeed bogus. r=me
Comment on attachment 73629 [details] Make ResourceLoader::start() protect and friend ResourceLoadScheduler Clearing flags on attachment: 73629 Committed r71884: <http://trac.webkit.org/changeset/71884>
All reviewed patches have been landed. Closing bug.