Bug 98527 - REGRESSION (r129478-r129480): http/tests/loading/text-content-type-with-binary-extension.html failing on Apple MountainLion Debug WK2 (Tests)
Summary: REGRESSION (r129478-r129480): http/tests/loading/text-content-type-with-binar...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mikhail Pozdnyakov
URL: http://build.webkit.org/results/Apple...
Keywords: LayoutTestFailure, MakingBotsRed, Regression
: 98994 (view as bug list)
Depends on: 95974 99152
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-05 08:59 PDT by Jer Noble
Modified: 2013-02-04 16:44 PST (History)
7 users (show)

See Also:


Attachments
patch (9.08 KB, patch)
2012-10-11 02:00 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch v2 (7.64 KB, patch)
2012-10-11 05:34 PDT, Mikhail Pozdnyakov
kenneth: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
to be landed (7.69 KB, patch)
2012-10-11 06:28 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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