Bug 125110

Summary: Possible crash in void ProgressTracker::progressHeartbeatTimerFired(Timer<ProgressTracker>*)
Product: WebKit Reporter: Brady Eidson <beidson>
Component: Page LoadingAssignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ddkilzer, japhet
Priority: P2 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+

Brady Eidson
Reported 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.
Attachments
Patch v1 (1.76 KB, patch)
2013-12-02 15:17 PST, Brady Eidson 		darin: review+
DetailsFormatted DiffDiff
Patch v2 - Also null check a FrameView (1.49 KB, patch)
2013-12-02 16:43 PST, Brady Eidson 		darin: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Brady Eidson
Comment 1 2013-12-02 15:17:17 PST
Created attachment 218223 [details] Patch v1
Darin Adler
Comment 2 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.
Brady Eidson
Comment 3 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
Brady Eidson
Comment 4 2013-12-02 16:39:56 PST
This null check might have been incorrect
Brady Eidson
Comment 5 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.
Brady Eidson
Comment 6 2013-12-02 16:43:09 PST
Created attachment 218237 [details] Patch v2 - Also null check a FrameView
Brady Eidson
Comment 7 2013-12-02 16:43:09 PST
Created attachment 218238
Brady Eidson
Comment 8 2013-12-02 17:24:37 PST
http://trac.webkit.org/changeset/159986
David Kilzer (:ddkilzer)
Comment 9 2014-01-19 21:00:08 PST
<rdar://problem/15217862>
Note You need to log in before you can comment on or make changes to this bug.