Bug 240284 – Web Inspector: Non-reproducible crash at WebKit::WebInspectorUIProxy::platformDetach

• Component: Web Inspector
• Status: RESOLVED FIXED

[Patrick Angle]

<rdar://91720039>

[Patrick Angle]

Created attachment 459129 [details]
Patch v1.0

[Devin Rousso]

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 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]

Created attachment 459141 [details]
Patch v1.1

[Patrick Angle]

Created attachment 459194 [details]
Patch v1.2

[Devin Rousso]

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 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]

Created attachment 459264 [details]
Patch v1.3

[Patrick Angle]

Created attachment 459268 [details]
Patch v1.3.1 - Changelog formatting issue

[Patrick Angle]

Created attachment 459269 [details]
Patch v1.3.2 - Fix Windows build

[Devin Rousso]

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]

(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]

Created attachment 459437 [details]
Patch v1.4

[Patrick Angle]

Created attachment 459450 [details]
Patch v1.5

[Radar WebKit Bug Importer]

<rdar://problem/93440479>

[Patrick Angle]

<rdar://91720039>

[Patrick Angle]

Pull request: https://github.com/WebKit/WebKit/pull/1360

[EWS]

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

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