Repository: electron/electron
File: shell/browser/api/electron_api_web_contents.cc (C++)

Code before:

void WebContents::DevToolsFocused() {
  Emit("devtools-focused");
}

void WebContents::DevToolsOpened() {
  v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
  v8::HandleScope handle_scope(isolate);
  DCHECK(inspectable_web_contents_);
  DCHECK(inspectable_web_contents_->GetDevToolsWebContents());
  auto handle = FromOrCreate(
      isolate, inspectable_web_contents_->GetDevToolsWebContents());
  devtools_web_contents_.Reset(isolate, handle.ToV8());

  // Set inspected tabID.
  inspectable_web_contents_->CallClientFunction(
      "DevToolsAPI", "setInspectedTabId", base::Value(ID()));

  // Inherit owner window in devtools when it doesn't have one.
  auto* devtools = inspectable_web_contents_->GetDevToolsWebContents();
  bool has_window = devtools->GetUserData(NativeWindowRelay::UserDataKey());
  if (owner_window_ && !has_window) {
    auto* owner_window = owner_window_.get();
    if (owner_window)
      handle->SetOwnerWindow(devtools, owner_window);
  }

  Emit("devtools-opened");
}

void WebContents::DevToolsClosed() {
  v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
  v8::HandleScope handle_scope(isolate);
  devtools_web_contents_.Reset();

  Emit("devtools-closed");
}

void WebContents::DevToolsResized() {
  observers_.Notify(&ExtendedWebContentsObserver::OnDevToolsResized);
}

void WebContents::SetOwnerWindow(NativeWindow* owner_window) {
  SetOwnerWindow(GetWebContents(), owner_window);
}

void WebContents::SetOwnerBaseWindow(std::optional<BaseWindow*> owner_window) {
  SetOwnerWindow(GetWebContents(),
                 owner_window ? (*owner_window)->window() : nullptr);
}


Review comment (bug):
There is no sequence change between these two calls. Based on the callstack, the crash is from the bool operation triggered by https://github.com/electron/electron/blob/b39305af61b573ee397ea1308ae46270637ba392/shell/browser/api/electron_api_web_contents.cc#L2184. If an incorrect sequence is involved for the current stack it would have to be on the interface endpoint that triggered `electron::InspectableWebContents::HandleMessageFromDevToolsFrontend` via `IPC::ChannelAssociatedGroupController::AcceptOnEndpointThread` 

The client for `blink::mojom::DevToolsFrontendHost` is held by https://source.chromium.org/chromium/chromium/src/+/main:content/browser/devtools/devtools_frontend_host_impl.h;l=53. The client endpoint is created in https://source.chromium.org/chromium/chromium/src/+/main:content/browser/devtools/devtools_frontend_host_impl.cc;l=117-118; which is definitely on the UI thread. When `task_runner` parameter is not passed when creating `AssociatedReceiver` the task runner of the current sequence will be chosen via https://source.chromium.org/chromium/chromium/src/+/main:mojo/public/cpp/bindings/lib/task_runner_helper.cc;l=12-18

I am not confident that the sequence that created `NativeWindow` and the sequence in which `WebContents::DevToolsOpened` got called are different. Can we start by adding some DCHECK ?

1) Adding `SEQUENCE_CHECKER` https://source.chromium.org/chromium/chromium/src/+/main:base/sequence_checker.h for `InsepctableWebContents` and `DCHECK_CALLED_ON_VALID_SEQUENCE` to `InspectableWebContents::LoadCompleted`
2) Adding `DCHECK(!owner_window_->WasInvalidated())` to ensure window of the webContents holding the devtools is valid
3) Adding `DCHECK_EQ(handle->owner_window(), nullptr)` to ensure the devtools does not have a owner window

```c++
if (owner_window_ && !has_window) {
    DCHECK(!owner_window_->WasInvalidated());
    DHCECK_EQ(handle->owner_window(), nullptr);
    handle->SetOwnerWindow(devtools, owner_window());
}
```

Code after:

void WebContents::DevToolsFocused() {
  Emit("devtools-focused");
}

void WebContents::DevToolsOpened() {
  v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
  v8::HandleScope handle_scope(isolate);
  DCHECK(inspectable_web_contents_);
  DCHECK(inspectable_web_contents_->GetDevToolsWebContents());
  auto handle = FromOrCreate(
      isolate, inspectable_web_contents_->GetDevToolsWebContents());
  devtools_web_contents_.Reset(isolate, handle.ToV8());

  // Set inspected tabID.
  inspectable_web_contents_->CallClientFunction(
      "DevToolsAPI", "setInspectedTabId", base::Value(ID()));

  // Inherit owner window in devtools when it doesn't have one.
  auto* devtools = inspectable_web_contents_->GetDevToolsWebContents();
  bool has_window = devtools->GetUserData(NativeWindowRelay::UserDataKey());
  if (owner_window_ && !has_window) {
    DCHECK(!owner_window_.WasInvalidated());
    DCHECK_EQ(handle->owner_window(), nullptr);
    handle->SetOwnerWindow(devtools, owner_window());
  }

  Emit("devtools-opened");
}

void WebContents::DevToolsClosed() {
  v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
  v8::HandleScope handle_scope(isolate);
  devtools_web_contents_.Reset();

  Emit("devtools-closed");
}

void WebContents::DevToolsResized() {
  observers_.Notify(&ExtendedWebContentsObserver::OnDevToolsResized);
}

void WebContents::SetOwnerWindow(NativeWindow* owner_window) {
  SetOwnerWindow(GetWebContents(), owner_window);
}

void WebContents::SetOwnerBaseWindow(std::optional<BaseWindow*> owner_window) {
  SetOwnerWindow(GetWebContents(),
                 owner_window ? (*owner_window)->window() : nullptr);
}