Bug 236672

Summary: Web Inspector: [Cocoa] Reentrancy in WebKit::WebInspectorUIProxy::open
Product: WebKit Reporter: Patrick Angle <pangle>
Component: Web InspectorAssignee: Patrick Angle <pangle>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch v1.0
none
[fast-cq] Patch v1.1 - Use RELEASE_LOG for logging none

Description Patrick Angle 2022-02-15 14:59:17 PST
<rdar://88358696>
Comment 1 Patrick Angle 2022-02-15 15:12:40 PST
Created attachment 452097 [details]
Patch v1.0
Comment 2 Devin Rousso 2022-02-15 16:02:33 PST
Comment on attachment 452097 [details]
Patch v1.0

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

rs=me

> Source/WebKit/UIProcess/Inspector/mac/WebInspectorUIProxyMac.mm:406
> +            WTFLogAlways("WebInspectorUIProxy::platformBringToFront - Inspected and inspector windows did not match while opening inspector. Falling back to detached inspector. Inspected page had window: %s", inspectedPage()->platformWindow() ? "YES" : "NO");

Do we _always_ want/need to log this?

Also, maybe we should use the `Inspector` log channel (e.g. `LOG(Inspector, ...);`)?
Comment 3 Patrick Angle 2022-02-15 16:22:24 PST
Comment on attachment 452097 [details]
Patch v1.0

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

>> Source/WebKit/UIProcess/Inspector/mac/WebInspectorUIProxyMac.mm:406
>> +            WTFLogAlways("WebInspectorUIProxy::platformBringToFront - Inspected and inspector windows did not match while opening inspector. Falling back to detached inspector. Inspected page had window: %s", inspectedPage()->platformWindow() ? "YES" : "NO");
> 
> Do we _always_ want/need to log this?
> 
> Also, maybe we should use the `Inspector` log channel (e.g. `LOG(Inspector, ...);`)?

I'd ideally like this logging to be present when someone files an issue with a sysdiagnose saying "hey, if I do these specific things in this order, Web Inspector detaches from the view into its own window". I can use `RELEASE_LOG` instead though, which allows us to target the Inspector channel. I think you are right that `*LOG(Inspector` is probably a better idea than `WTFLogAlways` here. Part of my thought process behind having the logging is so that we can hopefully better understand a report of "Web Inspector detached into a separate window" and get further clues as to the conditions that cause this in the first place.
Comment 4 Patrick Angle 2022-02-15 16:48:30 PST
Created attachment 452110 [details]
[fast-cq] Patch v1.1 - Use RELEASE_LOG for logging
Comment 5 EWS 2022-02-15 21:21:21 PST
Committed r289875 (247313@main): <https://commits.webkit.org/247313@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452110 [details].