Bug 136198 - DOMTimer may be deleted during timer fire
Summary: DOMTimer may be deleted during timer fire
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks: 136197
  Show dependency treegraph
 
Reported: 2014-08-23 23:30 PDT by Gavin Barraclough
Modified: 2014-08-27 09:43 PDT (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 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.
Comment 1 Gavin Barraclough 2014-08-25 16:10:05 PDT
Created attachment 237115 [details]
Fix
Comment 2 WebKit Commit Bot 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.
Comment 3 Geoffrey Garen 2014-08-25 16:16:22 PDT
Comment on attachment 237115 [details]
Fix

r=me
Comment 4 Geoffrey Garen 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.
Comment 5 Gavin Barraclough 2014-08-26 10:06:02 PDT
Created attachment 237154 [details]
Alternative fix, address the problem for DOMTimer too.
Comment 6 Geoffrey Garen 2014-08-26 10:36:56 PDT
Comment on attachment 237154 [details]
Alternative fix, address the problem for DOMTimer too.

r=me
Comment 7 Gavin Barraclough 2014-08-26 10:58:29 PDT
Transmitting file data ......
Committed revision 172963.
Comment 8 Csaba Osztrogonác 2014-08-26 11:05:42 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.
Comment 9 Csaba Osztrogonác 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 .
Comment 10 Csaba Osztrogonác 2014-08-26 11:11:43 PDT
Tim already fixed it in http://trac.webkit.org/changeset/172964, thanks.
Comment 11 Darin Adler 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.
Comment 12 Darin Adler 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.