Remove another potential stale CJS_Timer usage

Fix memory ownership model for PDFium timers.

The |app| class owns the CJS_Timer as part of its vector<unique_ptr>
to them.

The CJS_Timer "owns" its slot in the global ID to timer map, and
removes itself when it is destroyed. Nothing else deletes
from the global map. Deleting from the global map is
accompanied by a callback to the embedder to clear its
resources.

Next, the proper way to remove a CJS_Timer is by going
through the app, and having the app erase its unique ptr,
which then deletes the CJS_Timer, which in turn cleans up the
global map. Provide a CJS_Timer::Cancel static method to
do this conveniently.

There is a alternate path to the CJS_timer via JS and its
CJS_TimerObj. CJS_TimerObj owns a TimerObj that currently
points to the CJS_Timer. If the timer fires, and cleans
itself up, this can go stale.

Make the TimerObj maintain a weak reference via global
timer ID rather than a direct pointer to the CJS_Timer, so
that if the timer fires and is destroyed, future attempts
to cancel find nothing.

There is another path, where if the JS timer object is GC'd, then we
just clean up its CJS_TimerObj without touching
the actual CJS_Timers.  We could make this match the spec
by calling into the new cancel routine as described above,
but it seems weird to have a timer depend on whether a gc
happened or not.

A subsequent CL will rename these objects to more closely
match the conventions used by the other JS wrappers.

BUG=634716

Review-Url: https://codereview.chromium.org/2221513002

4 files changed, 46 insertions, 55 deletions

