Bug 71554 – [Qt] 4 Layout test fail due to network error constant values clash with WebkitError enum values in FrameloaderclientQt

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Deepak Sherveghar]

Following layout test failed because of changeset r99071 Bug: https://bugs.webkit.org/show_bug.cgi?id=62108
1. fast/overflow/overflow-height-float-not-removed-crash.html
2. fast/overflow/overflow-height-float-not-removed-crash3.html
3. fast/css/acid2-pixel.html
4. fast/css/acid2.html

This is due to enum value clash between WebkitError and Qt Network error codes.
This bug was revealed by changeset r99071 Bug=62108.

As of now Csaba Osztrogonac has skipped the failing test case: Changeset = 99168.

[Deepak Sherveghar]

Created attachment 113651 [details]
Patch.

The QNetworkReply::NetworkError constant values clash with enum values for WebKitError's.
Changed the enum values to 1k range. Fixes 4 failing layout test.
unskipped them as well from the platform qt skipped list.

[Simon Hausmann]

Interesting. Can you explain _where_ or how they clash?

[Simon Hausmann]

I see it now, we use QNetworkReply::error() and pass it straight to the ResourceError constructor.

[Simon Hausmann]

However... why do the "clash" matter? The error domain is all different. When we put a QNetworkReply error in the ResourceError, we use a "QtWebKit" domain. When we put the http status code it in, the domain is "HTTP". And similarly when we put one of those errors in there, the domain is something else.

Do you know how the layout test failure relates to that? Where is the error() used and what is it compared to?

[Deepak Sherveghar]

