Bug 95974 – [WK2][WTR] WebKitTestRunner UI process should be aware of Custom Policy Delegate

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Mikhail Pozdnyakov]

Currently WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction considers only 'WKBundlePagePolicyActionUse' policy action returned by the
InjectedBundle and skips the 'WKBundlePagePolicyActionPassThrough' result. It's leading to fast/loader/onload-policy-ignore-for-frame.html test failure.

The reason for it is apparently that 'WKBundlePagePolicyActionPassThrough' is returned also from InjectedBundlePagePolicyClient when there is no 
WKBundlePagePolicyClient callback provided. So we do not distinguish between two cases: 
1) there's no WKBundlePagePolicyClient decidePolicyForNavigationAction callback provided 
2) WKBundlePagePolicyClient decidePolicyForNavigationAction callback returns 'WKBundlePagePolicyActionPassThrough' value

[Mikhail Pozdnyakov]

> It's leading to fast/loader/onload-policy-ignore-for-frame.html test failure.

Actually fast/loader/onload-policy-ignore-for-frame.html will need also bug42546 to be fixed.

[Mikhail Pozdnyakov]

Created attachment 162480 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 162480 [details]
patch

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

Where is the unskipped test?

> Source/WebKit2/ChangeLog:11
> +        Before the change WebFrameLoaderClient::dispatchDecidePolicy callbacks considered only 'WKBundlePagePolicyActionUse' policy action returned by the
> +        InjectedBundle and skipped the 'WKBundlePagePolicyActionPassThrough' affecting fast/loader/onload-policy-ignore-for-frame.html test output.
> +
> +        The reason for it was apparently that 'WKBundlePagePolicyActionPassThrough' was returned also from InjectedBundlePagePolicyClient when there was no 

I would wrap this a bit...

[Mikhail Pozdnyakov]

