CJS_Timer should observe CJS_Runtime destruction.

Also remove dead CJS_EmbedObj::{Begin,End}Timer code.

BUG=539107
R=tsepez@chromium.org

Review URL: https://codereview.chromium.org/1384883002 .

5 files changed, 92 insertions, 82 deletions

diff --git a/fpdfsdk/include/javascript/JS_Object.h b/fpdfsdk/include/javascript/JS_Object.h
index c7f92ac61d..337da7ba20 100644
--- a/fpdfsdk/include/javascript/JS_Object.h
+++ b/fpdfsdk/include/javascript/JS_Object.h
@@ -15,11 +15,11 @@
 #include "../fsdk_mgr.h"          // For CPDFDoc_Environment
 #include "../fx_systemhandler.h"  // For IFX_SystemHandler
 #include "../jsapi/fxjs_v8.h"
+#include "JS_Runtime.h"
 
 class CPDFSDK_PageView;
 class CJS_Context;
 class CJS_Object;
-class CJS_Runtime;
 class CJS_Timer;
 
 class CJS_EmbedObj {
@@ -28,8 +28,6 @@ class CJS_EmbedObj {
   virtual ~CJS_EmbedObj();
 
   virtual void TimerProc(CJS_Timer* pTimer) {}
-  CJS_Timer* BeginTimer(CPDFDoc_Environment* pApp, FX_UINT nElapse);
-  void EndTimer(CJS_Timer* pTimer);
 
   CJS_Object* GetJSObject() const { return m_pJSObject; }
 
@@ -83,38 +81,22 @@ class CJS_Object {
   v8::Isolate* m_pIsolate;
 };
 
-class CJS_Timer {
+class CJS_Timer : public CJS_Runtime::Observer {
  public:
-  CJS_Timer(CJS_EmbedObj* pObj, CPDFDoc_Environment* pApp)
-      : m_nTimerID(0),
-        m_pEmbedObj(pObj),
-        m_bProcessing(FALSE),
-        m_dwStartTime(0),
-        m_dwTimeOut(0),
-        m_dwElapse(0),
-        m_pRuntime(NULL),
-        m_nType(0),
-        m_pApp(pApp) {}
-
-  virtual ~CJS_Timer() { KillJSTimer(); }
+  CJS_Timer(CJS_EmbedObj* pObj,
+            CPDFDoc_Environment* pApp,
+            CJS_Runtime* pRuntime,
+            int nType,
+            const CFX_WideString& script,
+            FX_DWORD dwElapse,
+            FX_DWORD dwTimeOut);
+  ~CJS_Timer() override;
 
- public:
-  FX_UINT SetJSTimer(FX_UINT nElapse);
   void KillJSTimer();
 
-  void SetType(int nType) { m_nType = nType; }
   int GetType() const { return m_nType; }
-
-  void SetStartTime(FX_DWORD dwStartTime) { m_dwStartTime = dwStartTime; }
-  FX_DWORD GetStartTime() const { return m_dwStartTime; }
-
-  void SetTimeOut(FX_DWORD dwTimeOut) { m_dwTimeOut = dwTimeOut; }
   FX_DWORD GetTimeOut() const { return m_dwTimeOut; }
-
-  void SetRuntime(CJS_Runtime* pRuntime) { m_pRuntime = pRuntime; }
-  CJS_Runtime* GetRuntime() const { return m_pRuntime; }
-
-  void SetJScript(const CFX_WideString& script) { m_swJScript = script; }
+  CJS_Runtime* GetRuntime() const { return m_bValid ? m_pRuntime : nullptr; }
   CFX_WideString GetJScript() const { return m_swJScript; }
 
   static void TimerProc(int idEvent);
@@ -123,19 +105,20 @@ class CJS_Timer {
   using TimerMap = std::map<FX_UINT, CJS_Timer*>;
   static TimerMap* GetGlobalTimerMap();
 
-  FX_UINT m_nTimerID;
-  CJS_EmbedObj* m_pEmbedObj;
-  FX_BOOL m_bProcessing;
+  // CJS_Runtime::Observer
+  void OnDestroyed() override;
+
+  FX_DWORD m_nTimerID;
+  CJS_EmbedObj* const m_pEmbedObj;
+  bool m_bProcessing;
+  bool m_bValid;
 
   // data
-  FX_DWORD m_dwStartTime;
-  FX_DWORD m_dwTimeOut;
-  FX_DWORD m_dwElapse;
-  CJS_Runtime* m_pRuntime;
-  CFX_WideString m_swJScript;
-  int m_nType;  // 0:Interval; 1:TimeOut
-
-  CPDFDoc_Environment* m_pApp;
+  const int m_nType;  // 0:Interval; 1:TimeOut
+  const FX_DWORD m_dwTimeOut;
+  const CFX_WideString m_swJScript;
+  CJS_Runtime* const m_pRuntime;
+  CPDFDoc_Environment* const m_pApp;
 };
 
 #endif  // FPDFSDK_INCLUDE_JAVASCRIPT_JS_OBJECT_H_
diff --git a/fpdfsdk/include/javascript/JS_Runtime.h b/fpdfsdk/include/javascript/JS_Runtime.h
index dd21e6d2f1..6fdc4b0708 100644
--- a/fpdfsdk/include/javascript/JS_Runtime.h
+++ b/fpdfsdk/include/javascript/JS_Runtime.h
@@ -19,6 +19,14 @@ class CJS_Context;
 
 class CJS_Runtime : public IFXJS_Runtime {
  public:
+  class Observer {
+   public:
+    virtual void OnDestroyed() = 0;
+
+   protected:
+    virtual ~Observer() {}
+  };
+
   using FieldEvent = std::pair<CFX_WideString, JS_EVENT_T>;
 
   explicit CJS_Runtime(CPDFDoc_Environment* pApp);
@@ -44,6 +52,9 @@ class CJS_Runtime : public IFXJS_Runtime {
   v8::Isolate* GetIsolate() const { return m_isolate; }
   v8::Local<v8::Context> NewJSContext();
 
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
  private:
   void DefineJSObjects();
 
@@ -55,6 +66,7 @@ class CJS_Runtime : public IFXJS_Runtime {
   v8::Isolate* m_isolate;
   bool m_isolateManaged;
   v8::Global<v8::Context> m_context;
+  std::set<Observer*> m_observers;
 };
 
 #endif  // FPDFSDK_INCLUDE_JAVASCRIPT_JS_RUNTIME_H_
diff --git a/fpdfsdk/src/javascript/JS_Object.cpp b/fpdfsdk/src/javascript/JS_Object.cpp
index 3cffec785d..7520a99778 100644
--- a/fpdfsdk/src/javascript/JS_Object.cpp
+++ b/fpdfsdk/src/javascript/JS_Object.cpp
@@ -56,20 +56,6 @@ void CJS_EmbedObj::Alert(CJS_Context* pContext, const FX_WCHAR* swMsg) {
   CJS_Object::Alert(pContext, swMsg);
 }
 
-CJS_Timer* CJS_EmbedObj::BeginTimer(CPDFDoc_Environment* pApp,
-                                    FX_UINT nElapse) {
-  CJS_Timer* pTimer = new CJS_Timer(this, pApp);
-  pTimer->SetJSTimer(nElapse);
-
-  return pTimer;
-}
-
-void CJS_EmbedObj::EndTimer(CJS_Timer* pTimer) {
-  ASSERT(pTimer != NULL);
-  pTimer->KillJSTimer();
-  delete pTimer;
-}
-
 void FreeObject(const v8::WeakCallbackInfo<CJS_Object>& data) {
   CJS_Object* pJSObj = data.GetParameter();
   pJSObj->ExitInstance();
@@ -123,20 +109,40 @@ void CJS_Object::Alert(CJS_Context* pContext, const FX_WCHAR* swMsg) {
   }
 }
 
-FX_UINT CJS_Timer::SetJSTimer(FX_UINT nElapse) {
-  if (m_nTimerID)
-    KillJSTimer();
+CJS_Timer::CJS_Timer(CJS_EmbedObj* pObj,
+                     CPDFDoc_Environment* pApp,
+                     CJS_Runtime* pRuntime,
+                     int nType,
+                     const CFX_WideString& script,
+                     FX_DWORD dwElapse,
+                     FX_DWORD dwTimeOut)
+    : m_nTimerID(0),
+      m_pEmbedObj(pObj),
+      m_bProcessing(false),
+      m_bValid(true),
+      m_nType(nType),
+      m_dwTimeOut(dwTimeOut),
+      m_pRuntime(pRuntime),
+      m_pApp(pApp) {
   IFX_SystemHandler* pHandler = m_pApp->GetSysHandler();
-  m_nTimerID = pHandler->SetTimer(nElapse, TimerProc);
+  m_nTimerID = pHandler->SetTimer(dwElapse, TimerProc);
   (*GetGlobalTimerMap())[m_nTimerID] = this;
-  m_dwElapse = nElapse;
-  return m_nTimerID;
+  m_pRuntime->AddObserver(this);
+}
+
+CJS_Timer::~CJS_Timer() {
+  CJS_Runtime* pRuntime = GetRuntime();
+  if (pRuntime)
+    pRuntime->RemoveObserver(this);
+  KillJSTimer();
 }
 
 void CJS_Timer::KillJSTimer() {
   if (m_nTimerID) {
-    IFX_SystemHandler* pHandler = m_pApp->GetSysHandler();
-    pHandler->KillTimer(m_nTimerID);
+    if (m_bValid) {
+      IFX_SystemHandler* pHandler = m_pApp->GetSysHandler();
+      pHandler->KillTimer(m_nTimerID);
+    }
     GetGlobalTimerMap()->erase(m_nTimerID);
     m_nTimerID = 0;
   }
@@ -148,10 +154,10 @@ void CJS_Timer::TimerProc(int idEvent) {
   if (it != GetGlobalTimerMap()->end()) {
     CJS_Timer* pTimer = it->second;
     if (!pTimer->m_bProcessing) {
-      pTimer->m_bProcessing = TRUE;
+      CFX_AutoRestorer<bool> scoped_processing(&pTimer->m_bProcessing);
+      pTimer->m_bProcessing = true;
       if (pTimer->m_pEmbedObj)
         pTimer->m_pEmbedObj->TimerProc(pTimer);
-      pTimer->m_bProcessing = FALSE;
     }
   }
 }
@@ -162,3 +168,7 @@ CJS_Timer::TimerMap* CJS_Timer::GetGlobalTimerMap() {
   static auto* s_TimerMap = new TimerMap;
   return s_TimerMap;
 }
+
+void CJS_Timer::OnDestroyed() {
+  m_bValid = false;
+}
diff --git a/fpdfsdk/src/javascript/JS_Runtime.cpp b/fpdfsdk/src/javascript/JS_Runtime.cpp
index 5c463ce498..4c502a7b75 100644
--- a/fpdfsdk/src/javascript/JS_Runtime.cpp
+++ b/fpdfsdk/src/javascript/JS_Runtime.cpp
@@ -12,7 +12,6 @@
 #include "../../include/javascript/JS_Define.h"
 #include "../../include/javascript/JS_Object.h"
 #include "../../include/javascript/JS_Value.h"
-#include "../../include/javascript/Document.h"
 #include "../../include/javascript/app.h"
 #include "../../include/javascript/color.h"
 #include "../../include/javascript/Consts.h"
@@ -54,6 +53,9 @@ CJS_Runtime::CJS_Runtime(CPDFDoc_Environment* pApp)
 }
 
 CJS_Runtime::~CJS_Runtime() {
+  for (auto* obs : m_observers)
+    obs->OnDestroyed();
+
   for (int i = 0, sz = m_ContextArray.GetSize(); i < sz; i++)
     delete m_ContextArray.GetAt(i);
 
@@ -176,6 +178,16 @@ v8::Local<v8::Context> CJS_Runtime::NewJSContext() {
   return v8::Local<v8::Context>::New(m_isolate, m_context);
 }
 
+void CJS_Runtime::AddObserver(Observer* observer) {
+  ASSERT(m_observers.find(observer) == m_observers.end());
+  m_observers.insert(observer);
+}
+
+void CJS_Runtime::RemoveObserver(Observer* observer) {
+  ASSERT(m_observers.find(observer) != m_observers.end());
+  m_observers.erase(observer);
+}
+
 CFX_WideString ChangeObjName(const CFX_WideString& str) {
   CFX_WideString sRet = str;
   sRet.Replace(L"_", L".");
diff --git a/fpdfsdk/src/javascript/app.cpp b/fpdfsdk/src/javascript/app.cpp
index 9d4fa9ca13..067ae108bd 100644
--- a/fpdfsdk/src/javascript/app.cpp
+++ b/fpdfsdk/src/javascript/app.cpp
@@ -408,16 +408,10 @@ FX_BOOL app::setInterval(IFXJS_Context* cc,
 
   CPDFDoc_Environment* pApp = pRuntime->GetReaderApp();
   ASSERT(pApp);
-  CJS_Timer* pTimer = new CJS_Timer(this, pApp);
+  CJS_Timer* pTimer =
+      new CJS_Timer(this, pApp, pRuntime, 0, script, dwInterval, 0);
   m_aTimer.Add(pTimer);
 
-  pTimer->SetType(0);
-  pTimer->SetRuntime(pRuntime);
-  pTimer->SetJScript(script);
-  pTimer->SetTimeOut(0);
-  //	pTimer->SetStartTime(GetTickCount());
-  pTimer->SetJSTimer(dwInterval);
-
   v8::Local<v8::Object> pRetObj = FXJS_NewFxDynamicObj(
       pRuntime->GetIsolate(), pContext,
       FXJS_GetObjDefnID(pRuntime->GetIsolate(), L"TimerObj"));
@@ -462,15 +456,10 @@ FX_BOOL app::setTimeOut(IFXJS_Context* cc,
   CPDFDoc_Environment* pApp = pRuntime->GetReaderApp();
   ASSERT(pApp);
 
-  CJS_Timer* pTimer = new CJS_Timer(this, pApp);
+  CJS_Timer* pTimer =
+      new CJS_Timer(this, pApp, pRuntime, 1, script, dwTimeOut, dwTimeOut);
   m_aTimer.Add(pTimer);
 
-  pTimer->SetType(1);
-  pTimer->SetRuntime(pRuntime);
-  pTimer->SetJScript(script);
-  pTimer->SetTimeOut(dwTimeOut);
-  pTimer->SetJSTimer(dwTimeOut);
-
   v8::Local<v8::Object> pRetObj = FXJS_NewFxDynamicObj(
       pRuntime->GetIsolate(), pContext,
       FXJS_GetObjDefnID(pRuntime->GetIsolate(), L"TimerObj"));
@@ -585,13 +574,17 @@ FX_BOOL app::execMenuItem(IFXJS_Context* cc,
 void app::TimerProc(CJS_Timer* pTimer) {
   ASSERT(pTimer != NULL);
 
+  CJS_Runtime* pRuntime = pTimer->GetRuntime();
+
   switch (pTimer->GetType()) {
     case 0:  // interval
-      RunJsScript(pTimer->GetRuntime(), pTimer->GetJScript());
+      if (pRuntime)
+        RunJsScript(pRuntime, pTimer->GetJScript());
       break;
     case 1:
       if (pTimer->GetTimeOut() > 0) {
-        RunJsScript(pTimer->GetRuntime(), pTimer->GetJScript());
+        if (pRuntime)
+          RunJsScript(pRuntime, pTimer->GetJScript());
         pTimer->KillJSTimer();
       }
       break;