WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
240284
Web Inspector: Non-reproducible crash at WebKit::WebInspectorUIProxy::platformDetach
https://bugs.webkit.org/show_bug.cgi?id=240284
Summary
Web Inspector: Non-reproducible crash at WebKit::WebInspectorUIProxy::platfor...
Patrick Angle
Reported
2022-05-10 11:37:15 PDT
<
rdar://91720039
>
Attachments
Patch v1.0
(2.71 KB, patch)
2022-05-10 11:47 PDT
,
Patrick Angle
no flags
Details
Formatted Diff
Diff
Patch v1.1
(7.77 KB, patch)
2022-05-10 17:11 PDT
,
Patrick Angle
no flags
Details
Formatted Diff
Diff
Patch v1.2
(7.35 KB, patch)
2022-05-11 18:01 PDT
,
Patrick Angle
no flags
Details
Formatted Diff
Diff
Patch v1.3
(14.33 KB, patch)
2022-05-12 17:43 PDT
,
Patrick Angle
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch v1.3.1 - Changelog formatting issue
(14.32 KB, patch)
2022-05-12 18:21 PDT
,
Patrick Angle
no flags
Details
Formatted Diff
Diff
Patch v1.3.2 - Fix Windows build
(14.33 KB, patch)
2022-05-12 18:29 PDT
,
Patrick Angle
no flags
Details
Formatted Diff
Diff
Patch v1.4
(14.90 KB, patch)
2022-05-16 10:53 PDT
,
Patrick Angle
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch v1.5
(14.92 KB, patch)
2022-05-16 13:17 PDT
,
Patrick Angle
pangle
: review?
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Patrick Angle
Comment 1
2022-05-10 11:47:28 PDT
Created
attachment 459129
[details]
Patch v1.0
Devin Rousso
Comment 2
2022-05-10 14:48:31 PDT
Comment on
attachment 459129
[details]
Patch v1.0 View in context:
https://bugs.webkit.org/attachment.cgi?id=459129&action=review
> Source/WebKit/UIProcess/Inspector/mac/WebInspectorUIProxyMac.mm:764 > + if (inspectedView)
I think we may also need to add this check inside `platformAttach` and/or `platformCreateFrontendWindow` since those can be called by `open`. Frankly, it might be best to just add a check for all of the `inspectedPage()` in this file 😅
Patrick Angle
Comment 3
2022-05-10 16:29:25 PDT
Comment on
attachment 459129
[details]
Patch v1.0 View in context:
https://bugs.webkit.org/attachment.cgi?id=459129&action=review
>> Source/WebKit/UIProcess/Inspector/mac/WebInspectorUIProxyMac.mm:764 >> + if (inspectedView) > > I think we may also need to add this check inside `platformAttach` and/or `platformCreateFrontendWindow` since those can be called by `open`. > > Frankly, it might be best to just add a check for all of the `inspectedPage()` in this file 😅
`open` shouldn't be reachable in this case because `WebInspectorUIProxy::closeFrontendPageAndWindow` will have already set m_isVisible to false. But I take your point that we should defend other call-sites anyways.
Patrick Angle
Comment 4
2022-05-10 17:11:06 PDT
Created
attachment 459141
[details]
Patch v1.1
Patrick Angle
Comment 5
2022-05-11 18:01:04 PDT
Created
attachment 459194
[details]
Patch v1.2
Devin Rousso
Comment 6
2022-05-12 11:43:16 PDT
Comment on
attachment 459194
[details]
Patch v1.2 View in context:
https://bugs.webkit.org/attachment.cgi?id=459194&action=review
> Source/WebKit/UIProcess/Inspector/mac/WebInspectorUIProxyMac.mm:425 > + ASSERT(inspectedPage());
how sure are we that each of these `ASSERT(inspectedPage())` is in fact unreachable (i.e. can never cause problems)? as an example, this function is called by `WebInspectorUIProxy::elementSelectionChanged`, which seems to come from IPC, so I'm not sure what guarantees we have about this being valid here :/
Patrick Angle
Comment 7
2022-05-12 14:19:27 PDT
Comment on
attachment 459194
[details]
Patch v1.2 View in context:
https://bugs.webkit.org/attachment.cgi?id=459194&action=review
>> Source/WebKit/UIProcess/Inspector/mac/WebInspectorUIProxyMac.mm:425 >> + ASSERT(inspectedPage()); > > how sure are we that each of these `ASSERT(inspectedPage())` is in fact unreachable (i.e. can never cause problems)? > > as an example, this function is called by `WebInspectorUIProxy::elementSelectionChanged`, which seems to come from IPC, so I'm not sure what guarantees we have about this being valid here :/
I guess it happening over IPC means we could end up racing teardown, yeah... I stand by my assertion (see what I did there 😉) for platformCreateFrontendWindow, since we should never as far as I can tell not have an inspected page at that point, and if we do what are we even trying to inspect. platformInspectedWindowHeight/Width on further investigation are only reachable on GTK/Windows, and only used within their WebInspectorUIProxy*.cpp file. So turns out those are very unreachable 😅. I'll do that cleanup now. platformAttach should also be an assertion IMO because it would be a mistake to call platformAttach without checking platformCanAttach first.
Patrick Angle
Comment 8
2022-05-12 17:43:56 PDT
Created
attachment 459264
[details]
Patch v1.3
Patrick Angle
Comment 9
2022-05-12 18:21:47 PDT
Created
attachment 459268
[details]
Patch v1.3.1 - Changelog formatting issue
Patrick Angle
Comment 10
2022-05-12 18:29:29 PDT
Created
attachment 459269
[details]
Patch v1.3.2 - Fix Windows build
Devin Rousso
Comment 11
2022-05-13 15:41:14 PDT
Comment on
attachment 459194
[details]
Patch v1.2 View in context:
https://bugs.webkit.org/attachment.cgi?id=459194&action=review
>>> Source/WebKit/UIProcess/Inspector/mac/WebInspectorUIProxyMac.mm:425 >>> + ASSERT(inspectedPage()); >> >> how sure are we that each of these `ASSERT(inspectedPage())` is in fact unreachable (i.e. can never cause problems)? >> >> as an example, this function is called by `WebInspectorUIProxy::elementSelectionChanged`, which seems to come from IPC, so I'm not sure what guarantees we have about this being valid here :/ > > I guess it happening over IPC means we could end up racing teardown, yeah... > > I stand by my assertion (see what I did there 😉) for platformCreateFrontendWindow, since we should never as far as I can tell not have an inspected page at that point, and if we do what are we even trying to inspect. > > platformInspectedWindowHeight/Width on further investigation are only reachable on GTK/Windows, and only used within their WebInspectorUIProxy*.cpp file. So turns out those are very unreachable 😅. I'll do that cleanup now. > > platformAttach should also be an assertion IMO because it would be a mistake to call platformAttach without checking platformCanAttach first.
i think both `platformCreateFrontendWindow` and `platformAttach` can be reached via `bringToFront`
Patrick Angle
Comment 12
2022-05-13 17:35:22 PDT
(In reply to Devin Rousso from
comment #11
)
> Comment on
attachment 459194
[details]
> Patch v1.2 > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=459194&action=review
> > >>> Source/WebKit/UIProcess/Inspector/mac/WebInspectorUIProxyMac.mm:425 > >>> + ASSERT(inspectedPage()); > >> > >> how sure are we that each of these `ASSERT(inspectedPage())` is in fact unreachable (i.e. can never cause problems)? > >> > >> as an example, this function is called by `WebInspectorUIProxy::elementSelectionChanged`, which seems to come from IPC, so I'm not sure what guarantees we have about this being valid here :/ > > > > I guess it happening over IPC means we could end up racing teardown, yeah... > > > > I stand by my assertion (see what I did there 😉) for platformCreateFrontendWindow, since we should never as far as I can tell not have an inspected page at that point, and if we do what are we even trying to inspect. > > [...] > > > > platformAttach should also be an assertion IMO because it would be a mistake to call platformAttach without checking platformCanAttach first. > > i think both `platformCreateFrontendWindow` and `platformAttach` can be > reached via `bringToFront`
Sorry, I should have responded to this again because I'd discovered more now that I've dug further into this beyond the narrow issue I set out to resolve. You are correct, they are reachable, and the latest patch makes the assumption that we might not always have an inspected page for each occurrence that had before this patch not asserted/guarded `inspectedPage()`, with the exception of `platformAttach` because I had previously thought it was only reachable from `attach`, which guards on `platformCanAttach` which in the latest patch returns false when there is no `inspectedPage`... Taking a fresh look today even my assertion in `platformAttach` isn't a given because `open` doesn't guard calling `platformAttach` on `platformCanAttach`... But that feels better handled by augmenting the existing check in `open` to handle this situation. Probably something along the lines of... ``` if (m_attached && platformCanAttach(m_canAttach)) platformAttach(); else { m_isAttached = false; platformCreateFrontendWindow(); } ``` since the alternative would probably end up being ignoring the call to `open` if we were to bail inside `platformAttach`.
Patrick Angle
Comment 13
2022-05-16 10:53:48 PDT
Created
attachment 459437
[details]
Patch v1.4
Patrick Angle
Comment 14
2022-05-16 13:17:54 PDT
Created
attachment 459450
[details]
Patch v1.5
Radar WebKit Bug Importer
Comment 15
2022-05-17 11:38:15 PDT
<
rdar://problem/93440479
>
Patrick Angle
Comment 16
2022-05-17 11:48:13 PDT
<
rdar://91720039
>
Patrick Angle
Comment 17
2022-06-07 15:01:33 PDT
Pull request:
https://github.com/WebKit/WebKit/pull/1360
EWS
Comment 18
2022-06-07 21:35:12 PDT
Committed
r295374
(
251382@main
): <
https://commits.webkit.org/251382@main
> Reviewed commits have been landed. Closing PR #1360 and removing active labels.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug