Bug 233293 - Web Inspector: Web Inspector^2 crashes after closing if Web Inspector^1 closed first
Summary: Web Inspector: Web Inspector^2 crashes after closing if Web Inspector^1 close...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-17 16:42 PST by BJ Burg
Modified: 2022-03-01 02:24 PST (History)
7 users (show)

See Also:


Attachments
Patch v1.0 (5.04 KB, patch)
2021-11-17 16:48 PST, BJ Burg
no flags Details | Formatted Diff | Diff
Patch v1.1 (for landing) (5.38 KB, patch)
2021-12-02 16:29 PST, BJ Burg
hi: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2021-11-17 16:42:56 PST
.
Comment 1 Radar WebKit Bug Importer 2021-11-17 16:43:21 PST
<rdar://problem/85526508>
Comment 2 BJ Burg 2021-11-17 16:48:03 PST
Created attachment 444606 [details]
Patch v1.0
Comment 3 Devin Rousso 2021-11-17 16:56:33 PST
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.
Comment 4 BJ Burg 2021-12-02 16:17:30 PST
(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.
Comment 5 Devin Rousso 2021-12-02 16:26:48 PST
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` 😅
Comment 6 BJ Burg 2021-12-02 16:29:56 PST
Created attachment 445788 [details]
Patch v1.1 (for landing)
Comment 7 Devin Rousso 2021-12-02 16:36:29 PST
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 :)
Comment 8 BJ Burg 2021-12-03 09:45:30 PST
Committed r286498 (244837@main): <https://commits.webkit.org/244837@main>