refactor: ginify DownloadItem (#22924)

lib/browser/api/app.ts

@@ -138,7 +138,3 @@ for (const name of events) {
 // Deprecate allowRendererProcessReuse but only if they set it to false, no need to log if
 // they are setting it to true
 deprecate.removeProperty(app, 'allowRendererProcessReuse', [false]);
-
-// Wrappers for native classes.
-const { DownloadItem } = process.electronBinding('download_item');
-Object.setPrototypeOf(DownloadItem.prototype, EventEmitter.prototype);

shell/browser/api/electron_api_download_item.cc

@@ -5,6 +5,7 @@
 #include "shell/browser/api/electron_api_download_item.h"
 
 #include <map>
+#include <memory>
 
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -53,16 +54,41 @@ namespace api {
 
 namespace {
 
-std::map<uint32_t, v8::Global<v8::Value>> g_download_item_objects;
+// Ordinarily base::SupportsUserData only supports strong links, where the
+// thing to which the user data is attached owns the user data. But we can't
+// make the api::DownloadItem owned by the DownloadItem, since it's owned by
+// V8. So this makes a weak link. The lifetimes of download::DownloadItem and
+// api::DownloadItem are fully independent, and either one may be destroyed
+// before the other.
+struct UserDataLink : base::SupportsUserData::Data {
+  explicit UserDataLink(base::WeakPtr<DownloadItem> item)
+      : download_item(item) {}
+
+  base::WeakPtr<DownloadItem> download_item;
+};
+
+const void* kElectronApiDownloadItemKey = &kElectronApiDownloadItemKey;
 
 }  // namespace
 
+gin::WrapperInfo DownloadItem::kWrapperInfo = {gin::kEmbedderNativeGin};
+
+// static
+DownloadItem* DownloadItem::FromDownloadItem(
+    download::DownloadItem* download_item) {
+  // ^- say that 7 times fast in a row
+  UserDataLink* data = static_cast<UserDataLink*>(
+      download_item->GetUserData(kElectronApiDownloadItemKey));
+  return data ? data->download_item.get() : nullptr;
+}
+
 DownloadItem::DownloadItem(v8::Isolate* isolate,
                            download::DownloadItem* download_item)
     : download_item_(download_item) {
   download_item_->AddObserver(this);
-  Init(isolate);
-  AttachAsUserData(download_item);
+  download_item_->SetUserData(
+      kElectronApiDownloadItemKey,
+      std::make_unique<UserDataLink>(weak_factory_.GetWeakPtr()));
 }
 
 DownloadItem::~DownloadItem() {
@@ -71,17 +97,23 @@ DownloadItem::~DownloadItem() {
     download_item_->RemoveObserver(this);
     download_item_->Remove();
   }
+}
 
-  // Remove from the global map.
-  g_download_item_objects.erase(weak_map_id());
+bool DownloadItem::CheckAlive() const {
+  if (!download_item_) {
+    gin_helper::ErrorThrower(v8::Isolate::GetCurrent())
+        .ThrowError("DownloadItem used after being destroyed");
+    return false;
+  }
+  return true;
 }
 
 void DownloadItem::OnDownloadUpdated(download::DownloadItem* item) {
+  if (!CheckAlive())
+    return;
   if (download_item_->IsDone()) {
     Emit("done", item->GetState());
-    // Destroy the item once item is downloaded.
-    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
-                                                  GetDestroyClosure());
+    Unpin();
   } else {
     Emit("updated", item->GetState());
   }
@@ -89,47 +121,66 @@ void DownloadItem::OnDownloadUpdated(download::DownloadItem* item) {
 
 void DownloadItem::OnDownloadDestroyed(download::DownloadItem* download_item) {
   download_item_ = nullptr;
-  // Destroy the native class immediately when downloadItem is destroyed.
-  delete this;
+  Unpin();
 }
 
 void DownloadItem::Pause() {
+  if (!CheckAlive())
+    return;
   download_item_->Pause();
 }
 
 bool DownloadItem::IsPaused() const {
+  if (!CheckAlive())
+    return false;
   return download_item_->IsPaused();
 }
 
 void DownloadItem::Resume() {
+  if (!CheckAlive())
+    return;
   download_item_->Resume(true /* user_gesture */);
 }
 
 bool DownloadItem::CanResume() const {
+  if (!CheckAlive())
+    return false;
   return download_item_->CanResume();
 }
 
 void DownloadItem::Cancel() {
+  if (!CheckAlive())
+    return;
   download_item_->Cancel(true);
 }
 
 int64_t DownloadItem::GetReceivedBytes() const {
+  if (!CheckAlive())
+    return 0;
   return download_item_->GetReceivedBytes();
 }
 
 int64_t DownloadItem::GetTotalBytes() const {
+  if (!CheckAlive())
+    return 0;
   return download_item_->GetTotalBytes();
 }
 
 std::string DownloadItem::GetMimeType() const {
+  if (!CheckAlive())
+    return "";
   return download_item_->GetMimeType();
 }
 
 bool DownloadItem::HasUserGesture() const {
+  if (!CheckAlive())
+    return false;
   return download_item_->HasUserGesture();
 }
 
 std::string DownloadItem::GetFilename() const {
+  if (!CheckAlive())
+    return "";
   return base::UTF16ToUTF8(
       net::GenerateFileName(GetURL(), GetContentDisposition(), std::string(),
                             download_item_->GetSuggestedFilename(),
@@ -138,22 +189,32 @@ std::string DownloadItem::GetFilename() const {
 }
 
 std::string DownloadItem::GetContentDisposition() const {
+  if (!CheckAlive())
+    return "";
   return download_item_->GetContentDisposition();
 }
 
 const GURL& DownloadItem::GetURL() const {
+  if (!CheckAlive())
+    return GURL::EmptyGURL();
   return download_item_->GetURL();
 }
 
-const std::vector<GURL>& DownloadItem::GetURLChain() const {
-  return download_item_->GetUrlChain();
+v8::Local<v8::Value> DownloadItem::GetURLChain(v8::Isolate* isolate) const {
+  if (!CheckAlive())
+    return v8::Local<v8::Value>();
+  return gin::ConvertToV8(isolate, download_item_->GetUrlChain());
 }
 
 download::DownloadItem::DownloadState DownloadItem::GetState() const {
+  if (!CheckAlive())
+    return download::DownloadItem::IN_PROGRESS;
   return download_item_->GetState();
 }
 
 bool DownloadItem::IsDone() const {
+  if (!CheckAlive())
+    return false;
   return download_item_->IsDone();
 }
 
@@ -175,23 +236,28 @@ void DownloadItem::SetSaveDialogOptions(
 }
 
 std::string DownloadItem::GetLastModifiedTime() const {
+  if (!CheckAlive())
+    return "";
   return download_item_->GetLastModifiedTime();
 }
 
 std::string DownloadItem::GetETag() const {
+  if (!CheckAlive())
+    return "";
   return download_item_->GetETag();
 }
 
 double DownloadItem::GetStartTime() const {
+  if (!CheckAlive())
+    return 0;
   return download_item_->GetStartTime().ToDoubleT();
 }
 
 // static
-void DownloadItem::BuildPrototype(v8::Isolate* isolate,
-                                  v8::Local<v8::FunctionTemplate> prototype) {
-  prototype->SetClassName(gin::StringToV8(isolate, "DownloadItem"));
-  gin_helper::Destroyable::MakeDestroyable(isolate, prototype);
-  gin_helper::ObjectTemplateBuilder(isolate, prototype->PrototypeTemplate())
+gin::ObjectTemplateBuilder DownloadItem::GetObjectTemplateBuilder(
+    v8::Isolate* isolate) {
+  return gin_helper::EventEmitterMixin<DownloadItem>::GetObjectTemplateBuilder(
+             isolate)
       .SetMethod("pause", &DownloadItem::Pause)
       .SetMethod("isPaused", &DownloadItem::IsPaused)
       .SetMethod("resume", &DownloadItem::Resume)
@@ -218,38 +284,25 @@ void DownloadItem::BuildPrototype(v8::Isolate* isolate,
       .SetMethod("getStartTime", &DownloadItem::GetStartTime);
 }
 
+const char* DownloadItem::GetTypeName() {
+  return "DownloadItem";
+}
+
 // static
-gin::Handle<DownloadItem> DownloadItem::Create(v8::Isolate* isolate,
-                                               download::DownloadItem* item) {
-  auto* existing = TrackableObject::FromWrappedClass(isolate, item);
+gin::Handle<DownloadItem> DownloadItem::FromOrCreate(
+    v8::Isolate* isolate,
+    download::DownloadItem* item) {
+  DownloadItem* existing = FromDownloadItem(item);
   if (existing)
-    return gin::CreateHandle(isolate, static_cast<DownloadItem*>(existing));
+    return gin::CreateHandle(isolate, existing);
 
   auto handle = gin::CreateHandle(isolate, new DownloadItem(isolate, item));
 
-  // Reference this object in case it got garbage collected.
-  g_download_item_objects[handle->weak_map_id()] =
-      v8::Global<v8::Value>(isolate, handle.ToV8());
+  handle->Pin(isolate);
+
   return handle;
 }
 
 }  // namespace api
 
 }  // namespace electron
-
-namespace {
-
-void Initialize(v8::Local<v8::Object> exports,
-                v8::Local<v8::Value> unused,
-                v8::Local<v8::Context> context,
-                void* priv) {
-  v8::Isolate* isolate = context->GetIsolate();
-  gin_helper::Dictionary(isolate, exports)
-      .Set("DownloadItem", electron::api::DownloadItem::GetConstructor(isolate)
-                               ->GetFunction(context)
-                               .ToLocalChecked());
-}
-
-}  // namespace
-
-NODE_LINKED_MODULE_CONTEXT_AWARE(electron_browser_download_item, Initialize)

shell/browser/api/electron_api_download_item.h

@@ -9,25 +9,51 @@
 #include <vector>
 
 #include "base/files/file_path.h"
+#include "base/memory/weak_ptr.h"
 #include "components/download/public/common/download_item.h"
 #include "gin/handle.h"
+#include "gin/wrappable.h"
+#include "shell/browser/event_emitter_mixin.h"
 #include "shell/browser/ui/file_dialog.h"
-#include "shell/common/gin_helper/trackable_object.h"
+#include "shell/common/gin_helper/pinnable.h"
 #include "url/gurl.h"
 
 namespace electron {
 
 namespace api {
 
-class DownloadItem : public gin_helper::TrackableObject<DownloadItem>,
+class DownloadItem : public gin::Wrappable<DownloadItem>,
+                     public gin_helper::Pinnable<DownloadItem>,
+                     public gin_helper::EventEmitterMixin<DownloadItem>,
                      public download::DownloadItem::Observer {
  public:
-  static gin::Handle<DownloadItem> Create(v8::Isolate* isolate,
-                                          download::DownloadItem* item);
+  static gin::Handle<DownloadItem> FromOrCreate(v8::Isolate* isolate,
+                                                download::DownloadItem* item);
 
-  static void BuildPrototype(v8::Isolate* isolate,
-                             v8::Local<v8::FunctionTemplate> prototype);
+  static DownloadItem* FromDownloadItem(download::DownloadItem*);
 
+  // gin::Wrappable
+  static gin::WrapperInfo kWrapperInfo;
+  gin::ObjectTemplateBuilder GetObjectTemplateBuilder(
+      v8::Isolate* isolate) override;
+  const char* GetTypeName() override;
+
+  // JS API, but also C++ calls it sometimes
+  void SetSavePath(const base::FilePath& path);
+  base::FilePath GetSavePath() const;
+  file_dialog::DialogSettings GetSaveDialogOptions() const;
+
+ private:
+  DownloadItem(v8::Isolate* isolate, download::DownloadItem* download_item);
+  ~DownloadItem() override;
+
+  bool CheckAlive() const;
+
+  // download::DownloadItem::Observer
+  void OnDownloadUpdated(download::DownloadItem* download) override;
+  void OnDownloadDestroyed(download::DownloadItem* download) override;
+
+  // JS API
   void Pause();
   bool IsPaused() const;
   void Resume();
@@ -40,30 +66,20 @@ class DownloadItem : public gin_helper::TrackableObject<DownloadItem>,
   std::string GetFilename() const;
   std::string GetContentDisposition() const;
   const GURL& GetURL() const;
-  const std::vector<GURL>& GetURLChain() const;
+  v8::Local<v8::Value> GetURLChain(v8::Isolate*) const;
   download::DownloadItem::DownloadState GetState() const;
   bool IsDone() const;
-  void SetSavePath(const base::FilePath& path);
-  base::FilePath GetSavePath() const;
-  file_dialog::DialogSettings GetSaveDialogOptions() const;
   void SetSaveDialogOptions(const file_dialog::DialogSettings& options);
   std::string GetLastModifiedTime() const;
   std::string GetETag() const;
   double GetStartTime() const;
 
- protected:
-  DownloadItem(v8::Isolate* isolate, download::DownloadItem* download_item);
-  ~DownloadItem() override;
-
-  // Override download::DownloadItem::Observer methods
-  void OnDownloadUpdated(download::DownloadItem* download) override;
-  void OnDownloadDestroyed(download::DownloadItem* download) override;
-
- private:
   base::FilePath save_path_;
   file_dialog::DialogSettings dialog_options_;
   download::DownloadItem* download_item_;
 
+  base::WeakPtrFactory<DownloadItem> weak_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(DownloadItem);
 };
 

shell/browser/api/electron_api_session.cc

@@ -294,7 +294,7 @@ void Session::OnDownloadCreated(content::DownloadManager* manager,
 
   v8::Locker locker(isolate());
   v8::HandleScope handle_scope(isolate());
-  auto handle = DownloadItem::Create(isolate(), item);
+  auto handle = DownloadItem::FromOrCreate(isolate(), item);
   if (item->GetState() == download::DownloadItem::INTERRUPTED)
     handle->SetSavePath(item->GetTargetFilePath());
   content::WebContents* web_contents =
@@ -641,7 +641,7 @@ void Session::CreateInterruptedDownload(const gin_helper::Dictionary& options) {
   }
   auto* download_manager =
       content::BrowserContext::GetDownloadManager(browser_context());
-  download_manager->GetDelegate()->GetNextId(base::BindRepeating(
+  download_manager->GetNextId(base::BindRepeating(
       &DownloadIdCallback, download_manager, path, url_chain, mime_type, offset,
       length, last_modified, etag, base::Time::FromDoubleT(start_time)));
 }

shell/browser/api/electron_api_tray.cc

@@ -170,7 +170,7 @@ bool Tray::IsDestroyed() {
 }
 
 void Tray::SetImage(gin::Handle<NativeImage> image) {
-  if (!CheckDestroyed())
+  if (!CheckAlive())
     return;
 #if defined(OS_WIN)
   tray_icon_->SetImage(image->GetHICON(GetSystemMetrics(SM_CXSMICON)));
@@ -180,7 +180,7 @@ void Tray::SetImage(gin::Handle<NativeImage> image) {
 }
 
 void Tray::SetPressedImage(gin::Handle<NativeImage> image) {
-  if (!CheckDestroyed())
+  if (!CheckAlive())
     return;
 #if defined(OS_WIN)
   tray_icon_->SetPressedImage(image->GetHICON(GetSystemMetrics(SM_CXSMICON)));
@@ -190,13 +190,13 @@ void Tray::SetPressedImage(gin::Handle<NativeImage> image) {
 }
 
 void Tray::SetToolTip(const std::string& tool_tip) {
-  if (!CheckDestroyed())
+  if (!CheckAlive())
     return;
   tray_icon_->SetToolTip(tool_tip);
 }
 
 void Tray::SetTitle(const std::string& title) {
-  if (!CheckDestroyed())
+  if (!CheckAlive())
     return;
 #if defined(OS_MACOSX)
   tray_icon_->SetTitle(title);
@@ -204,7 +204,7 @@ void Tray::SetTitle(const std::string& title) {
 }
 
 std::string Tray::GetTitle() {
-  if (!CheckDestroyed())
+  if (!CheckAlive())
     return std::string();
 #if defined(OS_MACOSX)
   return tray_icon_->GetTitle();
@@ -214,7 +214,7 @@ std::string Tray::GetTitle() {
 }
 
 void Tray::SetIgnoreDoubleClickEvents(bool ignore) {
-  if (!CheckDestroyed())
+  if (!CheckAlive())
     return;
 #if defined(OS_MACOSX)
   tray_icon_->SetIgnoreDoubleClickEvents(ignore);
@@ -222,7 +222,7 @@ void Tray::SetIgnoreDoubleClickEvents(bool ignore) {
 }
 
 bool Tray::GetIgnoreDoubleClickEvents() {
-  if (!CheckDestroyed())
+  if (!CheckAlive())
     return false;
 #if defined(OS_MACOSX)
   return tray_icon_->GetIgnoreDoubleClickEvents();
@@ -233,7 +233,7 @@ bool Tray::GetIgnoreDoubleClickEvents() {
 
 void Tray::DisplayBalloon(gin_helper::ErrorThrower thrower,
                           const gin_helper::Dictionary& options) {
-  if (!CheckDestroyed())
+  if (!CheckAlive())
     return;
   TrayIcon::BalloonOptions balloon_options;
 
@@ -263,19 +263,19 @@ void Tray::DisplayBalloon(gin_helper::ErrorThrower thrower,
 }
 
 void Tray::RemoveBalloon() {
-  if (!CheckDestroyed())
+  if (!CheckAlive())
     return;
   tray_icon_->RemoveBalloon();
 }
 
 void Tray::Focus() {
-  if (!CheckDestroyed())
+  if (!CheckAlive())
     return;
   tray_icon_->Focus();
 }
 
 void Tray::PopUpContextMenu(gin::Arguments* args) {
-  if (!CheckDestroyed())
+  if (!CheckAlive())
     return;
   gin::Handle<Menu> menu;
   gfx::Point pos;
@@ -298,14 +298,14 @@ void Tray::PopUpContextMenu(gin::Arguments* args) {
 }
 
 void Tray::CloseContextMenu() {
-  if (!CheckDestroyed())
+  if (!CheckAlive())
     return;
   tray_icon_->CloseContextMenu();
 }
 
 void Tray::SetContextMenu(gin_helper::ErrorThrower thrower,
                           v8::Local<v8::Value> arg) {
-  if (!CheckDestroyed())
+  if (!CheckAlive())
     return;
   gin::Handle<Menu> menu;
   if (arg->IsNull()) {
@@ -320,12 +320,12 @@ void Tray::SetContextMenu(gin_helper::ErrorThrower thrower,
 }
 
 gfx::Rect Tray::GetBounds() {
-  if (!CheckDestroyed())
+  if (!CheckAlive())
     return gfx::Rect();
   return tray_icon_->GetBounds();
 }
 
-bool Tray::CheckDestroyed() {
+bool Tray::CheckAlive() {
   if (!tray_icon_) {
     v8::Isolate* isolate = v8::Isolate::GetCurrent();
     v8::Locker locker(isolate);

shell/browser/api/electron_api_tray.h

@@ -99,7 +99,7 @@ class Tray : public gin::Wrappable<Tray>,
                       v8::Local<v8::Value> arg);
   gfx::Rect GetBounds();
 
-  bool CheckDestroyed();
+  bool CheckAlive();
 
   v8::Global<v8::Value> menu_;
   std::unique_ptr<TrayIcon> tray_icon_;

shell/browser/electron_download_manager_delegate.cc

@@ -65,11 +65,7 @@ ElectronDownloadManagerDelegate::~ElectronDownloadManagerDelegate() {
 void ElectronDownloadManagerDelegate::GetItemSavePath(
     download::DownloadItem* item,
     base::FilePath* path) {
-  v8::Isolate* isolate = v8::Isolate::GetCurrent();
-  v8::Locker locker(isolate);
-  v8::HandleScope handle_scope(isolate);
-  api::DownloadItem* download =
-      api::DownloadItem::FromWrappedClass(isolate, item);
+  api::DownloadItem* download = api::DownloadItem::FromDownloadItem(item);
   if (download)
     *path = download->GetSavePath();
 }
@@ -77,11 +73,7 @@ void ElectronDownloadManagerDelegate::GetItemSavePath(
 void ElectronDownloadManagerDelegate::GetItemSaveDialogOptions(
     download::DownloadItem* item,
     file_dialog::DialogSettings* options) {
-  v8::Isolate* isolate = v8::Isolate::GetCurrent();
-  v8::Locker locker(isolate);
-  v8::HandleScope handle_scope(isolate);
-  api::DownloadItem* download =
-      api::DownloadItem::FromWrappedClass(isolate, item);
+  api::DownloadItem* download = api::DownloadItem::FromDownloadItem(item);
   if (download)
     *options = download->GetSaveDialogOptions();
 }
@@ -165,13 +157,9 @@ void ElectronDownloadManagerDelegate::OnDownloadSaveDialogDone(
       browser_context->prefs()->SetFilePath(prefs::kDownloadDefaultDirectory,
                                             path.DirName());
 
-      v8::Isolate* isolate = v8::Isolate::GetCurrent();
-      v8::Locker locker(isolate);
-      v8::HandleScope handle_scope(isolate);
-      api::DownloadItem* download_item =
-          api::DownloadItem::FromWrappedClass(isolate, item);
-      if (download_item)
-        download_item->SetSavePath(path);
+      api::DownloadItem* download = api::DownloadItem::FromDownloadItem(item);
+      if (download)
+        download->SetSavePath(path);
     }
   }
 

shell/common/node_bindings.cc

@@ -38,7 +38,6 @@
   V(electron_browser_browser_view)       \
   V(electron_browser_content_tracing)    \
   V(electron_browser_dialog)             \
-  V(electron_browser_download_item)      \
   V(electron_browser_event)              \
   V(electron_browser_event_emitter)      \
   V(electron_browser_global_shortcut)    \

spec-main/api-session-spec.ts

@@ -311,7 +311,7 @@ describe('session module', () => {
       const { item, itemUrl, itemFilename } = await downloadPrevented;
       expect(itemUrl).to.equal(url);
       expect(itemFilename).to.equal('mockFile.txt');
-      expect(() => item.getURL()).to.throw('Object has been destroyed');
+      expect(() => item.getURL()).to.throw('DownloadItem used after being destroyed');
     });
   });