(In reply to comment #4)
> However... why do the "clash" matter? The error domain is all different. When we put a QNetworkReply error in the ResourceError, we use a "QtWebKit" domain. When we put the http status code it in, the domain is "HTTP". And similarly when we put one of those errors in there, the domain is something else.
> 
> Do you know how the layout test failure relates to that? Where is the error() used and what is it compared to?

In my patch for bug:62108, I added a new condition in the method shouldFallBack
bool FrameLoaderClientQt::shouldFallBack(const WebCore::ResourceError& error)
{
 - return !(error.isCancellation() || (error.errorCode() == WebKitErrorFrameLoadInterruptedByPolicyChange));
 + return !(error.isCancellation() || (error.errorCode() == WebKitErrorFrameLoadInterruptedByPolicyChange) || (error.errorCode() == WebKitErrorPluginWillHandleLoad));
}

The layout test fail because of this new condition.

shouldFallBack() tells the DOM if it should attempt to render the next nested fallback content if its parent fails to load.
In the failing layout test, we have an <object> tag with invalid url, expecting the fallback content to be loaded and rendered.
Now the object url fails to load, QNetworkReplyHandler sets the QNetworkReply::NetworkError as 203 (ContentNotFoundError).
Webkit calls shouldFallBack to check if fallback content can be loaded. Now since in shouldFallBack() we only check for error code and not for domains(which we actually should), error code 203 matches with WebKitErrorPluginWillHandleLoad enum value and returns false.

Hence the test case fail because fallback contents are not rendered.

Just did a quick check with the codebase and found that chromium port and webkit2 are the only port that check for domain along with error code.
I think as explained by you, we should also add domain check along with error code in shouldFallBack().
With domain check in place, enum constant value changes would not be required.

I was thinking somewhere along the lines of webkit2, Something like below for shouldFallBack() 

bool FrameLoaderClientQt::shouldFallBack(const WebCore::ResourceError& error)
{
   ResourceError cancelledError = cancelledError(ResourceRequest());

   if (error.errorCode() == cancelledError.errorCode() && error.domain() == cancelledError.domain())
        return false;

   if (error.errorCode() == WebKitErrorFrameLoadInterruptedByPolicyChange  && error.domain() == String("WebKitErrorDomain"))
        return false;

   if (error.errorCode() == WebKitErrorPlugInWillHandleLoad  && error.domain() == String("WebKit"))
        return false;

   return true;
}

Does that seem reasonable ???

[Simon Hausmann]

(In reply to comment #5)
> (In reply to comment #4)
> > However... why do the "clash" matter? The error domain is all different. When we put a QNetworkReply error in the ResourceError, we use a "QtWebKit" domain. When we put the http status code it in, the domain is "HTTP". And similarly when we put one of those errors in there, the domain is something else.
> > 
> > Do you know how the layout test failure relates to that? Where is the error() used and what is it compared to?
> 
> In my patch for bug:62108, I added a new condition in the method shouldFallBack
> bool FrameLoaderClientQt::shouldFallBack(const WebCore::ResourceError& error)
> {
>  - return !(error.isCancellation() || (error.errorCode() == WebKitErrorFrameLoadInterruptedByPolicyChange));
>  + return !(error.isCancellation() || (error.errorCode() == WebKitErrorFrameLoadInterruptedByPolicyChange) || (error.errorCode() == WebKitErrorPluginWillHandleLoad));
> }
> 
> The layout test fail because of this new condition.

Ahh, I see. Thanks for the explanation :)

> shouldFallBack() tells the DOM if it should attempt to render the next nested fallback content if its parent fails to load.
> In the failing layout test, we have an <object> tag with invalid url, expecting the fallback content to be loaded and rendered.
> Now the object url fails to load, QNetworkReplyHandler sets the QNetworkReply::NetworkError as 203 (ContentNotFoundError).
> Webkit calls shouldFallBack to check if fallback content can be loaded. Now since in shouldFallBack() we only check for error code and not for domains(which we actually should), error code 203 matches with WebKitErrorPluginWillHandleLoad enum value and returns false.
> 
> Hence the test case fail because fallback contents are not rendered.
> 
> Just did a quick check with the codebase and found that chromium port and webkit2 are the only port that check for domain along with error code.
> I think as explained by you, we should also add domain check along with error code in shouldFallBack().
> With domain check in place, enum constant value changes would not be required.
> 
> I was thinking somewhere along the lines of webkit2, Something like below for shouldFallBack() 
> 
> bool FrameLoaderClientQt::shouldFallBack(const WebCore::ResourceError& error)
> {
>    ResourceError cancelledError = cancelledError(ResourceRequest());
> 
>    if (error.errorCode() == cancelledError.errorCode() && error.domain() == cancelledError.domain())
>         return false;
> 
>    if (error.errorCode() == WebKitErrorFrameLoadInterruptedByPolicyChange  && error.domain() == String("WebKitErrorDomain"))
>         return false;
> 
>    if (error.errorCode() == WebKitErrorPlugInWillHandleLoad  && error.domain() == String("WebKit"))
>         return false;
> 
>    return true;
> }
> 
> Does that seem reasonable ???

Yes, I think that's the correct fix.

[Simon Hausmann]

Comment on attachment 113651 [details]
Patch.

Taking this patch out of the review queue. The right fix is to also check the error domain when checking the error code, as discussed/proposed :)

[Jesus Sanchez-Palencia]

(In reply to comment #6)
> > Does that seem reasonable ???
> 
> Yes, I think that's the correct fix.

+1 as it goes along with the current WK2 implementation.

On a side note, the only thing that bothers me is using this strings for error domains (i.e. "WebKit", "HTTP", etc) everywhere. Perhaps we should concentrate static functions somewhere together with the error code enums and use them in both WK1 and WK2 implementations, pretty much everywhere we create or check a ResourceError. But this is another discussion and another patch, not needed for this bug.

[Deepak Sherveghar]

Created attachment 114238 [details]
Updated Patch

[Simon Hausmann]

Comment on attachment 114238 [details]
Updated Patch

The patch looks good (the right fix), but I'd prefer to replace the use of DEFINE_STATIC_LOCAL with a straight comparision of the domains as string literals.

The effort of unifying the string literals of the domains to a central place can be a separate patch.

So r- for the DEFINE_STATIC_LOCAL, but otherwise I really appreciate that you're fixing this rather nasty issue that is hard to find!

[Simon Hausmann]

Comment on attachment 114238 [details]
Updated Patch

I changed my mind, I see that it's done like what in WebFrameLoaderClient, too.

[WebKit Review Bot]

Comment on attachment 114238 [details]
Updated Patch

Clearing flags on attachment: 114238

Committed r99699: <http://trac.webkit.org/changeset/99699>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.