Bug 220385 - Error in layout tests: "Passed ITP enabled state (0) does not match TCC setting (1)"
Summary: Error in layout tests: "Passed ITP enabled state (0) does not match TCC setti...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kate Cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-06 15:00 PST by Kate Cheney
Modified: 2021-01-07 19:59 PST (History)
3 users (show)

See Also:


Attachments
Patch (1.53 KB, patch)
2021-01-06 15:14 PST, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (2.30 KB, patch)
2021-01-06 19:24 PST, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch for landing (4.74 KB, patch)
2021-01-07 17:37 PST, Kate Cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kate Cheney 2021-01-06 15:00:42 PST
This error has been appearing when running layout tests.
Comment 1 Kate Cheney 2021-01-06 15:01:09 PST
<rdar://problem/70730482>
Comment 2 Kate Cheney 2021-01-06 15:14:38 PST
Created attachment 417132 [details]
Patch
Comment 3 Kate Cheney 2021-01-06 15:14:56 PST
(In reply to katherine_cheney from comment #2)
> Created attachment 417132 [details]
> Patch

Seeing if this causes flaky failures.
Comment 4 Kate Cheney 2021-01-06 19:24:03 PST
Created attachment 417145 [details]
Patch
Comment 5 Darin Adler 2021-01-07 15:37:39 PST
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?
Comment 6 Kate Cheney 2021-01-07 15:53:30 PST
(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 7 Darin Adler 2021-01-07 15:59:04 PST
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 8 Darin Adler 2021-01-07 16:00:28 PST
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 9 Darin Adler 2021-01-07 16:13:11 PST
Comment on attachment 417145 [details]
Patch

I’m gong to say review+ but I don’t like leaving behind code that’s mysterious.
Comment 10 Darin Adler 2021-01-07 16:13:43 PST
Like seriously, if you call shouldBeTreatedAsFullBrowser and the UI process is Safari you get false?
Comment 11 Kate Cheney 2021-01-07 17:37:12 PST
Created attachment 417232 [details]
Patch for landing
Comment 12 Kate Cheney 2021-01-07 17:37:46 PST
Ok, I changed the name to isRunningTest() to be more clear. Thanks for the review!
Comment 13 EWS 2021-01-07 19:59:30 PST
Committed r271283: <https://trac.webkit.org/changeset/271283>

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