Repository: electron/electron (language: cpp)

Issue:
API: Documented way to close a WebContents
There should be a documented way to close a WebContents and allow it to be garbage collected.

I'd prefer not to document the existing `webContents.destroy()` method if possible as it's quite aggressive and makes the underlying WebContents object unusable after being called. Ideally, I'd like to have a `webContents.close()` method that returns the object to the state that it was in prior to `loadURL()` being called, and marks it as available for GC once references are dropped.

i.e. this should work:

```js
const bv = new BrowserView
bv.webContents.loadURL("https://example.com")
// ... later ...
bv.webContents.close()
// If the reference to `bv` were dropped here, both `bv` and the webContents would be available for collection

// ... but if the reference is retained, the WebContents should be re-usable at a later date:
bv.webContents.loadURL("https://example.com")
```

Ref https://github.com/electron/electron/pull/23578.


Discussion / hints:
Heya @nornagon @ckerr, Happy new year 🎉 

I noticed that this issue has been labeled as an enchancement, is there any way we can label it with something that convey a bit more urgency? e.g. a bug/regression perhaps.

Reasoning as partially quoted from the https://github.com/electron/electron/pull/23578#issuecomment-742706524 by @yohan1234 in PR discussion which removes `BrowserView::destroy`.

>[...] The problem is:
>
> - BrowserView.destroy was removed
> - webContents does not have a public API destroy function
>
> [...]

So a previously working browser view (_in electron v10 and below_) now cause a memory leak (_in electron v11 and above_) which makes it unusable. 1 work around proposed in the discussion was to  **use an undocumented `webContents.destroy()`**  which is not ideal so would love to avoid it if possible.

