Bug 129388 – DocumentLoader should keep maps of ResourceLoaders instead of sets

• Component: Page Loading
• Status: RESOLVED FIXED

[Blaze Burg]

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.

[Brady Eidson]

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]

Created attachment 225500 [details]
the patch

[Blaze Burg]

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

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

Committed r164947: <http://trac.webkit.org/changeset/164947>