Changeset 35964 in webkit


Ignore:
Timestamp:
Aug 28, 2008 4:42:26 AM (16 years ago)
Author:
eric@webkit.org
Message:

Reviewed by Darin and Alexey.

Close a leak of PausedTimeouts if the JavaScriptDebugServer was destroyed
with timeouts paused.
https://bugs.webkit.org/show_bug.cgi?id=20469

I attempted to clean up the memory management of PausedTimeouts, I'm not
sure the solution I came up with is "cleaner", but it's in some ways
"safer", since it no longer uses raw pointers and manual new/delete.

This also now prevents CachedPage from needlessly creating Window
objects when caching pages which didn't already have one. :)

I also made Chrome.cpp no longer depend on the JavaScript bindings
(aka JSDOMWindowBase.h), since there was no real reason for it to.

  • bindings/js/JSDOMWindowBase.cpp: (WebCore::JSDOMWindowBase::pauseTimeouts): (WebCore::JSDOMWindowBase::resumeTimeouts):
  • bindings/js/JSDOMWindowBase.h:
  • bindings/js/ScriptController.cpp: (WebCore::ScriptController::pauseTimeouts): (WebCore::ScriptController::resumeTimeouts):
  • bindings/js/ScriptController.h:
  • history/CachedPage.cpp: (WebCore::CachedPage::CachedPage): (WebCore::CachedPage::restore):
  • page/Chrome.cpp: (WebCore::PageGroupLoadDeferrer::PageGroupLoadDeferrer): (WebCore::PageGroupLoadDeferrer::~PageGroupLoadDeferrer):
  • page/JavaScriptDebugServer.cpp: (WebCore::JavaScriptDebugServer::~JavaScriptDebugServer): (WebCore::JavaScriptDebugServer::setJavaScriptPaused):
Location:
trunk/WebCore
Files:
8 edited

Legend:

