Bug 23025 – Simplifying DOMTimer lifetime management code

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Dmitry Titov]

Following IRC discussion...

DOMTimer lifetime is a bit confusing - it is created from DOMTimer::install() but then destroyed sometimes by direct 'delete' (when Document is destroyed) and sometimes as a side-effect (in Document::removeTimeout which should only remove an ID from the Document's timeout map).

Proposed change makes this code a bit more straightforward. The timeoutMap methods on the Document now do not delete the timers, they only add/remove/find timers in the map.
The timers are added to the map in their constructor and removed from the map in their destructor, so consistency is guaranteed.
Timers are destroyed by directly calling 'delete' in 3 places:
- when Document is destroyed while timers are still alive, in DOMTimer::contextDestroyed()
- when single-shot timer fired, in DOMTimer::fired()
- when JS calls clearTimer, in DOMTimer::removeById()

[Dmitry Titov]

Created attachment 26300 [details]
Proposed patch

[Darin Adler]

Comment on attachment 26300 [details]
Proposed patch

> -    static_cast<Document*>(context)->removeTimeout(timeoutId);
> +    DOMTimer* timer = static_cast<Document*>(context)->findTimeout(timeoutId);
> +    delete timer;

I personally would like this to just be a one liner without a local variable.

> -    ActiveDOMObject::contextDestroyed();
> +    ActiveDOMObject::contextDestroyed();  // Clears m_scriptExecutionContext.

Is adding this comment really an improvement?

Normally we use one space before the "//".

r=me

[Dmitry Titov]

Created attachment 26387 [details]
Patch with changes after review

Thanks Darin!!
All comments addressed. Seems ready to be landed.

[Darin Adler]

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