Bug 161502 - Web Inspector: If inspector process crashes, re-inspecting the page does not work
Summary: Web Inspector: If inspector process crashes, re-inspecting the page does not ...
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: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-09-01 12:19 PDT by Joseph Pecoraro
Modified: 2016-09-02 14:01 PDT (History)
6 users (show)

See Also:

Attachments
[PATCH] Proposed Fix (6.35 KB, patch)
2016-09-01 20:21 PDT, Joseph Pecoraro 		bburg: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2016-09-01 12:19:45 PDT 
Summary:
If inspector process crashes, re-inspecting the page does not work immediately.

Steps to Reproduce:
1. Inspect this page
2. shell> kill -9 <pid-of-InspectorProcess>
3. Inspect this page
  => Nothing
4. Inspect this page again
  => Inspector shows but is broken

Notes:
- WebInspectorProxy has code to handle webProcessDidCrash. It must not be cleaning up everything.
Comment 1 Radar WebKit Bug Importer 2016-09-01 12:20:01 PDT 
<rdar://problem/28120368>
Comment 2 Joseph Pecoraro 2016-09-01 20:15:13 PDT 
This happens only on Mac platforms, because WebInspectorProxyMac's platformDidClose doesn't immediately close the WebPage that crashed. It just sets a timer to close it in 60 seconds. We need a stronger "close because it crashed" case.
Comment 3 Joseph Pecoraro 2016-09-01 20:21:38 PDT 
Created attachment 287720 [details]
[PATCH] Proposed Fix
Comment 4 BJ Burg 2016-09-02 10:02:18 PDT 
Comment on attachment 287720 [details]
[PATCH] Proposed Fix

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

r=me but please re-run through EWS when our lab is not on fire

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:171
> +    if (m_inspectorProxyObjCAdapter) {

It would be kind of worrying if this were nil. Is this possible if the process crashes prior to creating the adapter (like right after we spawn it i guess) ?
Comment 5 Joseph Pecoraro 2016-09-02 10:57:22 PDT 
Well this won't apply until bug 161515. I'll just wait for that.
Comment 6 Joseph Pecoraro 2016-09-02 13:40:50 PDT 
Comment on attachment 287720 [details]
[PATCH] Proposed Fix

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

>> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:171
>> +    if (m_inspectorProxyObjCAdapter) {
> 
> It would be kind of worrying if this were nil. Is this possible if the process crashes prior to creating the adapter (like right after we spawn it i guess) ?

I don't think it is nil, but its much easier to reason about this method when it just does the right thing regardless of the state of members.
Comment 7 Joseph Pecoraro 2016-09-02 13:43:25 PDT 
(In reply to comment #6)
> Comment on attachment 287720 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287720&action=review
> 
> >> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:171
> >> +    if (m_inspectorProxyObjCAdapter) {
> > 
> > It would be kind of worrying if this were nil. Is this possible if the process crashes prior to creating the adapter (like right after we spawn it i guess) ?
> 
> I don't think it is nil, but its much easier to reason about this method
> when it just does the right thing regardless of the state of members.

I mean to say, I made these changes not because they were required, but because it just made more sense to me. Just cleanup
Comment 8 Joseph Pecoraro 2016-09-02 14:01:56 PDT 
<https://trac.webkit.org/changeset/205370>