Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Web Inspector: Non-reproducible crash at WebKit::WebInspectorUIProxy::platformDetach #1360

Merged
merged 0 commits into from Jun 8, 2022

Conversation

patrickangle
Copy link
Contributor

@patrickangle patrickangle commented Jun 7, 2022

acf9551

Web Inspector: Non-reproducible crash at WebKit::WebInspectorUIProxy::platformDetach
https://bugs.webkit.org/show_bug.cgi?id=240284
rdar://91720039

Reviewed by Devin Rousso.

Speculative fix for non-reproducible crash. When `WebInspectorUIProxy::platformDetach` is called from
WebInspectorUIProxy::closeFrontendPageAndWindow`, it appears to be possible for the inspected page to have
already been discarded (via `WebInspectorUIProxy::reset`). This is reinforced by the fact that
`WebInspectorUIProxy::closeFrontendPageAndWindow` already guards use of `inspectedPage()`. We should do the same
here, and should also guard other places we use `inspectedPage()` to help future folks (including me)
understand that the existance of an `inspectedPage()` is not guaranteed.

* UIProcess/Inspector/mac/WebInspectorUIProxyMac.mm:
(WebKit::WebInspectorUIProxy::platformCreateFrontendWindow):
- If we don't have an inspected page when creating a window for the frontend, use the default window size by
passing an empty NSRect to `WebInspectorUIProxy::createFrontendWindow`.

(WebKit::WebInspectorUIProxy::platformResetState):
(WebKit::WebInspectorUIProxy::platformBringToFront):
(WebKit::WebInspectorUIProxy::platformBringInspectedPageToFront):
(WebKit::WebInspectorUIProxy::windowFrameDidChange):
- If the inspected page no longer exists, don't try to use it.

(WebKit::WebInspectorUIProxy::platformCanAttach):
(WebKit::WebInspectorUIProxy::platformAttach):
- Update `platformCanAttach` to return false if we no longer have an inspected page, and assert that we have an
inspected page in `platformAttach` since any call to it should be guarded on `platformCanAttach`

(WebKit::WebInspectorUIProxy::platformDetach):
- We should attempt to clean up as much of the attachment as we can, but work around the possibility that the inspected
page is no longer available.
- Clean up newlines while we are here.

(WebKit::WebInspectorUIProxy::platformInspectedWindowHeight): Deleted.
(WebKit::WebInspectorUIProxy::platformInspectedWindowWidth): Deleted.
- These methods were unused on Mac.

* UIProcess/Inspector/WebInspectorUIProxy.cpp:
(WebKit::WebInspectorUIProxy::open):
- Add a check that we can actually attach in our current state when opening, otherwise fall back to creating a window instead.

(WebKit::WebInspectorUIProxy::platformInspectedWindowHeight): Deleted.
(WebKit::WebInspectorUIProxy::platformInspectedWindowWidth): Deleted.
* UIProcess/Inspector/WebInspectorUIProxy.h:
* UIProcess/Inspector/gtk/WebInspectorUIProxyGtk.cpp:
(WebKit::WebInspectorUIProxy::platformAttach):
(WebKit::WebInspectorUIProxy::platformInspectedWindowHeight): Deleted.
(WebKit::WebInspectorUIProxy::platformInspectedWindowWidth): Deleted.
* UIProcess/Inspector/win/WebInspectorUIProxyWin.cpp:
(WebKit::WebInspectorUIProxy::platformAttach):
(WebKit::WebInspectorUIProxy::platformInspectedWindowHeight): Deleted.
(WebKit::WebInspectorUIProxy::platformInspectedWindowWidth): Deleted.
- Inline Windows/GTK usage of `platformInspectedWindow*`, since they are only used once, not used by the
superclass any more, and also not used on all platforms.

Canonical link: https://commits.webkit.org/251382@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295374 268f45cc-cd09-0410-ab3c-d52691b4dbfc

@patrickangle patrickangle self-assigned this Jun 7, 2022
@patrickangle patrickangle added Web Inspector Bugs related to the WebKit Web Inspector. WebKit Nightly Build labels Jun 7, 2022
@patrickangle patrickangle added the merge-queue Applied to send a pull request to merge-queue label Jun 8, 2022
@webkit-early-warning-system webkit-early-warning-system merged commit acf9551 into WebKit:main Jun 8, 2022
@webkit-early-warning-system
Copy link
Collaborator

Committed r295374 (251382@main): https://commits.webkit.org/251382@main

Reviewed commits have been landed. Closing PR #1360 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system removed the merge-queue Applied to send a pull request to merge-queue label Jun 8, 2022
@patrickangle patrickangle deleted the eng/b240284 branch June 16, 2022 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Web Inspector Bugs related to the WebKit Web Inspector.
Projects
None yet
3 participants