Bug 24260 - Loading progress for blank views
Summary: Loading progress for blank views
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Major
Assignee: Christian Dywan
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2009-02-28 06:09 PST by Christian Dywan
Modified: 2017-03-11 10:45 PST (History)
3 users (show)

See Also:


Attachments
Load test with debugging output about loading signals (3.04 KB, patch)
2009-02-28 07:23 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
This corrects mime and navigation handling (1.18 KB, patch)
2009-02-28 07:32 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
This corrects mime and navigation handling #2 (3.25 KB, patch)
2009-03-06 12:05 PST, Christian Dywan
gns: review-
Details | Formatted Diff | Diff
Load test (4.46 KB, patch)
2009-03-25 17:58 PDT, Christian Dywan
no flags Details | Formatted Diff | Diff
possible fix for policy crash (1.36 KB, patch)
2009-03-25 18:14 PDT, Christian Dywan
gns: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Dywan 2009-02-28 06:09:56 PST
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.
Comment 1 Christian Dywan 2009-02-28 07:23:29 PST
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.
Comment 2 Christian Dywan 2009-02-28 07:32:49 PST
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.
Comment 3 Xan Lopez 2009-02-28 07:41:26 PST
(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!
Comment 4 Christian Dywan 2009-03-06 12:05:04 PST
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.
Comment 5 Holger Freyther 2009-03-07 04:31:49 PST
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).
Comment 6 Holger Freyther 2009-03-07 04:54:05 PST
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.
Comment 7 Holger Freyther 2009-03-07 04:54:42 PST
Oh an rs=me on using webkit_web_policy...
Comment 8 Xan Lopez 2009-03-15 09:30:33 PDT
(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? 
Comment 9 Christian Dywan 2009-03-25 16:14:11 PDT
(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.
Comment 10 Christian Dywan 2009-03-25 17:58:04 PDT
Created attachment 28956 [details]
Load test
Comment 11 Christian Dywan 2009-03-25 18:04:47 PDT
(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.

Comment 12 Christian Dywan 2009-03-25 18:14:51 PDT
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.
Comment 13 Gustavo Noronha (kov) 2009-05-04 16:23:09 PDT
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.
Comment 14 Gustavo Noronha (kov) 2009-05-04 16:30:35 PDT
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.
Comment 15 Alexander Butenko 2009-06-22 20:28:35 PDT
seems already fixed now? Can we close?