diff --git a/fpdfsdk/javascript/JS_Object.cpp b/fpdfsdk/javascript/JS_Object.cpp
index 9ec316303d..b65699a18b 100644
--- a/fpdfsdk/javascript/JS_Object.cpp
+++ b/fpdfsdk/javascript/JS_Object.cpp
@@ -90,7 +90,7 @@ CJS_Timer::CJS_Timer(CJS_EmbedObj* pObj,
       m_pRuntime(pRuntime),
       m_pApp(pApp) {
   CFX_SystemHandler* pHandler = m_pApp->GetSysHandler();
-  m_nTimerID = pHandler->SetTimer(dwElapse, TimerProc);
+  m_nTimerID = pHandler->SetTimer(dwElapse, Trigger);
   (*GetGlobalTimerMap())[m_nTimerID] = this;
   m_pRuntime->AddObserver(this);
 }
@@ -99,23 +99,19 @@ CJS_Timer::~CJS_Timer() {
   CJS_Runtime* pRuntime = GetRuntime();
   if (pRuntime)
     pRuntime->RemoveObserver(this);
-  KillJSTimer();
-}
 
-void CJS_Timer::KillJSTimer() {
-  if (m_nTimerID) {
-    if (m_bValid) {
-      CFX_SystemHandler* pHandler = m_pApp->GetSysHandler();
-      pHandler->KillTimer(m_nTimerID);
-    }
-    GetGlobalTimerMap()->erase(m_nTimerID);
-    m_nTimerID = 0;
-  }
+  if (!m_nTimerID)
+    return;
+
+  if (m_bValid)
+    m_pApp->GetSysHandler()->KillTimer(m_nTimerID);
+
+  GetGlobalTimerMap()->erase(m_nTimerID);
 }
 
 // static
-void CJS_Timer::TimerProc(int idEvent) {
-  auto it = GetGlobalTimerMap()->find(idEvent);
+void CJS_Timer::Trigger(int nTimerID) {
+  auto it = GetGlobalTimerMap()->find(nTimerID);
   if (it == GetGlobalTimerMap()->end())
     return;
 
@@ -128,12 +124,24 @@ void CJS_Timer::TimerProc(int idEvent) {
     pTimer->m_pEmbedObj->TimerProc(pTimer);
 
   // Timer proc may have destroyed timer, find it again.
-  it = GetGlobalTimerMap()->find(idEvent);
+  it = GetGlobalTimerMap()->find(nTimerID);
   if (it == GetGlobalTimerMap()->end())
     return;
 
   pTimer = it->second;
   pTimer->m_bProcessing = false;
+  if (pTimer->IsOneShot())
+    pTimer->m_pEmbedObj->CancelProc(pTimer);
+}
+
+// static
+void CJS_Timer::Cancel(int nTimerID) {
+  auto it = GetGlobalTimerMap()->find(nTimerID);
+  if (it == GetGlobalTimerMap()->end())
+    return;
+
+  CJS_Timer* pTimer = it->second;
+  pTimer->m_pEmbedObj->CancelProc(pTimer);
 }
 
 // static
diff --git a/fpdfsdk/javascript/JS_Object.h b/fpdfsdk/javascript/JS_Object.h
index 146a7c93b5..aec94c0362 100644
--- a/fpdfsdk/javascript/JS_Object.h
+++ b/fpdfsdk/javascript/JS_Object.h
@@ -18,12 +18,14 @@ class CJS_Context;
 class CJS_Object;
 class CJS_Timer;
 class CPDFDoc_Environment;
+
 class CJS_EmbedObj {
  public:
   explicit CJS_EmbedObj(CJS_Object* pJSObject);
   virtual ~CJS_EmbedObj();
 
   virtual void TimerProc(CJS_Timer* pTimer) {}
+  virtual void CancelProc(CJS_Timer* pTimer) {}
 
   CJS_Object* GetJSObject() const { return m_pJSObject; }
 
@@ -76,15 +78,15 @@ class CJS_Timer : public CJS_Runtime::Observer {
             uint32_t dwTimeOut);
   ~CJS_Timer() override;
 
-  void KillJSTimer();
+  static void Trigger(int nTimerID);
+  static void Cancel(int nTimerID);
 
-  int GetType() const { return m_nType; }
+  bool IsOneShot() const { return m_nType == 1; }
   uint32_t GetTimeOut() const { return m_dwTimeOut; }
+  int GetTimerID() const { return m_nTimerID; }
   CJS_Runtime* GetRuntime() const { return m_bValid ? m_pRuntime : nullptr; }
   CFX_WideString GetJScript() const { return m_swJScript; }
 
-  static void TimerProc(int idEvent);
-
  private:
   using TimerMap = std::map<FX_UINT, CJS_Timer*>;
   static TimerMap* GetGlobalTimerMap();
diff --git a/fpdfsdk/javascript/app.cpp b/fpdfsdk/javascript/app.cpp
index 1077629244..329b625545 100644
--- a/fpdfsdk/javascript/app.cpp
+++ b/fpdfsdk/javascript/app.cpp
@@ -31,16 +31,12 @@ END_JS_STATIC_METHOD()
 IMPLEMENT_JS_CLASS(CJS_TimerObj, TimerObj)
 
 TimerObj::TimerObj(CJS_Object* pJSObject)
-    : CJS_EmbedObj(pJSObject), m_pTimer(nullptr) {}
+    : CJS_EmbedObj(pJSObject), m_nTimerID(0) {}
 
 TimerObj::~TimerObj() {}
 
 void TimerObj::SetTimer(CJS_Timer* pTimer) {
-  m_pTimer = pTimer;
-}
-
-CJS_Timer* TimerObj::GetTimer() const {
-  return m_pTimer;
+  m_nTimerID = pTimer->GetTimerID();
 }
 
 #define JS_STR_VIEWERTYPE L"pdfium"
@@ -473,20 +469,7 @@ void app::ClearTimerCommon(const CJS_Value& param) {
   if (!pTimerObj)
     return;
 
-  CJS_Timer* pTimer = pTimerObj->GetTimer();
-  if (!pTimer)
-    return;
-
-  pTimer->KillJSTimer();
-  auto iter = std::find_if(m_Timers.begin(), m_Timers.end(),
-                           [pTimer](const std::unique_ptr<CJS_Timer>& that) {
-                             return pTimer == that.get();
-                           });
-
-  if (iter != m_Timers.end())
-    m_Timers.erase(iter);
-
-  pTimerObj->SetTimer(nullptr);
+  CJS_Timer::Cancel(pTimerObj->GetTimerID());
 }
 
 FX_BOOL app::execMenuItem(IJS_Context* cc,
@@ -498,20 +481,18 @@ FX_BOOL app::execMenuItem(IJS_Context* cc,
 
 void app::TimerProc(CJS_Timer* pTimer) {
   CJS_Runtime* pRuntime = pTimer->GetRuntime();
+  if (pRuntime && (!pTimer->IsOneShot() || pTimer->GetTimeOut() > 0))
+    RunJsScript(pRuntime, pTimer->GetJScript());
+}
 
-  switch (pTimer->GetType()) {
-    case 0:  // interval
-      if (pRuntime)
-        RunJsScript(pRuntime, pTimer->GetJScript());
-      break;
-    case 1:
-      if (pTimer->GetTimeOut() > 0) {
-        if (pRuntime)
-          RunJsScript(pRuntime, pTimer->GetJScript());
-        pTimer->KillJSTimer();
-      }
-      break;
-  }
+void app::CancelProc(CJS_Timer* pTimer) {
+  auto iter = std::find_if(m_Timers.begin(), m_Timers.end(),
+                           [pTimer](const std::unique_ptr<CJS_Timer>& that) {
+                             return pTimer == that.get();
+                           });
+
+  if (iter != m_Timers.end())
+    m_Timers.erase(iter);
 }
 
 void app::RunJsScript(CJS_Runtime* pRuntime, const CFX_WideString& wsScript) {
diff --git a/fpdfsdk/javascript/app.h b/fpdfsdk/javascript/app.h
index c6cda555d9..bb083cedb4 100644
--- a/fpdfsdk/javascript/app.h
+++ b/fpdfsdk/javascript/app.h
@@ -20,12 +20,11 @@ class TimerObj : public CJS_EmbedObj {
   TimerObj(CJS_Object* pJSObject);
   ~TimerObj() override;
 
- public:
   void SetTimer(CJS_Timer* pTimer);
-  CJS_Timer* GetTimer() const;
+  int GetTimerID() const { return m_nTimerID; }
 
  private:
-  CJS_Timer* m_pTimer;
+  int m_nTimerID;  // Weak reference to timer through global map.
 };
 
 class CJS_TimerObj : public CJS_Object {
@@ -158,6 +157,7 @@ class app : public CJS_EmbedObj {
  private:
   // CJS_EmbedObj
   void TimerProc(CJS_Timer* pTimer) override;
+  void CancelProc(CJS_Timer* pTimer) override;
   void RunJsScript(CJS_Runtime* pRuntime, const CFX_WideString& wsScript);
 
   void ClearTimerCommon(const CJS_Value& param);