Bug 136198

Summary:

DOMTimer may be deleted during timer fire

Product:

WebKit

Reporter:

Gavin Barraclough <barraclough>

Component:

Bindings

Assignee:

Gavin Barraclough <barraclough>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, commit-queue, darin, esprehn+autocc, ggaren, kangil.han, ossy, thorton

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

136197

Attachments:

Description	Flags
Fix	ggaren: review+
Alternative fix, address the problem for DOMTimer too.	ggaren: review+

Gavin Barraclough

Reported 2014-08-23 23:30:19 PDT

For one-shot timers we move ownership of the ScheduledAction to a local RAII variable, so they'll survive until timer fire completes. However for interval timers the ScheduledAction remains owned by the DOMTimer, which could cancel itself from script. Change ownership model of ScheduledAction so even if the DOMTimer is cancelled the ScheduledAction won't be deleted mid execution.

Attachments
Fix (16.16 KB, patch) 2014-08-25 16:10 PDT, Gavin Barraclough	ggaren: review+	Details Formatted Diff Diff
Alternative fix, address the problem for DOMTimer too. (14.85 KB, patch) 2014-08-26 10:06 PDT, Gavin Barraclough	ggaren: review+	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Gavin Barraclough

Comment 1 2014-08-25 16:10:05 PDT

Created attachment 237115 [details] Fix

WebKit Commit Bot

Comment 2 2014-08-25 16:13:01 PDT

Attachment 237115 [details] did not pass style-queue: ERROR: Source/WebCore/page/DOMTimer.h:35: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Geoffrey Garen

Comment 3 2014-08-25 16:16:22 PDT

Comment on attachment 237115 [details] Fix r=me

Geoffrey Garen

Comment 4 2014-08-25 16:19:32 PDT

Something I said in person: Perhaps ScriptExecutionContext::m_timeouts should just be a map of RefPtr instead of raw pointer.

Gavin Barraclough

Comment 5 2014-08-26 10:06:02 PDT

Created attachment 237154 [details] Alternative fix, address the problem for DOMTimer too.

Geoffrey Garen

Comment 6 2014-08-26 10:36:56 PDT

Comment on attachment 237154 [details] Alternative fix, address the problem for DOMTimer too. r=me

Gavin Barraclough

Comment 7 2014-08-26 10:58:29 PDT

Transmitting file data ...... Committed revision 172963.

Csaba Osztrogonác

Comment 8 2014-08-26 11:05:42 PDT

Csaba Osztrogonác

Comment 9 2014-08-26 11:10:18 PDT

It seems, it isn't Linux issue, Apple Mac build is broken too. I assume, you need wtf/RefCounted.h instead of WTF/RefCounted.h .

Csaba Osztrogonác

Comment 10 2014-08-26 11:11:43 PDT

Tim already fixed it in http://trac.webkit.org/changeset/172964, thanks.

Darin Adler

Comment 11 2014-08-27 09:42:33 PDT

Comment on attachment 237154 [details] Alternative fix, address the problem for DOMTimer too. View in context: https://bugs.webkit.org/attachment.cgi?id=237154&action=review >> Source/WebCore/page/DOMTimer.h:31 >> +#include <WTF/RefCounted.h> > > It broke the Linux (EFL,GTK) builds, because this kind of magic includes work only on Mac. Slightly incorrect analysis. It’s not about “magic includes”, but simply case sensitive file systems. The correct include is <wtf/RefCounted.h>. Using <WTF/RefCounted.h> instead breaks builds on Macs with case sensitive file systems too. However, the HFS case insensitive file system is the one that many of us have on our computers.

Darin Adler

Comment 12 2014-08-27 09:43:01 PDT

(In reply to comment #9) > It seems, it isn't Linux issue, Apple Mac build is broken too. > I assume, you need wtf/RefCounted.h instead of WTF/RefCounted.h . Oops, I see you figured this out. Sorry for the repetitive comment.

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