Bug 220106 – Can't login to Skype from Microsoft Outlook account in Safari

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

[Kate Cheney]

Skype.com requires 3rd party cookies in order to complete the login flow under outlook.live.com.

[Kate Cheney]

rdar://problem/72453487

[Kate Cheney]

Created attachment 416695 [details]
Patch

[Alex Christensen]

Comment on attachment 416695 [details]
Patch

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

> Source/WebCore/page/Quirks.cpp:976
> +        return element.hasClass() && (element.classNames().contains("glyph_signIn_circle") || element.classNames().contains("mectrl_headertext") || element.classNames().contains("mectrl_header") || element.classNames().contains("ext-button primary") || element.classNames().contains("ext-primary"));

This would probably be easier to read if everything after "return element.hasClass() && (" had new lines.

> Source/WebCore/page/Quirks.cpp:981
> +        return element.hasClass() && (element.classNames().contains("_3ioEp2RGR5vb0gqRDsaFPa") || element.classNames().contains("_2Am2jvTaBz17UJ8XnfxFOy"));

Are these static?  They look like they might change.

> Source/WebCore/page/Quirks.cpp:1038
> +    DocumentStorageAccess::requestStorageAccessForNonDocumentQuirk(*m_document, WTFMove(domainInNeedOfStorageAccess), [firstPartyDomain, domainInNeedOfStorageAccess, completionHandler = WTFMove(completionHandler)](StorageAccessWasGranted storageAccessGranted) mutable {

m_document is a WeakPtr.  We should probably check if it's null.

> Source/WebCore/page/Quirks.cpp:1040
> +            return;

You need to call completionHandler here.

> Source/WebCore/page/Quirks.cpp:1125
> +            return requestStorageAccessAndHandleClick([&element, platformEvent, eventType, detail, relatedTarget] {

Will this lambda be called immediately always?  If not, it's unsafe to capture element by reference.  If yes, you may as well just use [&] instead of listing all captured variables.

> Source/WebCore/page/Quirks.cpp:1136
> +                auto domWindow = document->domWindow();

document is a WeakPtr.  We should probably check if it's null.

> Source/WebCore/page/Quirks.cpp:1143
> +                    if (is<Frame>(*abstractFrame)) {

I think we shouldn't dereference abstractFrame here without checking it.  Just use is<Frame>(abstractFrame)

> Source/WebCore/platform/network/NetworkStorageSession.cpp:434
> +    static NeverDestroyed<HashMap<String, RegistrableDomain>> map = [] {

Since this map is never added to after initializing, it seems like overkill to even have a map here.  Why don't we just have if (urlToMap.host() == "login.live.com") return "microsoft.com"; return urlToMap;  or something like that.  I also think this should live in Quirks rather than NetworkStorageSession because it has nothing to do with storage.

[Kate Cheney]

Comment on attachment 416695 [details]
Patch

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

Thanks for the comments!

>> Source/WebCore/page/Quirks.cpp:976
>> +        return element.hasClass() && (element.classNames().contains("glyph_signIn_circle") || element.classNames().contains("mectrl_headertext") || element.classNames().contains("mectrl_header") || element.classNames().contains("ext-button primary") || element.classNames().contains("ext-primary"));
> 
> This would probably be easier to read if everything after "return element.hasClass() && (" had new lines.

Ok, will change.

>> Source/WebCore/page/Quirks.cpp:981
>> +        return element.hasClass() && (element.classNames().contains("_3ioEp2RGR5vb0gqRDsaFPa") || element.classNames().contains("_2Am2jvTaBz17UJ8XnfxFOy"));
> 
> Are these static?  They look like they might change.

They do look rather random, but they are the class names of the Skype button element. As far as I can tell they are not any more likely to change than the other element class names, like sb-signin-button etc.

>> Source/WebCore/page/Quirks.cpp:1038
>> +    DocumentStorageAccess::requestStorageAccessForNonDocumentQuirk(*m_document, WTFMove(domainInNeedOfStorageAccess), [firstPartyDomain, domainInNeedOfStorageAccess, completionHandler = WTFMove(completionHandler)](StorageAccessWasGranted storageAccessGranted) mutable {
> 
> m_document is a WeakPtr.  We should probably check if it's null.

It is checked in the function that calls this one. Is that enough, or is it better practice to do it here as well?

>> Source/WebCore/page/Quirks.cpp:1040
>> +            return;
> 
> You need to call completionHandler here.

Will fix.

>> Source/WebCore/page/Quirks.cpp:1125
>> +            return requestStorageAccessAndHandleClick([&element, platformEvent, eventType, detail, relatedTarget] {
> 
> Will this lambda be called immediately always?  If not, it's unsafe to capture element by reference.  If yes, you may as well just use [&] instead of listing all captured variables.

It is called immediately after the user grants storage access. This has to go through IPC to the network process, though, so not always right away. I will remove the reference.

>> Source/WebCore/page/Quirks.cpp:1136
>> +                auto domWindow = document->domWindow();
> 
> document is a WeakPtr.  We should probably check if it's null.

You can't see it in the diff but it is checked at the beginning of this function.

>> Source/WebCore/page/Quirks.cpp:1143
>> +                    if (is<Frame>(*abstractFrame)) {
> 
> I think we shouldn't dereference abstractFrame here without checking it.  Just use is<Frame>(abstractFrame)

Ok, will change.

>> Source/WebCore/platform/network/NetworkStorageSession.cpp:434
>> +    static NeverDestroyed<HashMap<String, RegistrableDomain>> map = [] {
> 
> Since this map is never added to after initializing, it seems like overkill to even have a map here.  Why don't we just have if (urlToMap.host() == "login.live.com") return "microsoft.com"; return urlToMap;  or something like that.  I also think this should live in Quirks rather than NetworkStorageSession because it has nothing to do with storage.

You're right. I will adjust accordingly.

[Alex Christensen]

Comment on attachment 416695 [details]
Patch

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

>>> Source/WebCore/page/Quirks.cpp:1038
>>> +    DocumentStorageAccess::requestStorageAccessForNonDocumentQuirk(*m_document, WTFMove(domainInNeedOfStorageAccess), [firstPartyDomain, domainInNeedOfStorageAccess, completionHandler = WTFMove(completionHandler)](StorageAccessWasGranted storageAccessGranted) mutable {
>> 
>> m_document is a WeakPtr.  We should probably check if it's null.
> 
> It is checked in the function that calls this one. Is that enough, or is it better practice to do it here as well?

I don't see any guarantees that the lambda is called synchronously.  If it is, it's not necessary to do another check, but it wouldn't hurt in case someone changes it in the future.

>>> Source/WebCore/page/Quirks.cpp:1136
>>> +                auto domWindow = document->domWindow();
>> 
>> document is a WeakPtr.  We should probably check if it's null.
> 
> You can't see it in the diff but it is checked at the beginning of this function.

Same.  It might be checked in the calling function, but when the inside of the lambda is called the Document may have been destroyed.

[Kate Cheney]

Comment on attachment 416695 [details]
Patch

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

>>> Source/WebCore/page/Quirks.cpp:1125
>>> +            return requestStorageAccessAndHandleClick([&element, platformEvent, eventType, detail, relatedTarget] {
>> 
>> Will this lambda be called immediately always?  If not, it's unsafe to capture element by reference.  If yes, you may as well just use [&] instead of listing all captured variables.
> 
> It is called immediately after the user grants storage access. This has to go through IPC to the network process, though, so not always right away. I will remove the reference.

I remember why I did this. WebCore::Element has an implicitly-deleted copy constructor because it's eventual base class 'CanMakeWeakPtr<WebCore::Node>' has a deleted copy constructor. Is there any way to make it safe to pass by reference even with IPC or no?

[Kate Cheney]

Created attachment 416699 [details]
Patch

[Kate Cheney]

(In reply to katherine_cheney from comment #6)
> Comment on attachment 416695 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416695&action=review
> 
> >>> Source/WebCore/page/Quirks.cpp:1125
> >>> +            return requestStorageAccessAndHandleClick([&element, platformEvent, eventType, detail, relatedTarget] {
> >> 
> >> Will this lambda be called immediately always?  If not, it's unsafe to capture element by reference.  If yes, you may as well just use [&] instead of listing all captured variables.
> > 
> > It is called immediately after the user grants storage access. This has to go through IPC to the network process, though, so not always right away. I will remove the reference.
> 
> I remember why I did this. WebCore::Element has an implicitly-deleted copy
> constructor because it's eventual base class 'CanMakeWeakPtr<WebCore::Node>'
> has a deleted copy constructor. Is there any way to make it safe to pass by
> reference even with IPC or no?

I used a weakPtr. I think this should be safe.

[Alex Christensen]

Comment on attachment 416699 [details]
Patch

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

> Source/WebCore/page/Quirks.cpp:971
> +static bool isStorageAccessQuirkDomainElementPair(const URL& url, const Element& element)

I'm not sure "Pair" is a good thing to have in this name.

> Source/WebCore/page/Quirks.cpp:1050
> +    auto domainInNeedOfStorageAccess = RegistrableDomain(*domainsInNeedOfStorageAccess.value().begin().get());

We should probably check if begin().get() is null, which could happen if value() returns an empty container.

> Source/WebCore/platform/network/NetworkStorageSession.cpp:390
> +        map.add(RegistrableDomain::uncheckedCreateFromRegistrableDomainString("live.com"),

The HashMap& returned by this function should probably be const.

[Kate Cheney]

Created attachment 416713 [details]
Patch for landing

[Kate Cheney]

(In reply to Alex Christensen from comment #9)
> Comment on attachment 416699 [details]
> Patch
> 

Thanks for the review Alex!

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416699&action=review
> 
> > Source/WebCore/page/Quirks.cpp:971
> > +static bool isStorageAccessQuirkDomainElementPair(const URL& url, const Element& element)
> 
> I'm not sure "Pair" is a good thing to have in this name.
> 

I changed this to isStorageAccessQuirkDomainAndElement()

> > Source/WebCore/page/Quirks.cpp:1050
> > +    auto domainInNeedOfStorageAccess = RegistrableDomain(*domainsInNeedOfStorageAccess.value().begin().get());
> 
> We should probably check if begin().get() is null, which could happen if
> value() returns an empty container.
> 

I think this call will actually crash on an empty container, so I added an empty check before. 

> > Source/WebCore/platform/network/NetworkStorageSession.cpp:390
> > +        map.add(RegistrableDomain::uncheckedCreateFromRegistrableDomainString("live.com"),
> 
> The HashMap& returned by this function should probably be const.

Fixed.

[EWS]

Committed r271074: <https://trac.webkit.org/changeset/271074>

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