Summary: | Web Inspector: Page: mark any WebCore::Setting inspector override as UNLIKELY | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=193813 | ||||||||
Attachments: |
|
Description
Devin Rousso
2019-07-18 17:46:55 PDT
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. |