Bug 24260 – Loading progress for blank views

• Status: ASSIGNED
• Component: WebKitGTK

[Christian Dywan]

Some time ago blank views started to treat progress differently. This causes particularly Midori to show loading progress where none should be.

This is a regression. I will try to come up with a test case.

[Christian Dywan]

Created attachment 28115 [details]
Load test with debugging output about loading signals

So it turns out, the issue is a bit different from what I thought at first.

For one, Midori is returning TRUE where it *should* be FALSE, ie. all is fine, go for it.

The actual bug is that "about:blank" does, unlike any other URI, not report all loading signals IF the return value is TRUE.

TRUE should either always cancel the loading signals OR keep going.

Attached is a patch against the test program (UnitTests) which loads about:blank once and returns TRUE and then again and returns FALSE. It prints all signals to the console so you can see the problem.

[Christian Dywan]

Created attachment 28116 [details]
This corrects mime and navigation handling

Tracked down the cause, we basically don't handle the "handled" case of the mime decision and navigation request. We should be ignoring in that case. The patch does that, it fixes the test.

[Xan Lopez]

(In reply to comment #2)
> Created an attachment (id=28116) [review]
> This corrects mime and navigation handling
> 
> Tracked down the cause, we basically don't handle the "handled" case of the
> mime decision and navigation request. We should be ignoring in that case. The
> patch does that, it fixes the test.
> 

I think we should either use our webkit_web_policy_decision functions everywhere or not, but don't mix them with the low level C++ alternative. Other than that looks great!

[Christian Dywan]

Created attachment 28369 [details]
This corrects mime and navigation handling #2

(In reply to comment #3)
> I think we should either use our webkit_web_policy_decision functions
> everywhere or not, but don't mix them with the low level C++ alternative. Other
> than that looks great!

I agree mixing isn't good for readbility. I chose to use WebKitGTK+ API everywhere now including existing calls, I think it's a little easier on the eyes than the bracket-heavy WebCore version.

[Holger Freyther]

Could you add the test case as well? I think checking/verifying that the callback's has been called would be great as well (and fail when this was not the case).

[Holger Freyther]

Comment on attachment 28369 [details]
This corrects mime and navigation handling #2


> -    if (isHandled)
> +    if (isHandled) {
> +        webkit_web_policy_decision_ignore(m_policyDecision);
>          return;

okay, what happens if _use, _ignore, _download was already called? I think it can cause a crash.

[Holger Freyther]

Oh an rs=me on using webkit_web_policy...

[Xan Lopez]

(In reply to comment #6)
> (From update of attachment 28369 [details] [review])
> 
> > -    if (isHandled)
> > +    if (isHandled) {
> > +        webkit_web_policy_decision_ignore(m_policyDecision);
> >          return;
> 
> okay, what happens if _use, _ignore, _download was already called? I think it
> can cause a crash.
> 

Mmm, indeed. So what should we do here? Modify the policy decision objects so we can check when they have been used (just a simple flag set by all functions), so that we can ignore it if the user didn't do anything? Document that if the signal is handled it's your responsibility to ignore/cancel/use/download the decision? 

[Christian Dywan]

(In reply to comment #8)
> (In reply to comment #6)
> > (From update of attachment 28369 [details] [review] [review])
> > 
> > > -    if (isHandled)
> > > +    if (isHandled) {
> > > +        webkit_web_policy_decision_ignore(m_policyDecision);
> > >          return;
> > 
> > okay, what happens if _use, _ignore, _download was already called? I think it
> > can cause a crash.
> > 
> 
> Mmm, indeed. So what should we do here? Modify the policy decision objects so
> we can check when they have been used (just a simple flag set by all
> functions), so that we can ignore it if the user didn't do anything? Document
> that if the signal is handled it's your responsibility to
> ignore/cancel/use/download the decision? 

Thinking about it again from that point of view, it is probably the best idea to indeed say "if it's handled, one is expected to ignore, download or cancel as appropriate". Starting to work around this feels too messy.

That aside, it should really not crash imho, even if subsequent attempts to make a decision are not supported. I will check that while I update my test case to be automated instead of printf based.

[Christian Dywan]

Created attachment 28956 [details]
Load test

[Christian Dywan]

(In reply to comment #10)
> Created an attachment (id=28956) [review]
> Load test

This updated test is automated, but it fails. There looks to be one more load-started emission in the 'handled' case, no matter whether it does _ignore the decision or not. I'm not quite sure why that is. Even removing the _load_uri from inside the callback doesn't seem to fix that.

As for the crash, yes it does crash, and we should fix that.

[Christian Dywan]

Created attachment 28958 [details]
possible fix for policy crash

This change sets priv->isCancelled in the decision functions, so that subsequent calls won't crash it. We might even want to issue a critical, not sure.

[Gustavo Noronha (kov)]

Comment on attachment 28369 [details]
This corrects mime and navigation handling #2

>      if (!isHandled)
>          webkit_web_policy_decision_use(m_policyDecision);
> +    else
> +        webkit_web_policy_decision_ignore(m_policyDecision);
>  }

As has already been stated, I believe if you handle the signal you must take one of the decisions. Otherwise you should return FALSE. I'd rather improve the docs to say that. Of course we also need to make sure calling them twice will not go boom, though. The other changes look good to me, so I'll say r- to the patch, but rs=me to change the policy function calls for API ones, and add that explanation to the docs.

[Gustavo Noronha (kov)]

Comment on attachment 28958 [details]
possible fix for policy crash

> -    if (!priv->isCancelled)
> +    if (!priv->isCancelled) {
>          (core(priv->frame)->loader()->*(priv->framePolicyFunction))(WebCore::PolicyUse);
> +        priv->isCancelled = TRUE;

This looks very weird to me. Perhaps what we need is another private member that says alreadyDecided? Then if we have alreadyDecided == TRUE we g_warning that a decision has already been implemented, what do you think? I'll say r- for now.

[Alexander Butenko]

seems already fixed now? Can we close?