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.
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.
Interesting. Can you explain _where_ or how they clash?
I see it now, we use QNetworkReply::error() and pass it straight to the ResourceError constructor.
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 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 ???
(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 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 :)
(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.
Created attachment 114238 [details] Updated Patch
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 on attachment 114238 [details] Updated Patch I changed my mind, I see that it's done like what in WebFrameLoaderClient, too.
Comment on attachment 114238 [details] Updated Patch Clearing flags on attachment: 114238 Committed r99699: <http://trac.webkit.org/changeset/99699>
All reviewed patches have been landed. Closing bug.