Bug 86935

Summary: REGRESSION(r117501): IconDatabase asserts on startup in synchronousIconForPageURL().
Product: WebKit Reporter: Andreas Kling <kling>
Component: Page LoadingAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, fpizlo, ggaren, japhet, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Pretentious patch andersca: review+

Description Andreas Kling 2012-05-18 19:55:55 PDT
<rdar://problem/11480012>
Comment 1 Andreas Kling 2012-05-18 19:57:17 PDT
Created attachment 142843 [details]
Pretentious patch
Comment 2 Anders Carlsson 2012-05-18 22:51:03 PDT
Comment on attachment 142843 [details]
Pretentious patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142843&action=review

> Source/WebCore/loader/icon/IconDatabase.cpp:1529
> +        toRetain = m_urlsToRetain;
> +        toRelease = m_urlsToRelease;
> +        m_urlsToRetain.clear();
> +        m_urlsToRelease.clear();

I think you could use swap here.
Comment 3 Darin Adler 2012-05-19 16:54:12 PDT
Comment on attachment 142843 [details]
Pretentious patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142843&action=review

>> Source/WebCore/loader/icon/IconDatabase.cpp:1529
>> +        toRetain = m_urlsToRetain;
>> +        toRelease = m_urlsToRelease;
>> +        m_urlsToRetain.clear();
>> +        m_urlsToRelease.clear();
> 
> I think you could use swap here.

And a swap would be much more efficient than the assignment operator can be.

Otherwise, you could get significantly better performance using a vector rather than a counted set for the temporary copy, although there is probably not a suitable helper routine to copy the contents of a counted set into a suitably typed vector.
Comment 4 Andreas Kling 2012-05-21 00:07:04 PDT
(In reply to comment #2)
> (From update of attachment 142843 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142843&action=review
> 
> > Source/WebCore/loader/icon/IconDatabase.cpp:1529
> > +        toRetain = m_urlsToRetain;
> > +        toRelease = m_urlsToRelease;
> > +        m_urlsToRetain.clear();
> > +        m_urlsToRelease.clear();
> 
> I think you could use swap here.

Yeah, you guys win. I'll add a swap() to HashCountedSet.
Comment 5 Andreas Kling 2012-05-21 01:14:18 PDT
Committed r117744: <http://trac.webkit.org/changeset/117744>