Bug 63360 – fix possible race in LinkLoader

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gavin Peters]

fix possible race in LinkLoader

[Gavin Peters]

Created attachment 98551 [details]
Patch

[Gavin Peters]

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.

[Darin Adler]

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.

[Gavin Peters]

Created attachment 98559 [details]
Patch

[Gavin Peters]

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.

[Gavin Peters]

Added reviewers

[WebKit Review Bot]

Comment on attachment 98559 [details]
Patch

Clearing flags on attachment: 98559

Committed r89719: <http://trac.webkit.org/changeset/89719>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Gentilcore]

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?

[Gavin Peters]

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.