[related issue](https://github.com/electron/electron/issues/27071)
👋 @pushkin- thanks for linking, OOC, are you currently on electron v11 in your app? 
if so, have you been using `webContents.destroy`? 
I am interested in whether the method is safe enough to use without causing any crashes 🤔 
@kevinlinv Yes I'm on Electron 11. I encountered a crash which was because I wasn't detaching the BrowserView from the BrowserWindow before destroying its WebContents, but now that I'm doing that, I haven't seen any crashes.
Nice, thanks for the info, I might consider using `webContents.destroy` and update to `electron v11` given the need to support Apple ARM 👌 
A friendly 🏓 sorry for the noise. Would appreciate if this issue is looked at by someone who knows the codebase 👍 
It's rather unfortunate for browser view to leak in latest version of electron by default.
+1 for this issue. The work-around (calling destroy() on webContents) addressed the leak for me, but I did experience a hard crash that required wrapping the destroy call in `setImmediate` to avoid.
> a previously working browser view (_in electron v10 and below_) now cause a memory leak (_in electron v11 and above_) which makes it unusable.

Just so I'm clear on what the issue is here, the app were previously calling `browserview.destroy()`, then that function was removed—so the app was changed to not call `destroy()`, and now leaks?

I think this should be straightforwardly fixable by calling `browserview.webContents.destroy()` where there was previously a call to `browserview.destroy()`. I'm aware that the `WebContents.destroy()` method is not documented, but in this case I think it's OK to use. Once there exists a documented way to shut down a WebContents, the `destroy()` call can be replaced with that.

@stewartlord would you be able to file a separate bug with the crash info?
Hey @nornagon thanks for the reply.

> Just so I'm clear on what the issue is here, the app were previously calling browserview.destroy(), then that function was removed—so the app was changed to not call destroy(), and now leaks?

Yes, it's exactly as you've pointed out.

> I think this should be straightforwardly fixable by calling browserview.webContents.destroy() where there was previously a call to browserview.destroy(). I'm aware that the WebContents.destroy() method is not documented, but in this case I think it's OK to use. Once there exists a documented way to shut down a WebContents, the destroy() call can be replaced with that.

True, from my own use case, I'd prefer to not use undocumented api, but not too fussed if I really have to and will do so to allow upgrading to `electron@11`

---

It is more into the point of view of new `BrowserView` users in latest electron that would make this issue slightly unfortunate, as they would immediately run into a memory leak and may not be aware of it before they stumble into this issue somehow.

> +1 for this issue. The work-around (calling destroy() on webContents) addressed the leak for me, but I did experience a hard crash that required wrapping the destroy call in setImmediate to avoid.

@stewartlord I've just placed [the respective issue](https://github.com/electron/electron/issues/29626) with attached core dump.
@vladimiry Thank you 🙏 ..and @nornagon looks like I failed to deliver repro steps - very sorry about that!
> @stewartlord would you be able to file a separate bug with the crash info?

Unfortunately placing a separate issue appears to be a time-wasting procedure since it just got closed with `won't fix` status.

Fix patch:
diff --git a/docs/api/web-contents.md b/docs/api/web-contents.md
index 0de4fd03545c5..95d5278672901 100644
--- a/docs/api/web-contents.md
+++ b/docs/api/web-contents.md
@@ -934,6 +934,21 @@ Returns `string` - The title of the current web page.
 
 Returns `boolean` - Whether the web page is destroyed.
 
+#### `contents.close([opts])`
+
+* `opts` Object (optional)
+  * `waitForBeforeUnload` boolean - if true, fire the `beforeunload` event
+    before closing the page. If the page prevents the unload, the WebContents
+    will not be closed. The [`will-prevent-unload`](#event-will-prevent-unload)
+    will be fired if the page requests prevention of unload.
+
+Closes the page, as if the web content had called `window.close()`.
+
+If the page is successfully closed (i.e. the unload is not prevented by the
+page, or `waitForBeforeUnload` is false or unspecified), the WebContents will
+be destroyed and no longer usable. The [`destroyed`](#event-destroyed) event
+will be emitted.
+
 #### `contents.focus()`
 
 Focuses the web page.
diff --git a/shell/browser/api/electron_api_browser_window.cc b/shell/browser/api/electron_api_browser_window.cc
index 5b5adb540aa6d..422512af1e957 100644
--- a/shell/browser/api/electron_api_browser_window.cc
+++ b/shell/browser/api/electron_api_browser_window.cc
@@ -134,6 +134,7 @@ BrowserWindow::~BrowserWindow() {
     api_web_contents_->RemoveObserver(this);
     // Destroy the WebContents.
     OnCloseContents();
+    api_web_contents_->Destroy();
   }
 }
 
@@ -181,7 +182,6 @@ void BrowserWindow::WebContentsDestroyed() {
 
 void BrowserWindow::OnCloseContents() {
   BaseWindow::ResetBrowserViews();
-  api_web_contents_->Destroy();
 }
 
 void BrowserWindow::OnRendererResponsive(content::RenderProcessHost*) {
diff --git a/shell/browser/api/electron_api_web_contents.cc b/shell/browser/api/electron_api_web_contents.cc
index fe5380654170c..41d0408ff1f0d 100755
--- a/shell/browser/api/electron_api_web_contents.cc
+++ b/shell/browser/api/electron_api_web_contents.cc
@@ -1006,6 +1006,19 @@ void WebContents::Destroy() {
   }
 }
 
+void WebContents::Close(absl::optional<gin_helper::Dictionary> options) {
+  bool dispatch_beforeunload = false;
+  if (options)
+    options->Get("waitForBeforeUnload", &dispatch_beforeunload);
+  if (dispatch_beforeunload &&
+      web_contents()->NeedToFireBeforeUnloadOrUnloadEvents()) {
+    NotifyUserActivation();
+    web_contents()->DispatchBeforeUnload(false /* auto_cancel */);
+  } else {
+    web_contents()->Close();
+  }
+}
+
 bool WebContents::DidAddMessageToConsole(
     content::WebContents* source,
     blink::mojom::ConsoleMessageLevel level,
@@ -1196,6 +1209,8 @@ void WebContents::CloseContents(content::WebContents* source) {
 
   for (ExtendedWebContentsObserver& observer : observers_)
     observer.OnCloseContents();
+
+  Destroy();
 }
 
 void WebContents::ActivateContents(content::WebContents* source) {
@@ -3918,6 +3933,7 @@ v8::Local<v8::ObjectTemplate> WebContents::FillObjectTemplate(
   // destroyable.
   return gin_helper::ObjectTemplateBuilder(isolate, templ)
       .SetMethod("destroy", &WebContents::Destroy)
+      .SetMethod("close", &WebContents::Close)
       .SetMethod("getBackgroundThrottling",
                  &WebContents::GetBackgroundThrottling)
       .SetMethod("setBackgroundThrottling",
diff --git a/shell/browser/api/electron_api_web_contents.h b/shell/browser/api/electron_api_web_contents.h
index 9cec3ff614f85..d5c59ab6fa453 100644
--- a/shell/browser/api/electron_api_web_contents.h
+++ b/shell/browser/api/electron_api_web_contents.h
@@ -152,6 +152,7 @@ class WebContents : public ExclusiveAccessContext,
   const char* GetTypeName() override;
 
   void Destroy();
+  void Close(absl::optional<gin_helper::Dictionary> options);
   base::WeakPtr<WebContents> GetWeakPtr() { return weak_factory_.GetWeakPtr(); }
 
   bool GetBackgroundThrottling() const;
diff --git a/spec/api-web-contents-spec.ts b/spec/api-web-contents-spec.ts
index 98251ac79035f..4be69cfb74ea4 100644
--- a/spec/api-web-contents-spec.ts
+++ b/spec/api-web-contents-spec.ts
@@ -2133,6 +2133,83 @@ describe('webContents module', () => {
     });
   });
 
+  describe('close() method', () => {
+    afterEach(closeAllWindows);
+
+    it('closes when close() is called', async () => {
+      const w = (webContents as any).create() as WebContents;
+      const destroyed = emittedOnce(w, 'destroyed');
+      w.close();
+      await destroyed;
+      expect(w.isDestroyed()).to.be.true();
+    });
+
+    it('closes when close() is called after loading a page', async () => {
+      const w = (webContents as any).create() as WebContents;
+      await w.loadURL('about:blank');
+      const destroyed = emittedOnce(w, 'destroyed');
+      w.close();
+      await destroyed;
+      expect(w.isDestroyed()).to.be.true();
+    });
+
+    it('can be GCed before loading a page', async () => {
+      const v8Util = process._linkedBinding('electron_common_v8_util');
+      let registry: FinalizationRegistry<unknown> | null = null;
+      const cleanedUp = new Promise<number>(resolve => {
+        registry = new FinalizationRegistry(resolve as any);
+      });
+      (() => {
+        const w = (webContents as any).create() as WebContents;
+        registry!.register(w, 42);
+      })();
+      const i = setInterval(() => v8Util.requestGarbageCollectionForTesting(), 100);
+      defer(() => clearInterval(i));
+      expect(await cleanedUp).to.equal(42);
+    });
+
+    it('causes its parent browserwindow to be closed', async () => {
+      const w = new BrowserWindow({ show: false });
+      await w.loadURL('about:blank');
+      const closed = emittedOnce(w, 'closed');
+      w.webContents.close();
+      await closed;
+      expect(w.isDestroyed()).to.be.true();
+    });
+
+    it('ignores beforeunload if waitForBeforeUnload not specified', async () => {
+      const w = (webContents as any).create() as WebContents;
+      await w.loadURL('about:blank');
+      await w.executeJavaScript('window.onbeforeunload = () => "hello"; null');
+      w.on('will-prevent-unload', () => { throw new Error('unexpected will-prevent-unload'); });
+      const destroyed = emittedOnce(w, 'destroyed');
+      w.close();
+      await destroyed;
+      expect(w.isDestroyed()).to.be.true();
+    });
+
+    it('runs beforeunload if waitForBeforeUnload is specified', async () => {
+      const w = (webContents as any).create() as WebContents;
+      await w.loadURL('about:blank');
+      await w.executeJavaScript('window.onbeforeunload = () => "hello"; null');
+      const willPreventUnload = emittedOnce(w, 'will-prevent-unload');
+      w.close({ waitForBeforeUnload: true });
+      await willPreventUnload;
+      expect(w.isDestroyed()).to.be.false();
+    });
+
+    it('overriding beforeunload prevention results in webcontents close', async () => {
+      const w = (webContents as any).create() as WebContents;
+      await w.loadURL('about:blank');
+      await w.executeJavaScript('window.onbeforeunload = () => "hello"; null');
+      w.once('will-prevent-unload', e => e.preventDefault());
+      const destroyed = emittedOnce(w, 'destroyed');
+      w.close({ waitForBeforeUnload: true });
+      await destroyed;
+      expect(w.isDestroyed()).to.be.true();
+    });
+  });
+
   describe('content-bounds-updated event', () => {
     afterEach(closeAllWindows);
     it('emits when moveTo is called', async () => {


Test patch: