DocumentLoader keeps three sets of all active ResourceLoaders (for subresources, plugins, and multipart subresources). To deterministically replay network traffic, we need to be able to get a ResourceLoader by its identifier out of the DocumentLoader, and then simulate a network callback on the loader. So, we should use a Map (where keys are identifiers) instead of a Set.
We occasionally (semi regularly?) end up with identifier-related bugs. Add plenty of ASSERTs around interaction with this map to make sure you're not adding a loader with the same identifier in twice, or getting a loader out that you expect to be there but it's not.
Created attachment 225500 [details] the patch
(In reply to comment #1) > Add plenty of ASSERTs around interaction with this map to make sure you're not adding a loader with the same identifier in twice, or getting a loader out that you expect to be there but it's not. I added these asserts. One of them caught an apparent leak (see the ChangeLog). Win!
Comment on attachment 225500 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=225500&action=review Would be nice to find an even better idiom for the "copy values and iterate" that doesn't require two extra lines of code and explicitly specifying the vector type. > Source/WebCore/loader/DocumentLoader.cpp:1343 > + m_subresourceLoaders.set(loader->identifier(), loader); Should use add instead of set. The only difference is that set will replace an existing element if there is one, and set is implemented by doing an add and then an additional write after the fact. > Source/WebCore/loader/DocumentLoader.cpp:1362 > + m_plugInStreamLoaders.set(loader->identifier(), loader); Should use add instead of set. Same reasons as above. > Source/WebCore/loader/DocumentLoader.cpp:1499 > + m_multipartSubresourceLoaders.set(loader->identifier(), loader); Should use add instead of set. Same reasons as above. > Source/WebCore/loader/mac/DocumentLoaderMac.cpp:44 > + for (auto& loader : loadersCopy) > + if (ResourceHandle* handle = loader->handle()) > handle->schedule(pair); Multi-line for loop body gets braces in WebKit coding style. > Source/WebCore/loader/mac/DocumentLoaderMac.cpp:53 > + for (auto& loader : loadersCopy) > + if (ResourceHandle* handle = loader->handle()) > handle->unschedule(pair); Multi-line for loop body gets braces in WebKit coding style.
(In reply to comment #4) > (From update of attachment 225500 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=225500&action=review > > > Source/WebCore/loader/DocumentLoader.cpp:1343 > > + m_subresourceLoaders.set(loader->identifier(), loader); I meant to do what you said, but I mixed them up. Thanks!
Comment on attachment 225500 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=225500&action=review >> Source/WebCore/loader/mac/DocumentLoaderMac.cpp:44 >> handle->schedule(pair); > > Multi-line for loop body gets braces in WebKit coding style. Oops. I think check-webkit-style should have caught this (existing bug: https://bugs.webkit.org/show_bug.cgi?id=34189)
Committed r164947: <http://trac.webkit.org/changeset/164947>