Bug 49351 – More ResourceLoadScheduler cleanup

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

* 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.

[Nate Chapin]

Created attachment 73554 [details]
patch

[Antti Koivisto]

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.

[Adam Barth]

Created attachment 73578 [details]
Patch for landing

[WebKit Commit Bot]

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 [e-mail], [e-mail], [e-mail], and [e-mail].  The commit-queue is continuing to process your patch.

[Antti Koivisto]

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)

[Alexey Proskuryakov]

Does this mean that assertLoaderBeingCounted() is a useful assertion (see bug 49320 for context)?

[Nate Chapin]

(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.

[Antti Koivisto]

(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.

[Antti Koivisto]

(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.

[Nate Chapin]

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 :)

[Antti Koivisto]

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

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.