Bug 199925 – Web Inspector: Page: mark any WebCore::Setting inspector override as UNLIKELY

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

This way, if the inspector crashes, we don't still use the overridden settings value.

[Devin Rousso]

Created attachment 374433 [details]
Patch

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

(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.

[Devin Rousso]

(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.

[Joseph Pecoraro]

(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?

[Devin Rousso]

(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`.

[Devin Rousso]

Created attachment 374615 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 374615 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 374615 [details]
Patch

Clearing flags on attachment: 374615

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

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53411226>