Bug 92774

Summary:

Crash in FrameLoader::checkLoadComplete with non-browser client app

Product:

WebKit

Reporter:

Antti Koivisto <koivisto>

Component:

Page Loading

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, cmarcelo, inferno, macpherson, menard, webkit.review.bot

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
speculative fix	ap: review+

Description Antti Koivisto 2012-07-31 11:09:16 PDT

Seen with a non-browser client app

0   WebCore                       	0x327aaf3c WebCore::FrameLoader::checkLoadComplete() (FrameLoader.cpp:2399)
1   WebCore                       	0x32a6cd30 WebCore::CSSFontSelector::beginLoadTimerFired(WebCore::Timer<WebCore::CSSFontSelector>*) (CSSFontSelector.cpp:619)
2   WebCore                       	0x327e69a6 WebCore::ThreadTimers::sharedTimerFiredInternal() (ThreadTimers.cpp:129)

Comment 1 Antti Koivisto 2012-07-31 11:13:34 PDT

<rdar://problem/11956222>

Comment 2 Antti Koivisto 2012-07-31 11:27:54 PDT

Created attachment 155591 [details]
speculative fix

Comment 3 Alexey Proskuryakov 2012-07-31 11:39:49 PDT

Comment on attachment 155591 [details]
speculative fix

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

> Source/WebCore/css/CSSFontSelector.cpp:581
> +    // It is possible load calls could cause CSSFontSelector to get deleted synchronously.

"CSSFontSelector can be deleted via beginLoadIfNeeded() or loadDone() unless protected." (or better something more specific about how that could happen).

Both "possible" and "could" make this difficult to read.

> Source/WebCore/css/CSSFontSelector.cpp:582
> +    // If detached clearDocument() will get called ensuring m_document is null.

I think that you're explaining why m_document won't be a dangling pointer, but that seems excessive. We don't make such comment every time we use a member variable pointing to some other object, and don't make it for every variable that has a clearXXX() method. Is there something unusual here that needs to be explained?

Comment 4 Antti Koivisto 2012-07-31 11:52:24 PDT

http://trac.webkit.org/changeset/124229

Comment 5 Abhishek Arya 2012-08-01 15:32:11 PDT

*** Bug 91701 has been marked as a duplicate of this bug. ***

Comment 6 Abhishek Arya 2012-08-01 15:33:51 PDT

Confirming at the testcase we had at ClusterFuzz got fixed by your AWESOMENESS :)

Comment 7 Alexey Proskuryakov 2012-08-01 15:37:41 PDT

Great! Can we have the test case landed then?