Bug 61733 – [Chromium] REGRESSION: Crash in WebCore::HTMLLinkElement::onloadTimerFired after r87628

Bug 61733 - [Chromium] REGRESSION: Crash in WebCore::HTMLLinkElement::onloadTimerFired after r87628


Summary:	[Chromium] REGRESSION: Crash in WebCore::HTMLLinkElement::onloadTimerFired af...

Status:	RESOLVED FIXED

Product:	WebKit
Classification:	Unclassified
Component:	HTML DOM
Version:	528+ (Nightly build)
Platform:	All All

Importance:	P1 Normal
Assigned To:	Mikhail Naganov

URL:
Keywords:

Depends on:	61736
Blocks:
	Show dependency tree / graph

Reported:	2011-05-30 09:31 PDT by Mikhail Naganov
Modified:	2011-05-30 12:29 PDT (History)
CC List:	2 users (show)

See Also:

Attachments
patch (1.10 KB, patch) 2011-05-30 09:37 PDT, Mikhail Naganov	no flags	Details \| Formatted Diff \| Diff
Updated patch (2.02 KB, patch) 2011-05-30 10:15 PDT, Adam Barth	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Mikhail Naganov 2011-05-30 09:31:32 PDT

Having r87628 in place, Chrome reliability bot crashes in WebCore::HTMLLinkElement::onloadTimerFired

http://build.chromium.org/p/chromium/builders/Win%20Reliability/builds/4073/steps/reliability%3A%20partial%20result%20of%20current%20build/logs/stdio

This is because the change makes WebCore::CachedResource::setRequest to call checkNotify on request reset.
HTMLLinkElement registers itself as CachedResource client via m_cachedSheet, which can happen even if m_cachedLinkResource wasn't set.
As a result, WebCore::HTMLLinkElement::notifyFinished is got called with unset m_cachedLinkResource, which causes a crash in HTMLLinkElement::onloadTimerFired

Comment 1 Mikhail Naganov 2011-05-30 09:37:28 PDT

Created attachment 95348 [details]
patch

Comment 2 Adam Barth 2011-05-30 09:54:43 PDT

Comment on attachment 95348 [details]
patch

I'm not sure this patch is correct.  Why is notifyFinished being called with a different cached resource?

Comment 3 Adam Barth 2011-05-30 10:15:41 PDT

Created attachment 95351 [details]
Updated patch

Comment 4 Adam Barth 2011-05-30 10:19:12 PDT

I'm going to land this patch without a test because this is blocking WebKit => Chromium integration.  I'll add the test in Bug 61736.

Comment 5 Adam Barth 2011-05-30 10:21:19 PDT

Committed r87693: <http://trac.webkit.org/changeset/87693>

Comment 6 Alexey Proskuryakov 2011-05-30 12:29:26 PDT

Thanks Adam! I don't have the time to deeply investigate this right now, but the patch looks very reasonable.