Bug 63360 - fix possible race in LinkLoader
Summary: fix possible race in LinkLoader
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Peters
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-24 15:24 PDT by Gavin Peters
Modified: 2011-06-27 10:11 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.07 KB, patch)
2011-06-24 15:29 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (4.18 KB, patch)
2011-06-24 16:24 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Peters 2011-06-24 15:24:33 PDT
fix possible race in LinkLoader
Comment 1 Gavin Peters 2011-06-24 15:29:15 PDT
Created attachment 98551 [details]
Patch
Comment 2 Gavin Peters 2011-06-24 15:34:24 PDT
Comment on attachment 98551 [details]
Patch

I'm hoping this is the race I'm chasing for bug 80729, and I was hoping this change would find some results in the canary.  However, the ASSERT at the top of LinkLoader::notifyFinished gives me some pause.
Comment 3 Darin Adler 2011-06-24 15:49:08 PDT
Comment on attachment 98551 [details]
Patch

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

> Source/WebCore/loader/LinkLoader.cpp:62
> +void LinkLoader::linkLoadTimerFired(Timer<LinkLoader>* timer)

You should either omit the argument name, since it’s unused, or keep the ASSERT_UNUSED.

> Source/WebCore/loader/LinkLoader.cpp:-63
> -    ASSERT_UNUSED(timer, timer == &m_linkLoadedTimer);

Why get rid of this?

> Source/WebCore/loader/LinkLoader.cpp:67
> +void LinkLoader::linkLoadingErrorTimerFired(Timer<LinkLoader>* timer)

You should either omit the argument name, since it’s unused, or use ASSERT_UNUSED.
Comment 4 Gavin Peters 2011-06-24 16:24:06 PDT
Created attachment 98559 [details]
Patch
Comment 5 Gavin Peters 2011-06-24 16:25:52 PDT
Comment on attachment 98559 [details]
Patch

Thanks for the review Darin.  There was no good reason to be rid of that ASSERT, so I left it in and added another for good measure.  Also, I'm keeping the timer parameters just for the assert.
Comment 6 Gavin Peters 2011-06-24 16:29:21 PDT
Added reviewers
Comment 7 WebKit Review Bot 2011-06-24 17:39:46 PDT
Comment on attachment 98559 [details]
Patch

Clearing flags on attachment: 98559

Committed r89719: <http://trac.webkit.org/changeset/89719>
Comment 8 WebKit Review Bot 2011-06-24 17:39:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Tony Gentilcore 2011-06-27 02:01:40 PDT
Comment on attachment 98559 [details]
Patch

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

> Source/WebCore/loader/LinkLoader.cpp:83
> +    m_cachedLinkResource->removeClient(this);

Once the CachedResourceHandle's client is removed, it can purge its internal buffer. Just wanted to make sure that is safe. Perhaps it is since HTMLLinkElement::linkLoaded doesn't actually do anything with the data?
Comment 10 Gavin Peters 2011-06-27 10:11:01 PDT
Thanks Tony.

You're right, since we ignore the data, we're good.  In fact if the current resource is requested again (say, as an image), the cache drops the entire prefetch cachedresource and gets a new one.  So we're fine, for now.

This summer I hope to allow cached resources to mutate their type.  That will mean that this could become a bug, so it's helpful you flagged it.