Bug 219949 – Cannot see 'Add to my stations' button in BBC World service Radio

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

BBC uses the presence of third party cookies to determine whether to show specific elements in their radio player. These do not appear with ITP and full third party cookie blocking.

[Kate Cheney]

<rdar://problem/60319532>

[Kate Cheney]

Created attachment 416350 [details]
Patch

[Alex Christensen]

Comment on attachment 416350 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:2724
> +    if (m_frame.document()) {

if (auto* document = m_frame.document())
That'll allow you to use it in the next two lines.

> Source/WebCore/loader/FrameLoader.cpp:2726
> +        if (m_frame.document()->settings().needsSiteSpecificQuirks()) {
> +            if (m_frame.document()->url().string() == Quirks::staticRadioPlayerURLString() && m_client->BBCRadioPlayerPopup()) {

This could be another && instead of two if statements.

> Source/WebCore/loader/FrameLoaderClient.h:385
> +    virtual bool BBCRadioPlayerPopup() const { return false; };

This probably shouldn't be here, but definitely needs a better name indicating its use.

> Source/WebCore/page/DOMWindow.cpp:2550
> +        frame->setBBCRadioPlayerPopup(true);

We should probably add a null check for frame after it's declared and return nullptr if it's null instead of crashing.  There was a code path that used it unchecked, but we can fix that one too.

> Source/WebCore/page/Frame.h:385
> +    bool m_BBCRadioPlayerPopup { false };

I don't think we should introduce a boolean for this because that will likely lead to cases where it's not reset properly.  If we do have to add one, it should have a prefix like isShowing or something, but I don't think it should be necessary here or on WebPage, and there certainly shouldn't be two places that remember that we are doing the bbc radio quirk.

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

document is a weak pointer.  It should be checked before dereferencing.

> Source/WebCore/page/Quirks.cpp:1118
> +                                ExceptionOr<RefPtr<WindowProxy>> proxyOrException =  domWindow->open(*domWindow, *domWindow, staticRadioPlayerURLString(), emptyString(), BBCRadioPlayerPopUpWindowFeatureString);

auto
extra space.
Should we do anything with proxyOrException?

> Source/WebKit/UIProcess/WebPageProxy.cpp:5582
> +    if (request.url().string() == "https://static.radioplayer.co.uk/")

Quirks::staticRadioPlayerURLString

[Kate Cheney]

Comment on attachment 416350 [details]
Patch

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

Thanks for the comments Alex

>> Source/WebCore/loader/FrameLoader.cpp:2724
>> +    if (m_frame.document()) {
> 
> if (auto* document = m_frame.document())
> That'll allow you to use it in the next two lines.

Will change.

>> Source/WebCore/loader/FrameLoader.cpp:2726
>> +            if (m_frame.document()->url().string() == Quirks::staticRadioPlayerURLString() && m_client->BBCRadioPlayerPopup()) {
> 
> This could be another && instead of two if statements.

Will change.

>> Source/WebCore/page/DOMWindow.cpp:2550
>> +        frame->setBBCRadioPlayerPopup(true);
> 
> We should probably add a null check for frame after it's declared and return nullptr if it's null instead of crashing.  There was a code path that used it unchecked, but we can fix that one too.

Good idea, I will add that.

>> Source/WebCore/page/Frame.h:385
>> +    bool m_BBCRadioPlayerPopup { false };
> 
> I don't think we should introduce a boolean for this because that will likely lead to cases where it's not reset properly.  If we do have to add one, it should have a prefix like isShowing or something, but I don't think it should be necessary here or on WebPage, and there certainly shouldn't be two places that remember that we are doing the bbc radio quirk.

My intention was to prevent any other page from being automatically redirected if navigating to https://static.radio.player.com, which would happen if I didn't include an indication that the pop-up had been triggered. I am not sure how much that should be a concern (https://static.radio.player.com is just a blank page).

Do you know if there is a way to wait for a page to finish loading from the site in Quirks::triggerOptionalStorageAccessQuirk where I call domWindow->open() to perform the redirect from there? That way we know the redirect only happens for the quirk.

>> Source/WebCore/page/Quirks.cpp:1116
>> +                            auto domWindow = document->domWindow();
> 
> document is a weak pointer.  It should be checked before dereferencing.

Will add this.

>> Source/WebCore/page/Quirks.cpp:1118
>> +                                ExceptionOr<RefPtr<WindowProxy>> proxyOrException =  domWindow->open(*domWindow, *domWindow, staticRadioPlayerURLString(), emptyString(), BBCRadioPlayerPopUpWindowFeatureString);
> 
> auto
> extra space.
> Should we do anything with proxyOrException?

Maybe if we are able to handle the redirect from here instead of in Frame.cpp. Then I can use proxyOrException to perform a redirect.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:5582
>> +    if (request.url().string() == "https://static.radioplayer.co.uk/")
> 
> Quirks::staticRadioPlayerURLString

Good catch, will change.

[Kate Cheney]

Created attachment 416475 [details]
Patch

[Kate Cheney]

Comment on attachment 416350 [details]
Patch

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

>>> Source/WebCore/page/Frame.h:385
>>> +    bool m_BBCRadioPlayerPopup { false };
>> 
>> I don't think we should introduce a boolean for this because that will likely lead to cases where it's not reset properly.  If we do have to add one, it should have a prefix like isShowing or something, but I don't think it should be necessary here or on WebPage, and there certainly shouldn't be two places that remember that we are doing the bbc radio quirk.
> 
> My intention was to prevent any other page from being automatically redirected if navigating to https://static.radio.player.com, which would happen if I didn't include an indication that the pop-up had been triggered. I am not sure how much that should be a concern (https://static.radio.player.com is just a blank page).
> 
> Do you know if there is a way to wait for a page to finish loading from the site in Quirks::triggerOptionalStorageAccessQuirk where I call domWindow->open() to perform the redirect from there? That way we know the redirect only happens for the quirk.

In the latest patch I inject javascript on load when I create the popup to solve this.

[Kate Cheney]

Created attachment 416522 [details]
Patch

[Alex Christensen]

Comment on attachment 416522 [details]
Patch

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

> Source/WebCore/page/DOMWindow.cpp:2505
> +        if (document->settings().needsSiteSpecificQuirks() && urlStringToOpen == Quirks::BBCRadioPlayerURLString()) {

It looks like you'll need to open Quirks.h to fix the windows build.

> Source/WebCore/page/Quirks.cpp:1155
> +            if (auto subResourceDomainsInNeedOfStorageAccess = NetworkStorageSession::subResourceDomainsInNeedOfStorageAccessForFirstParty(firstPartyDomain)) {

We tend to avoid deeply nested code like this by adding early returns.
auto subResourceDomainsInNeedOfStorageAccess = ;
if (!subResourceDomainsInNeedOfStorageAccess)
    return StorageAccessResult::ShouldNotCancelEvent;
if (storageAccessGranted != StorageAccessWasGranted::Yes)
    return StorageAccessResult::ShouldNotCancelEvent;
etc.

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

I think there's a version of is that takes a pointer and returns false if it's null.

[Kate Cheney]

Created attachment 416620 [details]
Patch

[Kate Cheney]

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

Thanks for the review!

> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416522&action=review
> 
> > Source/WebCore/page/DOMWindow.cpp:2505
> > +        if (document->settings().needsSiteSpecificQuirks() && urlStringToOpen == Quirks::BBCRadioPlayerURLString()) {
> 
> It looks like you'll need to open Quirks.h to fix the windows build.
> 

I think my latest patch fixed it. I will wait for EWS before landing.

> > Source/WebCore/page/Quirks.cpp:1155
> > +            if (auto subResourceDomainsInNeedOfStorageAccess = NetworkStorageSession::subResourceDomainsInNeedOfStorageAccessForFirstParty(firstPartyDomain)) {
> 
> We tend to avoid deeply nested code like this by adding early returns.
> auto subResourceDomainsInNeedOfStorageAccess = ;
> if (!subResourceDomainsInNeedOfStorageAccess)
>     return StorageAccessResult::ShouldNotCancelEvent;
> if (storageAccessGranted != StorageAccessWasGranted::Yes)
>     return StorageAccessResult::ShouldNotCancelEvent;
> etc.

I adjusted this to use early returns.

> 
> > Source/WebCore/page/Quirks.cpp:1168
> > +                                    if (abstractFrame && is<Frame>(*abstractFrame)) {
> 
> I think there's a version of is that takes a pointer and returns false if
> it's null.

I removed the check for abstractFrame.

[EWS]

Committed r271059: <https://trac.webkit.org/changeset/271059>

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

[Radar WebKit Bug Importer]

<rdar://problem/72595090>