Bug 98527

Summary: REGRESSION (r129478-r129480): http/tests/loading/text-content-type-with-binary-extension.html failing on Apple MountainLion Debug WK2 (Tests)
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: Tools / TestsAssignee: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, jberlin, kenneth, mikhail.pozdnyakov, ossy, tonikitoo, webkit.review.bot
Priority: P2 Keywords: LayoutTestFailure, MakingBotsRed, Regression
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK2%20(Tests)/r129480%20(1445)/results.html
Bug Depends on: 95974, 99152    
Bug Blocks:    
Attachments:
Description Flags
patch
none
patch v2
kenneth: review+, webkit.review.bot: commit-queue-
to be landed none

Comment 1 Kenneth Rohde Christiansen 2012-10-05 09:02:51 PDT
Misha can you please have a look at this?
Comment 2 Jer Noble 2012-10-05 11:10:28 PDT
It looks like the patch in question is insufficient.  After the web process returns "pass through" in response to InjectedBundlePage::decidePolicyForResponse(), it then queries the UIProcess through InjectedBundlePage::decidePolicyForResponse().  Because WKTR has not installed a WKPagePolicyClient, it defaults to "use", and the page loads the binary data.
Comment 3 Jer Noble 2012-10-05 11:41:00 PDT
Added this test to TestExpectations as Failing in r130536.
Comment 4 Mikhail Pozdnyakov 2012-10-09 04:41:26 PDT
Think the described problem can be fixed with the same patch with bug#95974
Comment 5 Mikhail Pozdnyakov 2012-10-09 06:35:39 PDT
(In reply to comment #4)
> Think the described problem can be fixed with the same patch with bug#95974

Just uploaded patch for bug#95974 that contains partial fix for this bug -- it's introducing PagePolicyClient for WTR. Should also add decidePolicyForResponse callback to fix this bug problem.
Comment 6 Mikhail Pozdnyakov 2012-10-11 02:00:50 PDT
Created attachment 168173 [details]
patch
Comment 7 Mikhail Pozdnyakov 2012-10-11 02:02:09 PDT
*** Bug 98994 has been marked as a duplicate of this bug. ***
Comment 8 Kenneth Rohde Christiansen 2012-10-11 04:05:22 PDT
Comment on attachment 168173 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168173&action=review

> LayoutTests/platform/efl-wk2/http/tests/loading/text-content-type-with-binary-extension-expected.txt:10
> +Binary data not loaded as text. Maybe PASS.

Maybe PASS?

> Tools/WebKitTestRunner/TestController.cpp:1081
> +    if (WKURLResponseHTTPStatusCode(response) == 204) {

We have defines for these numbers somewhere in webkit!

> Tools/WebKitTestRunner/TestController.cpp:1087
> +    // If the URL Response has "Content-Disposition: attachment;" header, then
> +    // we should download it.

keep on one line

> Tools/WebKitTestRunner/TestController.cpp:1108
> +    // We should ignore downloadable top-level content for subframes, with an exception for text/xml and application/xml so we can still support Acid3 test.
> +    // It makes the browser intentionally behave differently when it comes to text(application)/xml content in subframes vs. mainframe.

the browser? this is the test system, is this behavior even tested?
Comment 9 Mikhail Pozdnyakov 2012-10-11 04:21:26 PDT
Comment on attachment 168173 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168173&action=review

>> LayoutTests/platform/efl-wk2/http/tests/loading/text-content-type-with-binary-extension-expected.txt:10
>> +Binary data not loaded as text. Maybe PASS.
> 
> Maybe PASS?

That is test's expected output :)

>> Tools/WebKitTestRunner/TestController.cpp:1081
>> +    if (WKURLResponseHTTPStatusCode(response) == 204) {
> 
> We have defines for these numbers somewhere in webkit!

in WebCore.. WebCore/HTTPStatusCodes.h. Are we supposed to use WebCore data types in WTR?

>> Tools/WebKitTestRunner/TestController.cpp:1087
>> +    // we should download it.
> 
> keep on one line

sure.

>> Tools/WebKitTestRunner/TestController.cpp:1108
>> +    // It makes the browser intentionally behave differently when it comes to text(application)/xml content in subframes vs. mainframe.
> 
> the browser? this is the test system, is this behavior even tested?

Checking which layout tests should cover it..

Thanks for having a look!
Comment 10 Mikhail Pozdnyakov 2012-10-11 05:33:04 PDT
> > +    // It makes the browser intentionally behave differently when it comes to text(application)/xml content in subframes vs. mainframe.
> 
> the browser? this is the test system, is this behavior even tested?

Furthermore, the response has been already handled by WKBundlePagePolicyClient so no need in that complicetd logic here simple ignoring should be enough.
Comment 11 Mikhail Pozdnyakov 2012-10-11 05:34:04 PDT
Created attachment 168200 [details]
patch v2

Simplified TestController::decidePolicyForResponse().
Comment 12 WebKit Review Bot 2012-10-11 06:04:36 PDT
Comment on attachment 168200 [details]
patch v2

Rejecting attachment 168200 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
Kit/chromium/third_party/yasm/source/patched-yasm --revision 154708 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
51>At revision 154708.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/14262134
Comment 13 Mikhail Pozdnyakov 2012-10-11 06:28:28 PDT
Created attachment 168211 [details]
to be landed
Comment 14 WebKit Review Bot 2012-10-11 07:24:37 PDT
Comment on attachment 168211 [details]
to be landed

Clearing flags on attachment: 168211

Committed r131057: <http://trac.webkit.org/changeset/131057>
Comment 15 WebKit Review Bot 2012-10-11 07:24:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Csaba Osztrogon√°c 2012-10-12 01:34:43 PDT
(In reply to comment #14)
> (From update of attachment 168211 [details])
> Clearing flags on attachment: 168211
> 
> Committed r131057: <http://trac.webkit.org/changeset/131057>

It caused a regression on Qt-WK2 - https://bugs.webkit.org/show_bug.cgi?id=99152
Comment 17 Jessie Berlin 2013-02-04 16:32:53 PST
http/tests/loading/text-content-type-with-binary-extension.htm is showing up as an unexpected pass on the Mac WK2 bots. I am going to remove the failure test expectation for mac-wk2.
Comment 18 Jessie Berlin 2013-02-04 16:44:36 PST
Remove the failing expectation for mac-wk2 in http://trac.webkit.org/changeset/141831