Bug 220019 - Login flows on sites with ITP quirks no longer work with ITP disabled
Summary: Login flows on sites with ITP quirks no longer work with ITP disabled
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: 2020-12-18 10:43 PST by Kate Cheney
Modified: 2020-12-18 16:12 PST (History)
5 users (show)

See Also:


Attachments
Patch (2.11 KB, patch)
2020-12-18 10:50 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 2020-12-18 10:43:57 PST
We should add a check for ITP enabled before attempting to apply quirks, otherwise the quirks will commandeer the login process, fail (because no ITP), and thus result in inability to login when ITP is disabled.
Comment 1 Radar WebKit Bug Importer 2020-12-18 10:44:51 PST
<rdar://problem/72472858>
Comment 2 Kate Cheney 2020-12-18 10:50:09 PST
Created attachment 416529 [details]
Patch
Comment 3 Darin Adler 2020-12-18 10:59:23 PST
Comment on attachment 416529 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416529&action=review

> Source/WebCore/ChangeLog:14
> +        No new tests, this will fix login flows on sites with ITP quirks when
> +        ITP is turned off. I confirmed these cases manually.

Disappointing there’s no way to test this. A test has value even in a simple case like this, so if you do think of a way to validate it, that would be welcome.
Comment 4 Kate Cheney 2020-12-18 14:54:05 PST
(In reply to Darin Adler from comment #3)
> Comment on attachment 416529 [details]
> Patch

Thank you for the review!

> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416529&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        No new tests, this will fix login flows on sites with ITP quirks when
> > +        ITP is turned off. I confirmed these cases manually.
> 
> Disappointing there’s no way to test this. A test has value even in a simple
> case like this, so if you do think of a way to validate it, that would be
> welcome.

I have been trying to think of a good way to test this. We’d need to test that we do not trigger the quirks when ITP is disabled. I don’t believe our test infrastructure supports loading arbitrary domains, so this eliminates testing an actual quirk’d site. 

One idea is I could add a new simple quirk in this function for localhost and then create a layout test for it to make sure that the quirk does not work with ITP disabled. But, I am not sure it is a good idea to add code in WebCore that is only triggered for a test case.
Comment 5 Darin Adler 2020-12-18 15:37:45 PST
(In reply to katherine_cheney from comment #4)
> I have been trying to think of a good way to test this. We’d need to test
> that we do not trigger the quirks when ITP is disabled. I don’t believe our
> test infrastructure supports loading arbitrary domains, so this eliminates
> testing an actual quirk’d site. 

I think we should consider adding a way to "spoof" which site something is coming from in WebKitTestRunner, to test site-specific hacks. That could be used to make tests for bugs like this one, without requiring we add something specific to this quirk. Worth talking to some other WebKit people about how easy that would be.
Comment 6 Kate Cheney 2020-12-18 16:08:08 PST
(In reply to Darin Adler from comment #5)
> (In reply to katherine_cheney from comment #4)
> > I have been trying to think of a good way to test this. We’d need to test
> > that we do not trigger the quirks when ITP is disabled. I don’t believe our
> > test infrastructure supports loading arbitrary domains, so this eliminates
> > testing an actual quirk’d site. 
> 
> I think we should consider adding a way to "spoof" which site something is
> coming from in WebKitTestRunner, to test site-specific hacks. That could be
> used to make tests for bugs like this one, without requiring we add
> something specific to this quirk. Worth talking to some other WebKit people
> about how easy that would be.

Ok, I like that idea. I filed rdar://72484249 and will talk with some others about the best way to do this.
Comment 7 EWS 2020-12-18 16:12:07 PST
Committed r270995: <https://trac.webkit.org/changeset/270995>

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