Bug 71554 - [Qt] 4 Layout test fail due to network error constant values clash with WebkitError enum values in FrameloaderclientQt
Summary: [Qt] 4 Layout test fail due to network error constant values clash with Webki...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2011-11-04 05:07 PDT by Deepak Sherveghar
Modified: 2011-11-09 06:01 PST (History)
4 users (show)

See Also:


Attachments
Patch. (4.05 KB, patch)
2011-11-04 05:28 PDT, Deepak Sherveghar
hausmann: review-
hausmann: commit-queue-
Details | Formatted Diff | Diff
Updated Patch (3.85 KB, patch)
2011-11-09 03:43 PST, Deepak Sherveghar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Deepak Sherveghar 2011-11-04 05:07:32 PDT
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.
Comment 1 Deepak Sherveghar 2011-11-04 05:28:06 PDT
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.
Comment 2 Simon Hausmann 2011-11-04 05:55:13 PDT
Interesting. Can you explain _where_ or how they clash?
Comment 3 Simon Hausmann 2011-11-04 06:16:15 PDT
I see it now, we use QNetworkReply::error() and pass it straight to the ResourceError constructor.
Comment 4 Simon Hausmann 2011-11-04 06:54:08 PDT
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?
Comment 5 Deepak Sherveghar 2011-11-04 11:51:01 PDT
(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 ???
Comment 6 Simon Hausmann 2011-11-04 12:15:33 PDT
(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.
Comment 7 Simon Hausmann 2011-11-04 12:16:16 PDT
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 :)
Comment 8 Jesus Sanchez-Palencia 2011-11-08 10:37:10 PST
(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.
Comment 9 Deepak Sherveghar 2011-11-09 03:43:13 PST
Created attachment 114238 [details]
Updated Patch
Comment 10 Simon Hausmann 2011-11-09 05:20:08 PST
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!
Comment 11 Simon Hausmann 2011-11-09 05:45:54 PST
Comment on attachment 114238 [details]
Updated Patch

I changed my mind, I see that it's done like what in WebFrameLoaderClient, too.
Comment 12 WebKit Review Bot 2011-11-09 06:01:07 PST
Comment on attachment 114238 [details]
Updated Patch

Clearing flags on attachment: 114238

Committed r99699: <http://trac.webkit.org/changeset/99699>
Comment 13 WebKit Review Bot 2011-11-09 06:01:12 PST
All reviewed patches have been landed.  Closing bug.