We have found a handful of sites attempting to authenticate against an unrelated third party. If enough sites use the third party, it get's recognized as a prevalent resource and is blocked or partitioned. Add a temporary compatibility hack to support testing while these sites correct their content.
Created attachment 315507 [details] Patch
Comment on attachment 315507 [details] Patch Attachment 315507 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4122733 New failing tests: security/contentSecurityPolicy/video-with-data-url-allowed-by-media-src-star.html storage/indexeddb/modern/new-database-after-user-delete.html
Created attachment 315522 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 315507 [details] Patch Attachment 315507 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4123320 New failing tests: imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html
Created attachment 315528 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 315507 [details] Patch r=me
Comment on attachment 315507 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315507&action=review > Source/WebCore/loader/ResourceLoadObserver.cpp:139 > +static bool resourceNeedsSSOQuirk(const URL& potentialSSO) potentialSSO -> potentialSSOURL ? > Source/WebCore/loader/ResourceLoadObserver.cpp:142 > + static const char* const ssoOrigns[] = { typo: ssoOrigns > Source/WebCore/loader/ResourceLoadObserver.cpp:151 > + const auto& lowercaseDomain = potentialSSO.host().convertToASCIILowercase(); Why do we convert to lowercase if the HashSet uses ASCIICaseInsensitiveHash ?
<rdar://problem/33325881>
Created attachment 315957 [details] Patch
Comment on attachment 315957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315957&action=review > Source/WebCore/loader/ResourceLoadObserver.cpp:139 > +// FIXME: This quirk was added to address <rdar://problem/33325881> and should be removed once content is fixed. What does it mean for the content to be fixed? What is broken? > Source/WebCore/loader/ResourceLoadObserver.cpp:146 > + static NeverDestroyed<HashSet<String, ASCIICaseInsensitiveHash>> ssoOriginsHash = [] { > + HashSet<String, ASCIICaseInsensitiveHash> set; > + set.add(ASCIILiteral("sp.auth.adobe.com")); > + return set; > + }(); This can be written as static const auto ssoOriginsHash = makeNeverDestroyed(HashSet<String, ASCIICaseInsensitiveHash> { "sp.auth.adobe.com" };
Comment on attachment 315957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315957&action=review >> Source/WebCore/loader/ResourceLoadObserver.cpp:139 >> +// FIXME: This quirk was added to address <rdar://problem/33325881> and should be removed once content is fixed. > > What does it mean for the content to be fixed? What is broken? You cannot currently log into abc.go.com? >> Source/WebCore/loader/ResourceLoadObserver.cpp:146 >> + }(); > > This can be written as > > static const auto ssoOriginsHash = makeNeverDestroyed(HashSet<String, ASCIICaseInsensitiveHash> { > "sp.auth.adobe.com" > }; Looks nicer, will do.
Created attachment 315991 [details] Patch
(In reply to Chris Dumez from comment #11) > Comment on attachment 315957 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315957&action=review > > >> Source/WebCore/loader/ResourceLoadObserver.cpp:139 > >> +// FIXME: This quirk was added to address <rdar://problem/33325881> and should be removed once content is fixed. > > > > What does it mean for the content to be fixed? What is broken? > > You cannot currently log into abc.go.com? I meant, what exactly is sp.auth.adobe.com need to do to fix their content? Should this be disabled by the "Disable Site-specific Hacks" menu item (which is bound to Settings's needsSiteSpecificQuirks member)?
(In reply to Sam Weinig from comment #13) > (In reply to Chris Dumez from comment #11) > > Comment on attachment 315957 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=315957&action=review > > > > >> Source/WebCore/loader/ResourceLoadObserver.cpp:139 > > >> +// FIXME: This quirk was added to address <rdar://problem/33325881> and should be removed once content is fixed. > > > > > > What does it mean for the content to be fixed? What is broken? > > > > You cannot currently log into abc.go.com? > > I meant, what exactly is sp.auth.adobe.com need to do to fix their content? This information is in the radar and I don't think the comment is the right place to provide such information. > > Should this be disabled by the "Disable Site-specific Hacks" menu item > (which is bound to Settings's needsSiteSpecificQuirks member)? I can look into it but I seem to remember that last time Simon suggested this and we looked into it, that setting was disabled for some configurations we cared about.
(In reply to Chris Dumez from comment #14) > (In reply to Sam Weinig from comment #13) > > (In reply to Chris Dumez from comment #11) > > > Comment on attachment 315957 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=315957&action=review > > > > > > >> Source/WebCore/loader/ResourceLoadObserver.cpp:139 > > > >> +// FIXME: This quirk was added to address <rdar://problem/33325881> and should be removed once content is fixed. > > > > > > > > What does it mean for the content to be fixed? What is broken? > > > > > > You cannot currently log into abc.go.com? > > > > I meant, what exactly is sp.auth.adobe.com need to do to fix their content? > > This information is in the radar and I don't think the comment is the right > place to provide such information. > > > > > Should this be disabled by the "Disable Site-specific Hacks" menu item > > (which is bound to Settings's needsSiteSpecificQuirks member)? > > I can look into it but I seem to remember that last time Simon suggested > this and we looked into it, that setting was disabled for some > configurations we cared about. I think it was iOS. I cannot find any Cocoa API to get this setting and the setting is off by default. Am I missing something?
(In reply to Chris Dumez from comment #15) > (In reply to Chris Dumez from comment #14) > > (In reply to Sam Weinig from comment #13) > > > (In reply to Chris Dumez from comment #11) > > > > Comment on attachment 315957 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=315957&action=review > > > > > > > > >> Source/WebCore/loader/ResourceLoadObserver.cpp:139 > > > > >> +// FIXME: This quirk was added to address <rdar://problem/33325881> and should be removed once content is fixed. > > > > > > > > > > What does it mean for the content to be fixed? What is broken? > > > > > > > > You cannot currently log into abc.go.com? > > > > > > I meant, what exactly is sp.auth.adobe.com need to do to fix their content? > > > > This information is in the radar and I don't think the comment is the right > > place to provide such information. > > > > > > > > Should this be disabled by the "Disable Site-specific Hacks" menu item > > > (which is bound to Settings's needsSiteSpecificQuirks member)? > > > > I can look into it but I seem to remember that last time Simon suggested > > this and we looked into it, that setting was disabled for some > > configurations we cared about. > > I think it was iOS. I cannot find any Cocoa API to get this setting and the > setting is off by default. Am I missing something? Just because it is missing on iOS, why wouldn't we want to provide the functionality on Mac? That said, there may be no objc API for this yet, but it would be easy to add, and if you want, one can just cast WKPreferences to WKPreferencesRef and use the C-SPI (though I would recommend adding the objc API if you want this functionality on iOS).
Comment on attachment 315991 [details] Patch r=me
> Should this be disabled by the "Disable Site-specific Hacks" menu item > (which is bound to Settings's needsSiteSpecificQuirks member)? Yeah, I agree that it would be nice for the menu item to disable this workaround, so folks working on a fix can test the fix.
Comment on attachment 315991 [details] Patch Clearing flags on attachment: 315991 Committed r219701: <http://trac.webkit.org/changeset/219701>
All reviewed patches have been landed. Closing bug.
(In reply to Geoffrey Garen from comment #18) > > Should this be disabled by the "Disable Site-specific Hacks" menu item > > (which is bound to Settings's needsSiteSpecificQuirks member)? > > Yeah, I agree that it would be nice for the menu item to disable this > workaround, so folks working on a fix can test the fix. Looking into this now. Because the setting is currently disabled on iOS, and because I do not want to enable this setting on iOS at this point to avoid unexpected changes in behavior. I guess we can use this setting but only rely on it on Mac?
(In reply to Chris Dumez from comment #21) > (In reply to Geoffrey Garen from comment #18) > > > Should this be disabled by the "Disable Site-specific Hacks" menu item > > > (which is bound to Settings's needsSiteSpecificQuirks member)? > > > > Yeah, I agree that it would be nice for the menu item to disable this > > workaround, so folks working on a fix can test the fix. > > Looking into this now. Because the setting is currently disabled on iOS, and > because I do not want to enable this setting on iOS at this point to avoid > unexpected changes in behavior. I guess we can use this setting but only > rely on it on Mac? https://bugs.webkit.org/show_bug.cgi?id=174691
Comment on attachment 315991 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315991&action=review > Source/WebCore/loader/ResourceLoadObserver.cpp:145 > + static const auto ssoOriginsHash = makeNeverDestroyed(HashSet<String, ASCIICaseInsensitiveHash> { > + "sp.auth.adobe.com" > + }); > + return ssoOriginsHash.get().contains(url.host()); Why not just call equalLetterIgnoringASCIICase here? Seems overkill to construct a single element HashSet.
(In reply to Darin Adler from comment #23) > Comment on attachment 315991 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315991&action=review > > > Source/WebCore/loader/ResourceLoadObserver.cpp:145 > > + static const auto ssoOriginsHash = makeNeverDestroyed(HashSet<String, ASCIICaseInsensitiveHash> { > > + "sp.auth.adobe.com" > > + }); > > + return ssoOriginsHash.get().contains(url.host()); > > Why not just call equalLetterIgnoringASCIICase here? Seems overkill to > construct a single element HashSet. I thought about it but using the set is more scalable given that I expect more domains may need to get added. That said, I do not mind using equalLetterIgnoringASCIICase() until we add more domains.
(In reply to Chris Dumez from comment #24) > I thought about it but using the set is more scalable given that I expect > more domains may need to get added. That said, I do not mind using > equalLetterIgnoringASCIICase() until we add more domains. I am annoyed by the overuse of HashSet for fixed sets where more efficient algorithms may work better. I think I need to build a FixedSet family of class templates. Then it can be a HashSet only when the set is huge, an array with binary search when it’s smaller, an array with linear search when it’s even smaller, and a single function call when it has exactly one element. Either FixedSet can automatically do the right thing, we can have multiple FixedSet classes with identical interfaces but different implementations (FixedSingleItemSet, FixedLinearSearchSet, FixedBinarySearchSet, FixedHashSet), or FixedSet can take a template parameter to tell it which type of implementation to generate.
(In reply to Darin Adler from comment #25) > (In reply to Chris Dumez from comment #24) > > I thought about it but using the set is more scalable given that I expect > > more domains may need to get added. That said, I do not mind using > > equalLetterIgnoringASCIICase() until we add more domains. > > I am annoyed by the overuse of HashSet for fixed sets where more efficient > algorithms may work better. I think I need to build a FixedSet family of > class templates. Then it can be a HashSet only when the set is huge, an > array with binary search when it’s smaller, an array with linear search when > it’s even smaller, and a single function call when it has exactly one > element. Either FixedSet can automatically do the right thing, we can have > multiple FixedSet classes with identical interfaces but different > implementations (FixedSingleItemSet, FixedLinearSearchSet, > FixedBinarySearchSet, FixedHashSet), or FixedSet can take a template > parameter to tell it which type of implementation to generate. I filed bug 174715 to track this.