Unmodified
Added
Removed
  • trunk/WebCore/ChangeLog

    r35963 r35964  
     12008-08-20  Eric Seidel  <eric@webkit.org>
     2
     3        Reviewed by Darin and Alexey.
     4
     5        Close a leak of PausedTimeouts if the JavaScriptDebugServer was destroyed
     6        with timeouts paused.
     7        https://bugs.webkit.org/show_bug.cgi?id=20469
     8       
     9        I attempted to clean up the memory management of PausedTimeouts, I'm not
     10        sure the solution I came up with is "cleaner", but it's in some ways
     11        "safer", since it no longer uses raw pointers and manual new/delete.
     12       
     13        This also now prevents CachedPage from needlessly creating Window
     14        objects when caching pages which didn't already have one. :)
     15       
     16        I also made Chrome.cpp no longer depend on the JavaScript bindings
     17        (aka JSDOMWindowBase.h), since there was no real reason for it to.
     18
     19        * bindings/js/JSDOMWindowBase.cpp:
     20        (WebCore::JSDOMWindowBase::pauseTimeouts):
     21        (WebCore::JSDOMWindowBase::resumeTimeouts):
     22        * bindings/js/JSDOMWindowBase.h:
     23        * bindings/js/ScriptController.cpp:
     24        (WebCore::ScriptController::pauseTimeouts):
     25        (WebCore::ScriptController::resumeTimeouts):
     26        * bindings/js/ScriptController.h:
     27        * history/CachedPage.cpp:
     28        (WebCore::CachedPage::CachedPage):
     29        (WebCore::CachedPage::restore):
     30        * page/Chrome.cpp:
     31        (WebCore::PageGroupLoadDeferrer::PageGroupLoadDeferrer):
     32        (WebCore::PageGroupLoadDeferrer::~PageGroupLoadDeferrer):
     33        * page/JavaScriptDebugServer.cpp:
     34        (WebCore::JavaScriptDebugServer::~JavaScriptDebugServer):
     35        (WebCore::JavaScriptDebugServer::setJavaScriptPaused):
     36
    1372008-08-27  Holger Hans Peter Freyther <zecke@selfish.org>
    238
  • trunk/WebCore/bindings/js/JSDOMWindowBase.cpp

    r35853 r35964  
    12111211}
    12121212
    1213 PausedTimeouts* JSDOMWindowBase::pauseTimeouts()
    1214 {
    1215     size_t count = d()->timeouts.size();
    1216     if (count == 0)
    1217         return 0;
    1218 
    1219     PausedTimeout* t = new PausedTimeout [count];
    1220     PausedTimeouts* result = new PausedTimeouts(t, count);
     1213void JSDOMWindowBase::pauseTimeouts(OwnPtr<PausedTimeouts>& result)
     1214{
     1215    size_t timeoutsCount = d()->timeouts.size();
     1216    if (!timeoutsCount) {
     1217        result.clear();
     1218        return;
     1219    }
     1220
     1221    PausedTimeout* t = new PausedTimeout[timeoutsCount];
     1222    result.set(new PausedTimeouts(t, timeoutsCount));
    12211223
    12221224    JSDOMWindowBaseData::TimeoutsMap::iterator it = d()->timeouts.begin();
    1223     for (size_t i = 0; i != count; ++i, ++it) {
     1225    for (size_t i = 0; i != timeoutsCount; ++i, ++it) {
    12241226        int timeoutId = it->first;
    12251227        DOMWindowTimer* timer = it->second;
     
    12341236    deleteAllValues(d()->timeouts);
    12351237    d()->timeouts.clear();
    1236 
    1237     return result;
    1238 }
    1239 
    1240 void JSDOMWindowBase::resumeTimeouts(PausedTimeouts* timeouts)
     1238}
     1239
     1240void JSDOMWindowBase::resumeTimeouts(OwnPtr<PausedTimeouts>& timeouts)
    12411241{
    12421242    if (!timeouts)
     
    12511251    }
    12521252    delete [] array;
     1253    timeouts.clear();
    12531254}
    12541255
  • trunk/WebCore/bindings/js/JSDOMWindowBase.h

    r35807 r35964  
    7272        int installTimeout(KJS::ExecState*, KJS::JSValue* function, const KJS::ArgList& args, int t, bool singleShot);
    7373        void clearTimeout(int timerId, bool delAction = true);
    74         PausedTimeouts* pauseTimeouts();
    75         void resumeTimeouts(PausedTimeouts*);
     74
     75        void pauseTimeouts(OwnPtr<PausedTimeouts>&);
     76        void resumeTimeouts(OwnPtr<PausedTimeouts>&);
    7677
    7778        void timerFired(DOMWindowTimer*);
  • trunk/WebCore/bindings/js/ScriptController.cpp

    r35853 r35964  
    3737#include "Page.h"
    3838#include "PageGroup.h"
     39#include "PausedTimeouts.h"
    3940#include "runtime_root.h"
    4041#include "Settings.h"
     
    361362}
    362363
     364void ScriptController::pauseTimeouts(OwnPtr<PausedTimeouts>& result)
     365{
     366    if (!haveWindowShell()) {
     367        result.clear();
     368        return;
     369    }
     370
     371    windowShell()->window()->pauseTimeouts(result);
     372}
     373
     374void ScriptController::resumeTimeouts(OwnPtr<PausedTimeouts>& pausedTimeouts)
     375{
     376    if (!haveWindowShell()) {
     377        // Callers can assume we will always clear the passed in timeouts
     378        pausedTimeouts.clear();
     379        return;
     380    }
     381
     382    windowShell()->window()->resumeTimeouts(pausedTimeouts);
     383}
     384
    363385} // namespace WebCore
  • trunk/WebCore/bindings/js/ScriptController.h

    r35591 r35964  
    104104    void updateDocument();
    105105
     106    void pauseTimeouts(OwnPtr<PausedTimeouts>&);
     107    void resumeTimeouts(OwnPtr<PausedTimeouts>&);
     108
    106109    void clearScriptObjects();
    107110    void cleanupScriptObjectsForPlugin(void*);
  • trunk/WebCore/history/CachedPage.cpp

    r35853 r35964  
    8787    if (proxy->haveWindowShell()) {
    8888        m_window = proxy->windowShell()->window();
    89         m_pausedTimeouts.set(m_window->pauseTimeouts());
     89        m_window->pauseTimeouts(m_pausedTimeouts);
    9090    }
    9191
     
    115115        if (m_window) {
    116116            windowShell->setWindow(m_window.get());
    117             windowShell->window()->resumeTimeouts(m_pausedTimeouts.get());
     117            windowShell->window()->resumeTimeouts(m_pausedTimeouts);
    118118        } else {
    119119            windowShell->setWindow(new (JSDOMWindow::commonJSGlobalData()) JSDOMWindow(mainFrame->domWindow(), windowShell));
  • trunk/WebCore/page/Chrome.cpp

    r35953 r35964  
    3030#include "HTMLNames.h"
    3131#include "HitTestResult.h"
    32 #include "JSDOMWindow.h"
    3332#include "Page.h"
    3433#include "PageGroup.h"
     
    429428#if !PLATFORM(MAC)
    430429            for (Frame* frame = otherPage->mainFrame(); frame; frame = frame->tree()->traverseNext()) {
    431                 if (JSDOMWindow* window = toJSDOMWindow(frame)) {
    432                     PausedTimeouts* timeouts = window->pauseTimeouts();
    433 
    434                     m_pausedTimeouts.append(make_pair(RefPtr<Frame>(frame), timeouts));
    435                 }
     430                OwnPtr<PausedTimeouts> timeouts;
     431                frame->script()->pauseTimeouts(timeouts);
     432                if (timeouts)
     433                    m_pausedTimeouts.append(make_pair(RefPtr<Frame>(frame), timeouts.take()));
    436434            }
    437435#endif
     
    449447    if (!m_wasDeferringTimers)
    450448        setDeferringTimers(false);
    451    
    452     size_t count = m_deferredFrames.size();
    453     for (size_t i = 0; i < count; ++i)
     449
     450    for (size_t i = 0; i < m_deferredFrames.size(); ++i)
    454451        if (Page* page = m_deferredFrames[i]->page())
    455452            page->setDefersLoading(false);
    456453
    457454#if !PLATFORM(MAC)
    458     count = m_pausedTimeouts.size();
    459 
    460     for (size_t i = 0; i < count; i++) {
    461         JSDOMWindow* window = toJSDOMWindow(m_pausedTimeouts[i].first.get());
    462         if (window)
    463             window->resumeTimeouts(m_pausedTimeouts[i].second);
    464         delete m_pausedTimeouts[i].second;
     455    for (size_t i = 0; i < m_pausedTimeouts.size(); i++) {
     456        Frame* frame = m_pausedTimeouts[i].first.get();
     457        OwnPtr<PausedTimeouts> timeouts(m_pausedTimeouts[i].second);
     458        frame->script()->resumeTimeouts(timeouts);
    465459    }
    466460#endif
  • trunk/WebCore/page/JavaScriptDebugServer.cpp

    r35908 r35964  
    4040#include "Page.h"
    4141#include "PageGroup.h"
     42#include "PausedTimeouts.h"
    4243#include "PluginView.h"
    4344#include "ScrollView.h"
     
    7475    deleteAllValues(m_pageListenersMap);
    7576    deleteAllValues(m_breakpoints);
     77    deleteAllValues(m_pausedTimeouts);
    7678}
    7779
     
    357359
    358360    if (JSDOMWindow* window = toJSDOMWindow(frame)) {
    359         if (paused)
    360             m_pausedTimeouts.set(frame, window->pauseTimeouts());
    361         else
    362             window->resumeTimeouts(m_pausedTimeouts.take(frame));
     361        if (paused) {
     362            OwnPtr<PausedTimeouts> timeouts;
     363            window->pauseTimeouts(timeouts);
     364            m_pausedTimeouts.set(frame, timeouts.release());
     365        } else {
     366            OwnPtr<PausedTimeouts> timeouts(m_pausedTimeouts.take(frame));
     367            window->resumeTimeouts(timeouts);
     368        }
    363369    }
    364370
Note: See TracChangeset for help on using the changeset viewer.