Web Inspector: Web Inspector^2 crashes after closing if Web Inspector^1 closed first

• Component: Web Inspector
• Status: RESOLVED FIXED

[BJ Burg]

.

[Radar WebKit Bug Importer]

<rdar://problem/85526508>

[BJ Burg]

Created attachment 444606 [details]
Patch v1.0

[Devin Rousso]

Comment on attachment 444606 [details]
Patch v1.0

View in context: https://bugs.webkit.org/attachment.cgi?id=444606&action=review

> Source/WebKit/ChangeLog:12
> +        Other operations using m_inspectedPage should be guarded in case it tha the inspected

oops

> Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.cpp:421
> +    m_inspectedPageIdentifier = m_inspectedPage->identifier();

Why are we only saving this here?  Why not right after when `m_inspectedPage` is set (e.g. `WebInspectorUIProxy::updateForNewPageProcess`)?

Also, when do we clear this?  Maybe when we clear `m_inspectedPage` (e.g. `WebInspectorUIProxy::reset`)?

> Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.cpp:423
> +    m_inspectorPage->process().addMessageReceiver(Messages::WebInspectorUIProxy::messageReceiverName(), m_inspectedPageIdentifier, *this);

Should we also do the same thing for the use in `WebInspectorUIProxy::openLocalInspectorFrontend`?

> Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.h:292
> +    WebPageProxyIdentifier m_inspectedPageIdentifier { };

I don't think the `{ }` is needed.

[BJ Burg]

(In reply to Devin Rousso from comment #3)
> Comment on attachment 444606 [details]
> Patch v1.0
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444606&action=review
> 
> > Source/WebKit/ChangeLog:12
> > +        Other operations using m_inspectedPage should be guarded in case it tha the inspected
> 
> oops
> 
> > Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.cpp:421
> > +    m_inspectedPageIdentifier = m_inspectedPage->identifier();
> 
> Why are we only saving this here?  Why not right after when
> `m_inspectedPage` is set (e.g.
> `WebInspectorUIProxy::updateForNewPageProcess`)?
> 
> Also, when do we clear this?  Maybe when we clear `m_inspectedPage` (e.g.
> `WebInspectorUIProxy::reset`)?

Good suggestion, I fixed it.

> > Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.cpp:423
> > +    m_inspectorPage->process().addMessageReceiver(Messages::WebInspectorUIProxy::messageReceiverName(), m_inspectedPageIdentifier, *this);
> 
> Should we also do the same thing for the use in
> `WebInspectorUIProxy::openLocalInspectorFrontend`?

I tried changing it to use the cached identifier, but it resulted in "Message not understood: EstablishConnection". We shouldn't change the cached identifier after setting it because the same destination is needed to add and remove the message receiver.

So, I think it is fine to re-query here as the code currently does.

[Devin Rousso]

Comment on attachment 444606 [details]
Patch v1.0

View in context: https://bugs.webkit.org/attachment.cgi?id=444606&action=review

>>> Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.cpp:423
>>> +    m_inspectorPage->process().addMessageReceiver(Messages::WebInspectorUIProxy::messageReceiverName(), m_inspectedPageIdentifier, *this);
>> 
>> Should we also do the same thing for the use in `WebInspectorUIProxy::openLocalInspectorFrontend`?
> 
> I tried changing it to use the cached identifier, but it resulted in "Message not understood: EstablishConnection". We shouldn't change the cached identifier after setting it because the same destination is needed to add and remove the message receiver.
> 
> So, I think it is fine to re-query here as the code currently does.

Err, I meant more to use `m_inspectedPageIdentifier` instead of `m_inspectedPage->identifier()`.  I would hope that the identifier doesn't change in between the call to `createFrontendPage` and sending `WebInspectorUI::EstablishConnection` 😅

[BJ Burg]

Created attachment 445788 [details]
Patch v1.1 (for landing)

[Devin Rousso]

Comment on attachment 445788 [details]
Patch v1.1 (for landing)

View in context: https://bugs.webkit.org/attachment.cgi?id=445788&action=review

r=me, nice! =D

> Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.cpp:424
> +    m_inspectorPage->process().addMessageReceiver(Messages::WebInspectorUIProxy::messageReceiverName(), m_inspectedPageIdentifier, *this);

please see comment #5 :)

[BJ Burg]

Committed r286498 (244837@main): <https://commits.webkit.org/244837@main>