Crash when closing WebContents inside DidFinishNavigation

[Deepak Mohan]

Hi,

In Electron we expose various stages of navigation as events, there was a report recently about a crash when user tried to destroy a webContents when a navigation failed https://github.com/electron/electron/issues/8930 . I created a minimal example with content shell to reproduce the scenario

// Destroys WebContents when a navigation has committed.

class NavigationCommitObserver : public WebContentsObserver {

 public:

  explicit NavigationCommitObserver(WebContents* web_contents)

      : WebContentsObserver(web_contents) {}

 private:

  void DidFinishNavigation(NavigationHandle* navigation_handle) override {

    if (navigation_handle->HasCommitted() &&

        !navigation_handle->IsErrorPage())

      web_contents()->Close();

  }

};

IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, ShutdownOnNavigation) {

  GURL main_url(embedded_test_server()->GetURL("/simple_page.html"));

  Shell* new_shell = CreateBrowser();

  NavigationCommitObserver observer(new_shell->web_contents());

  EXPECT_TRUE(NavigateToURL(new_shell, main_url));

}

the crash stack trace https://gist.github.com/deepak1556/94a7bc0f551ab8919ee7bb4dac70cdea . Is this the expected behavior ?

Thanks,

Deepak

[Daniel Cheng]

Synchronously destroying the observed object inside an observer callback is somewhat of an anti-pattern: there's no guarantee on what order observer callbacks are invoked, observer callbacks may be re-entered, the code dispatching these observations doesn't expect to have to check if |this| is destroyed, etc.

I would suggest making BrowserWindow.close in Electron asynchronous to avoid this issue.

Daniel

--
You received this message because you are subscribed to the Google Groups "Chromium Embedders" group.
To unsubscribe from this group and stop receiving emails from it, send an email to embedder-dev...@chromium.org.
To post to this group, send email to embedd...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/embedder-dev/ea1767b6-009a-4013-b235-568938b20bfc%40chromium.org.

[Deepak Mohan]

Thanks for the explanation!

> I would suggest making BrowserWindow.close in Electron asynchronous to avoid this issue.

Yup that's the plan, wanted to make sure if its the right thing to do here.

- Deepak

[Camille Lamy]

As Daniel suggested, you should really make the closing asynchronous. All the code that's dispatching the navigation WebContentsObserver methods expects the WebContents to still be alive at the end of the methods.

--

You received this message because you are subscribed to the Google Groups "Chromium Embedders" group.
To unsubscribe from this group and stop receiving emails from it, send an email to embedder-dev...@chromium.org.
To post to this group, send email to embedd...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/embedder-dev/5388eea2-77fc-4e5f-9785-7235321514fe%40chromium.org.

[Deepak Mohan]

> All the code that's dispatching the navigation WebContentsObserver methods expects the WebContents to still be alive at the end of the methods.

Thanks for confirming! I didn't know about the anti-pattern Daniel mentioned, will try to avoid such scenarios in the future. Was under the impression that DidFinishNavigation fires at the end of a navigation life cycle, hence its safe to destroy stuff there.

- Deepak