Bug 220385 – Error in layout tests: "Passed ITP enabled state (0) does not match TCC setting (1)"

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

This error has been appearing when running layout tests.

[Kate Cheney]

<rdar://problem/70730482>

[Kate Cheney]

Created attachment 417132 [details]
Patch

[Kate Cheney]

(In reply to katherine_cheney from comment #2)
> Created attachment 417132 [details]
> Patch

Seeing if this causes flaky failures.

[Kate Cheney]

Created attachment 417145 [details]
Patch

[Darin Adler]

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?

[Kate Cheney]

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

[Darin Adler]

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

[Darin Adler]

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.

[Darin Adler]

Comment on attachment 417145 [details]
Patch

I’m gong to say review+ but I don’t like leaving behind code that’s mysterious.

[Darin Adler]

Like seriously, if you call shouldBeTreatedAsFullBrowser and the UI process is Safari you get false?

[Kate Cheney]

Created attachment 417232 [details]
Patch for landing

[Kate Cheney]

Ok, I changed the name to isRunningTest() to be more clear. Thanks for the review!

[EWS]

Committed r271283: <https://trac.webkit.org/changeset/271283>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417232 [details].