Summary: | DocumentLoader should keep maps of ResourceLoaders instead of sets | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | BJ Burg <bburg> | ||||
Component: | Page Loading | Assignee: | BJ Burg <bburg> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | ap, beidson, commit-queue, japhet, kling, timothy | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 129391 | ||||||
Attachments: |
|
Description
BJ Burg
2014-02-26 13:57:18 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. 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> |