Login flows on sites with ITP quirks no longer work with ITP disabled

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

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.

[Radar WebKit Bug Importer]

<rdar://problem/72472858>

[Kate Cheney]

Created attachment 416529 [details]
Patch

[Darin Adler]

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.

[Kate Cheney]

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

[Darin Adler]

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

[Kate Cheney]

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

[EWS]

Committed r270995: <https://trac.webkit.org/changeset/270995>

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