(In reply to comment #3)
> (From update of attachment 162480 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162480&action=review
> 
> Where is the unskipped test?
> 
test fast/loader/onload-policy-ignore-for-frame.html will be unskipped 
in bug42546 patch, I'm working at it at the moment.
> 
> I would wrap this a bit...
Could you please clarify?

[Alexey Proskuryakov]

Comment on attachment 162480 [details]
patch

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

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePagePolicyClient.cpp:40
> +bool InjectedBundlePagePolicyClient::decidePolicyForNavigationAction(WebPage* page, WebFrame* frame, InjectedBundleNavigationAction* action, const ResourceRequest& resourceRequest, WKBundlePagePolicyAction& result, RefPtr<APIObject>& userData)
>  {
>      if (!m_client.decidePolicyForNavigationAction)
> -        return WKBundlePagePolicyActionPassThrough;
> +        return false;

This doesn't really look right to me. What's the caller to do with two separate results - a boolean and a WKBundlePagePolicyAction? Calling decidePolicyForNavigationAction must result in a decision, not in an array of vaguely defined data points the caller is advised to make its own decision from.

[Mikhail Pozdnyakov]

(In reply to comment #5)
> (From update of attachment 162480 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162480&action=review
> 
> > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePagePolicyClient.cpp:40
> > +bool InjectedBundlePagePolicyClient::decidePolicyForNavigationAction(WebPage* page, WebFrame* frame, InjectedBundleNavigationAction* action, const ResourceRequest& resourceRequest, WKBundlePagePolicyAction& result, RefPtr<APIObject>& userData)
> >  {
> >      if (!m_client.decidePolicyForNavigationAction)
> > -        return WKBundlePagePolicyActionPassThrough;
> > +        return false;
> 
> This doesn't really look right to me. What's the caller to do with two separate results - a boolean and a WKBundlePagePolicyAction? Calling decidePolicyForNavigationAction must result in a decision, not in an array of vaguely defined data points the caller is advised to make its own decision from.

boolean result just shows who actually should make the decision: injected bundle or UI process. If returns 'true' decision is taken from injected bundle otherwise from UI process.

[Alexey Proskuryakov]

Yes, this is exactly what looks not quite right to me.

[Sam Weinig]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 162480 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=162480&action=review
> > 
> > > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePagePolicyClient.cpp:40
> > > +bool InjectedBundlePagePolicyClient::decidePolicyForNavigationAction(WebPage* page, WebFrame* frame, InjectedBundleNavigationAction* action, const ResourceRequest& resourceRequest, WKBundlePagePolicyAction& result, RefPtr<APIObject>& userData)
> > >  {
> > >      if (!m_client.decidePolicyForNavigationAction)
> > > -        return WKBundlePagePolicyActionPassThrough;
> > > +        return false;
> > 
> > This doesn't really look right to me. What's the caller to do with two separate results - a boolean and a WKBundlePagePolicyAction? Calling decidePolicyForNavigationAction must result in a decision, not in an array of vaguely defined data points the caller is advised to make its own decision from.
> 
> boolean result just shows who actually should make the decision: injected bundle or UI process. If returns 'true' decision is taken from injected bundle otherwise from UI process.

That is what pass through is supposed to mean.

[Mikhail Pozdnyakov]

> > boolean result just shows who actually should make the decision: injected bundle or UI process. If returns 'true' decision is taken from injected bundle otherwise from UI process.
> 
> That is what pass through is supposed to mean.
Yes, currently 'pass through' from injected bundle means that injected bundle 
does not make decision and should be ignored. But in some cases this 'pass through' from injected bundle should be applied to frame loader, and 
fast/loader/onload-policy-ignore-for-frame.html, for instance, relies on it.

[Mikhail Pozdnyakov]

(In reply to comment #7)
> Yes, this is exactly what looks not quite right to me.

Yeah, my current makes the InjectedBundlePagePolicyClient interface weird, will try to find better solution.

[Mikhail Pozdnyakov]

Created attachment 162737 [details]
patch v2

The proposed solution adds new method 'isNull()' to the base APIClient class. This method returns 'true' if
API client was not initialized and returns 'false' otherwise. This method is then
used to detect who should make the policy decision in WebFrameLoaderClient (bundle client if WKBundlePagePolicyClient
is initialized and UIProcess otherwise).

Also ChangeLog description improved.

[Mikhail Pozdnyakov]

Created attachment 162744 [details]
patch v3

Found a problem in my previous patch: m_isNull was not reset in initialize() method. Fixed now.

[Mikhail Pozdnyakov]

Alexey, Sam could you please take a look?

[Alexey Proskuryakov]

Sam or Anders should review this.

[Anders Carlsson]

Comment on attachment 162744 [details]
patch v3

I don't think this is correct. "Pass through" isn't the same thing as ignore. Due to the way state is managed in the UI process, it's not possible to ignore a load from the bundle.

[Mikhail Pozdnyakov]

(In reply to comment #15)
> (From update of attachment 162744 [details])
> I don't think this is correct. "Pass through" isn't the same thing as ignore. Due to the way state is managed in the UI process, it's not possible to ignore a load from the bundle.

Thanks for taking a look!
Yeah, my problem was probably that I treated "Pass through" as "Ignore".
I guess proper fix should add "Ignore" policy for bundle client.

[Mikhail Pozdnyakov]

Created attachment 166423 [details]
patch v4

Added WKBundlePagePolicyActionIgnore policy to be used by bundle client.

[Anders Carlsson]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 162744 [details] [details])
> > I don't think this is correct. "Pass through" isn't the same thing as ignore. Due to the way state is managed in the UI process, it's not possible to ignore a load from the bundle.
> 
> Thanks for taking a look!
> Yeah, my problem was probably that I treated "Pass through" as "Ignore".
> I guess proper fix should add "Ignore" policy for bundle client.

Like I said, if we add an Ignore policy things get out of sync in the UI process.

[Brady Eidson]

(In reply to comment #18)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > (From update of attachment 162744 [details] [details] [details])
> > > I don't think this is correct. "Pass through" isn't the same thing as ignore. Due to the way state is managed in the UI process, it's not possible to ignore a load from the bundle.
> > 
> > Thanks for taking a look!
> > Yeah, my problem was probably that I treated "Pass through" as "Ignore".
> > I guess proper fix should add "Ignore" policy for bundle client.
> 
> Like I said, if we add an Ignore policy things get out of sync in the UI process.

I'll add more to what Anders is saying.

What we have here is a failing layout test.  To fix it we should not add Ignore to the WebProcess.  Instead we should properly implement Ignore support in the UIProcess of WebKitTestRunner.

[Mikhail Pozdnyakov]

(In reply to comment #19)
> I'll add more to what Anders is saying.
> 
> What we have here is a failing layout test.  To fix it we should not add Ignore to the WebProcess.  Instead we should properly implement Ignore support in the UIProcess of WebKitTestRunner.

Thank you very much indeed for the hint! 
Changing the bug title accordingly.

[Mikhail Pozdnyakov]

Created attachment 167283 [details]
patch v5

WTR bundle client notifies UI process about Custom Policy Delegate via newly added message, so that TestController is aware of whether Custom Policy Delegate as enabled and permissive. This data is used then in PlatformWebViewEfl.cpp to unskip
fast/loader/onload-policy-ignore-for-frame.html for EFL WK2.

[Brady Eidson]

Comment on attachment 167283 [details]
patch v5

I - admittedly - haven't looked at WKTR to get the answer to this myself.  But could this not be done in a cross platform way?  This policy delegate isn't really about platform specific ports so much as it is the cross platform UI Process of WKTR itself.

[Mikhail Pozdnyakov]

(In reply to comment #22)
> (From update of attachment 167283 [details])
> I - admittedly - haven't looked at WKTR to get the answer to this myself.  But could this not be done in a cross platform way?  This policy delegate isn't really about platform specific ports so much as it is the cross platform UI Process of WKTR itself.

It is possible to make solution cross platform if WTR Test controller sets PagePolicyClient. The reason why I did not do it from the beginning is that PagePolicyClient is usually set in port implementation and it has some logic that won't be tested if PagePolicyClient is reset by WTR.
However I see that other clients (WKPageUIClient, WKPageLoaderClient) are reset in WTR and probably PagePolicyClient should not be exception. Obviously port-specific logic is supposed to be tested by port-specific API unit tests rather than by WTR. So I will change my solution. Thanks for having a look!

[Mikhail Pozdnyakov]

Created attachment 167748 [details]
patch v6

Added PagePolicyClient to WTR so that solution applies to all the ports.

[Mikhail Pozdnyakov]

Could please anyone review?

[WebKit Review Bot]

Comment on attachment 167748 [details]
patch v6

Clearing flags on attachment: 167748

Committed r130967: <http://trac.webkit.org/changeset/130967>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #26)
> (From update of attachment 167748 [details])
> Clearing flags on attachment: 167748
> 
> Committed r130967: <http://trac.webkit.org/changeset/130967>

It caused a regression on Qt-WK2 - https://bugs.webkit.org/show_bug.cgi?id=98994
Could you check and fix it, please?