This error has been appearing when running layout tests.
<rdar://problem/70730482>
Created attachment 417132 [details] Patch
(In reply to katherine_cheney from comment #2) > Created attachment 417132 [details] > Patch Seeing if this causes flaky failures.
Created attachment 417145 [details] Patch
Comment on attachment 417145 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417145&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1200 > + // We do not need to log a discrepency between states for WebKitTestRunner or TestWebKitAPI. Spelling error in discrepancy. > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1201 > + if (itpEnabled != passedEnabledState && !shouldBeTreatedAsFullBrowser(WebCore::applicationBundleIdentifier())) I don’t understand why shouldBeTreatedAsFullBrowser is the correct check here. I understand that we don’t want logs in testing because this is normal there. But it seems we think it’s important to keep the logs for other cases. What are those cases where we do want logs? Why? Who reads these logs?
(In reply to Darin Adler from comment #5) > Comment on attachment 417145 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=417145&action=review > > > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1200 > > + // We do not need to log a discrepency between states for WebKitTestRunner or TestWebKitAPI. > > Spelling error in discrepancy. > Will fix. > > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1201 > > + if (itpEnabled != passedEnabledState && !shouldBeTreatedAsFullBrowser(WebCore::applicationBundleIdentifier())) > > I don’t understand why shouldBeTreatedAsFullBrowser is the correct check > here. > shouldBeTreatedAsFullBrowser() is a check to determine if the app is WebKitTestRunner or TestWebKitAPI, because most of the time they should be treated as full browser applications to properly run tests. Here it is a convenient way to check for WTR/TWA without duplicating code. > I understand that we don’t want logs in testing because this is normal > there. But it seems we think it’s important to keep the logs for other > cases. What are those cases where we do want logs? Why? Who reads these logs? The intention for this logging was to capture the case where an app manages to have a different ITP state than the user-selected value. We want to note this in case the UIProcess is trying to bypass the TCC check. It was a RELEASE_ASSERT originally but was causing issues (see rdar://60027693). I have been fielding reports of these logs, although I do think the presence of an ASSERT would bring more attention to these logs once rdar://60027693 gets resolved.
Comment on attachment 417145 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417145&action=review >>> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1201 >>> + if (itpEnabled != passedEnabledState && !shouldBeTreatedAsFullBrowser(WebCore::applicationBundleIdentifier())) >> >> I don’t understand why shouldBeTreatedAsFullBrowser is the correct check here. >> >> I understand that we don’t want logs in testing because this is normal there. But it seems we think it’s important to keep the logs for other cases. What are those cases where we do want logs? Why? Who reads these logs? > > shouldBeTreatedAsFullBrowser() is a check to determine if the app is WebKitTestRunner or TestWebKitAPI, because most of the time they should be treated as full browser applications to properly run tests. Here it is a convenient way to check for WTR/TWA without duplicating code. I don’t understand why a "is running tests" function is named "shouldBeTreatedAsFullBrowser".
Comment on attachment 417145 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417145&action=review >>>> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1201 >>>> + if (itpEnabled != passedEnabledState && !shouldBeTreatedAsFullBrowser(WebCore::applicationBundleIdentifier())) >>> >>> I don’t understand why shouldBeTreatedAsFullBrowser is the correct check here. >>> >>> I understand that we don’t want logs in testing because this is normal there. But it seems we think it’s important to keep the logs for other cases. What are those cases where we do want logs? Why? Who reads these logs? >> >> shouldBeTreatedAsFullBrowser() is a check to determine if the app is WebKitTestRunner or TestWebKitAPI, because most of the time they should be treated as full browser applications to properly run tests. Here it is a convenient way to check for WTR/TWA without duplicating code. > > I don’t understand why a "is running tests" function is named "shouldBeTreatedAsFullBrowser". I think we’ll need to rename this or add a second function that does the same thing.
Comment on attachment 417145 [details] Patch I’m gong to say review+ but I don’t like leaving behind code that’s mysterious.
Like seriously, if you call shouldBeTreatedAsFullBrowser and the UI process is Safari you get false?
Created attachment 417232 [details] Patch for landing
Ok, I changed the name to isRunningTest() to be more clear. Thanks for the review!
Committed r271283: <https://trac.webkit.org/changeset/271283> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417232 [details].