Bug 125110

Summary:

Possible crash in void ProgressTracker::progressHeartbeatTimerFired(Timer<ProgressTracker>*)

Product:

WebKit

Reporter:

Brady Eidson <beidson>

Component:

Page Loading

Assignee:

Brady Eidson <beidson>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, ddkilzer, japhet

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch v1	darin: review+
Patch v2 - Also null check a FrameView	darin: review+

Description Brady Eidson 2013-12-02 15:12:10 PST

Possible crash in void ProgressTracker::progressHeartbeatTimerFired(Timer<ProgressTracker>*)

We know that it is possible to crash in ProgressTracker::progressHeartbeatTimerFired(Timer<ProgressTracker>*) with a null m_originatingProgressFrame.

On the surface it appears this should not be possible because any time m_originatingProgressFrame is cleared out the progress heartbeat timer is also stopped.  It is likely a race condition with stopping the timer in multi-threaded environments, yet we still have no way to reproduce.

There's no harm in null checking m_originatingProgressFrame before calling in to its loader.

Comment 1 Brady Eidson 2013-12-02 15:17:17 PST

Created attachment 218223 [details]
Patch v1

Comment 2 Darin Adler 2013-12-02 15:22:42 PST

Comment on attachment 218223 [details]
Patch v1

While there is no harm in adding a null check, if this is actually a race condition, then a null check is likely not sufficient to fix the problem. It might, however, reduce the frequency of a crash.

Comment 3 Brady Eidson 2013-12-02 15:28:57 PST

(In reply to comment #2)
> (From update of attachment 218223 [details])
> While there is no harm in adding a null check, if this is actually a race condition, then a null check is likely not sufficient to fix the problem. It might, however, reduce the frequency of a crash.

Agreed.

Based on looking at disassembly this is our only theory on what's going on, so a more comprehensive fix isn't presenting itself yet.

Landed in http://trac.webkit.org/changeset/159974

Comment 4 Brady Eidson 2013-12-02 16:39:56 PST

This null check might have been incorrect

Comment 5 Brady Eidson 2013-12-02 16:40:51 PST

More specifically, ProgressTracker::progressHeartbeatTimerFired(Timer<ProgressTracker>*) might be calling in to a valid m_originatingProgressFrame (and its valid loader), but in FrameLoader::loadProgressingStatusChanged() we might have a null FrameView.

Comment 6 Brady Eidson 2013-12-02 16:43:09 PST

Created attachment 218237 [details]
Patch v2 - Also null check a FrameView

Comment 7 Brady Eidson 2013-12-02 16:43:09 PST

Created attachment 218238

Comment 8 Brady Eidson 2013-12-02 17:24:37 PST

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

Comment 9 David Kilzer (:ddkilzer) 2014-01-19 21:00:08 PST

<rdar://problem/15217862>