fix: nullptr check when closing windows

[codebytere]

Description of Change

Closes #14161.

We weren't performing a nullptr check, so we would experience an EXC_BAD_ACCESS crash
if an object in the WindowVector is destroyed during cleanup. This fixes that by adding said nullptr check.

cc @zcbenz @deepak1556 @MarshallOfSound

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• PR title follows semantic commit guidelines
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fixed an occasional crash when closing all BrowserWindows.

[MarshallOfSound]

Can we explain how this helps? These aren't smart pointers, if this is an invalid access it's a UAF not a nullptr issue.

WindowVector === std::vector<NativeWindow*> so unless we're injecting / replacing nullptr in the vector this won't fix the issue afaik

[deepak1556]

changing to std::vector<base::WeakPtr<NativeWindow>> will help rather than maintaining a raw map, the ideal solution is for the map to be maintained by AddWindow , RemoveWindow , but I don't know when is case this happens. Open to solutions.

[MarshallOfSound]

If we make this a vector of weak pointers this solution makes sense, without it I don't think this fix actually helps.

[zcbenz]

Both DestroyAllWindows and CloseAllWindows work on a copied array of windows, and the source array is being updated when a window is created/destroyed, so its values can not be nullptr.

It seems the app destroyed other windows when a window is being closed, and the copied windows array still included freed pointers and crashed when calling window->IsClosed(). So checking null would not fix the crash as it is a wild pointer instead of null pointer.

// app is doing something like this:
childWindow.once('closed', () => {
  parentWindow.destroy()
})

An ideal solution would be forbidding users to delete NativeWindow and have WindowList manage the deletion, but that would be a fairly large refactoring. I think a small workaround would be something like:

void WindowList::DestroyAllWindows() {
  std::vector<base::WeakPtr<NativeWindow>> windows = ConvertToWeakPtrVector(GetInstance()->windows_);

  for (const auto& window : windows) {
    if (window)
      window->CloseImmediately();
  }
}

[deepak1556]

@zcbenz any reason not maintain the WindowList vector contents as weakptr rather than converting at the call site ?

[zcbenz]

I think the internal records should still be a vector of raw ptrs, the purpose of WindowList is to provide a precise list of existing windows and it is doing the work correctly. Changing the internal vector to store weak ptrs would be a weird design, because WindowList already correctly manages the window list so there is no way for its elements to be null.

There is crash because CloseAllWindows and DestroyAllWindows are deleting windows while iterating them, so I think we should only fix these methods.

[release-clerk]

Release Notes Persisted

Fixed an occasional crash when closing all BrowserWindows.

[trop]

I have automatically backported this PR to "8-x-y", please check out #23022

[trop]

I have automatically backported this PR to "9-x-y", please check out #23023

[trop]

I have automatically backported this PR to "7-2-x", please check out #23024