Changeset 241499 in webkit


Ignore:
Timestamp:
Feb 13, 2019, 10:43:13 PM (6 years ago)
Author:
rniwa@webkit.org
Message:

Crash in DOMTimer::fired
https://bugs.webkit.org/show_bug.cgi?id=194638

Reviewed by Brent Fulgham.

Source/WebCore:

This patch continues the saga of hunting down timer related crashes after r239814, r225985, r227934.

The crash was caused by the bug that we don't remove a DOMTimer from NestedTimersMap if a DOMTimer
is created & installed inside another DOMTimer's callback (via execute call in DOMTimer::fired).

Fixed the crash by using a Ref in NestedTimersMap. This will keep the timer alive until we exit
from DOMTimer::fired. Because DOMTimer::fired always calls stopTracking() which clears the map
we would not leak these DOM timers.

We could, alternatively, use WeakPtr in NestedTimersMap but that would unnecessarily increase the
size of DOMTimer for a very marginal benefit of DOMTimer objcets being deleted slightly earlier.
Deleting itself in DOMTimer's destructor involves more logic & house keeping in the timer code,
and is no longer the preferred approach when dealing with these classes of bugs in WebKit.

Test: fast/dom/timer-destruction-during-firing.html

  • page/DOMTimer.cpp:

(WebCore::NestedTimersMap::add):
(WebCore::DOMTimer::install):
(WebCore::DOMTimer::fired):

LayoutTests:

Added a regression test. It needs debug assertions without the fix.

  • fast/dom/timer-destruction-during-firing-expected.txt: Added.
  • fast/dom/timer-destruction-during-firing.html: Added.
Location:
trunk
Files:
2 added
3 edited

Legend:

Unmodified
Added
Removed
  • trunk/LayoutTests/ChangeLog

    r241497 r241499  
     12019-02-13  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Crash in DOMTimer::fired
     4        https://bugs.webkit.org/show_bug.cgi?id=194638
     5
     6        Reviewed by Brent Fulgham.
     7
     8        Added a regression test. It needs debug assertions without the fix.
     9
     10        * fast/dom/timer-destruction-during-firing-expected.txt: Added.
     11        * fast/dom/timer-destruction-during-firing.html: Added.
     12
    1132019-02-13  Nikita Vasilyev  <nvasilyev@apple.com>
    214
  • trunk/Source/WebCore/ChangeLog

    r241495 r241499  
     12019-02-13  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Crash in DOMTimer::fired
     4        https://bugs.webkit.org/show_bug.cgi?id=194638
     5
     6        Reviewed by Brent Fulgham.
     7
     8        This patch continues the saga of hunting down timer related crashes after r239814, r225985, r227934.
     9
     10        The crash was caused by the bug that we don't remove a DOMTimer from NestedTimersMap if a DOMTimer
     11        is created & installed inside another DOMTimer's callback (via execute call in DOMTimer::fired).
     12
     13        Fixed the crash by using a Ref in NestedTimersMap. This will keep the timer alive until we exit
     14        from DOMTimer::fired. Because DOMTimer::fired always calls stopTracking() which clears the map
     15        we would not leak these DOM timers.
     16
     17        We could, alternatively, use WeakPtr in NestedTimersMap but that would unnecessarily increase the
     18        size of DOMTimer for a very marginal benefit of DOMTimer objcets being deleted slightly earlier.
     19        Deleting itself in DOMTimer's destructor involves more logic & house keeping in the timer code,
     20        and is no longer the preferred approach when dealing with these classes of bugs in WebKit.
     21
     22        Test: fast/dom/timer-destruction-during-firing.html
     23
     24        * page/DOMTimer.cpp:
     25        (WebCore::NestedTimersMap::add):
     26        (WebCore::DOMTimer::install):
     27        (WebCore::DOMTimer::fired):
     28
    1292019-02-13  Joseph Pecoraro  <pecoraro@apple.com>
    230
  • trunk/Source/WebCore/page/DOMTimer.cpp

    r239427 r241499  
    114114
    115115struct NestedTimersMap {
    116     typedef HashMap<int, DOMTimer*>::const_iterator const_iterator;
     116    typedef HashMap<int, Ref<DOMTimer>>::const_iterator const_iterator;
    117117
    118118    static NestedTimersMap* instanceForContext(ScriptExecutionContext& context)
     
    140140    }
    141141
    142     void add(int timeoutId, DOMTimer* timer)
     142    void add(int timeoutId, Ref<DOMTimer>&& timer)
    143143    {
    144144        if (isTrackingNestedTimers)
    145             nestedTimers.add(timeoutId, timer);
     145            nestedTimers.add(timeoutId, WTFMove(timer));
    146146    }
    147147
     
    163163
    164164    static bool isTrackingNestedTimers;
    165     HashMap<int /* timeoutId */, DOMTimer*> nestedTimers;
     165    HashMap<int /* timeoutId */, Ref<DOMTimer>> nestedTimers;
    166166};
    167167
     
    236236    // Keep track of nested timer installs.
    237237    if (NestedTimersMap* nestedTimers = NestedTimersMap::instanceForContext(context))
    238         nestedTimers->add(timer->m_timeoutId, timer);
     238        nestedTimers->add(timer->m_timeoutId, *timer);
    239239
    240240    return timer->m_timeoutId;
     
    391391    // Check if we should throttle nested single-shot timers.
    392392    if (nestedTimers) {
    393         for (auto& keyValue : *nestedTimers) {
    394             auto* timer = keyValue.value;
     393        for (auto& idAndTimer : *nestedTimers) {
     394            auto& timer = idAndTimer.value;
    395395            if (timer->isActive() && !timer->repeatInterval())
    396396                timer->updateThrottlingStateIfNecessary(fireState);
Note: See TracChangeset for help on using the changeset viewer.