RESOLVED FIXED 129388
DocumentLoader should keep maps of ResourceLoaders instead of sets 
https://bugs.webkit.org/show_bug.cgi?id=129388
Summary DocumentLoader should keep maps of ResourceLoaders instead of sets
Blaze Burg
Reported 2014-02-26 13:57:18 PST
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.
Attachments
the patch (9.40 KB, patch)
2014-02-28 15:25 PST, Blaze Burg 		darin: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Brady Eidson
Comment 1 2014-02-26 14:41:29 PST
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.
Blaze Burg
Comment 2 2014-02-28 15:25:39 PST
Created attachment 225500 [details] the patch
Blaze Burg
Comment 3 2014-02-28 15:26:28 PST
(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!
Darin Adler
Comment 4 2014-03-01 16:37:51 PST
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.
Blaze Burg
Comment 5 2014-03-01 20:42:59 PST
(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!
Blaze Burg
Comment 6 2014-03-02 11:21:20 PST
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)
Blaze Burg
Comment 7 2014-03-02 12:29:13 PST
Committed r164947: <http://trac.webkit.org/changeset/164947>
Note You need to log in before you can comment on or make changes to this bug.