Created attachment 374433 [details]
Patch

Comment on attachment 374433 [details]
Patch

Ideally this is already always nullopt if there is no inspector frontend, right?

Maybe we should just ASSERT(InspectorInstrumentation::hasFrontends()) inside the if.

(In reply to Devin Rousso from comment #0)
> This way, if the inspector crashes, we don't still use the overridden
> settings value.

Shouldn't the InspectorPageAgent have cleaned up after itself in:

    void InspectorPageAgent::willDestroyFrontendAndBackend(Inspector::DisconnectReason)
    {
        ErrorString unused;
        disable(unused);
    }

Which should set all override values to nullopt.

(In reply to Joseph Pecoraro from comment #3)
> (In reply to Devin Rousso from comment #0)
> > This way, if the inspector crashes, we don't still use the overridden settings value.
> 
> Shouldn't the InspectorPageAgent have cleaned up after itself in:
> 
>     void
> InspectorPageAgent::willDestroyFrontendAndBackend(Inspector::DisconnectReason)
>     {
>         ErrorString unused;
>         disable(unused);
>     }
> 
> Which should set all override values to nullopt.
Yes, but what about if the inspector crashes?  We could potentially end up in a state where the override didn't get removed.  Due to the potentially sensitive nature of some of these settings (e.g. CORS), this is an added precaution.

(In reply to Devin Rousso from comment #4)
> (In reply to Joseph Pecoraro from comment #3)
> > (In reply to Devin Rousso from comment #0)
> > > This way, if the inspector crashes, we don't still use the overridden settings value.
> > 
> > Shouldn't the InspectorPageAgent have cleaned up after itself in:
> > 
> >     void
> > InspectorPageAgent::willDestroyFrontendAndBackend(Inspector::DisconnectReason)
> >     {
> >         ErrorString unused;
> >         disable(unused);
> >     }
> > 
> > Which should set all override values to nullopt.
> Yes, but what about if the inspector crashes?

If the:

    • Inspected Page Web Content Process crashes
        - this state goes away and a new process will be spawned with its own WebCore::Settings

    • Inspector Web Content Process crashes
        - InspectorPageAgent::willDestroyFrontendAndBackend should run since a frontend went away

    • UIProcess Crashes
        - all of these processes go away

Am I overlooking something?

(In reply to Joseph Pecoraro from comment #5)
>     • Inspector Web Content Process crashes
>         - InspectorPageAgent::willDestroyFrontendAndBackend should run since a frontend went away
Oh, I didn't know we did that =D

In that case, I agree that this isn't "necessary".  I'll change it to just be an `ASSERT`.

Created attachment 374615 [details]
Patch

Comment on attachment 374615 [details]
Patch

r=me

Comment on attachment 374615 [details]
Patch

Clearing flags on attachment: 374615

Committed r247697: <https://trac.webkit.org/changeset/247697>

All reviewed patches have been landed.  Closing bug.

<rdar://problem/53411226>