Bug 37382 – [Qt] XFrameOptions/x-frame-options-deny-meta-tag* tests have extra favicon.ico request

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Robert Hogan]

http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-in-body.html
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-parent-same-origin-deny.html
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag.html

These tests have an extra line in the expected results:
http://127.0.0.1:8000/favicon.ico - willSendRequest <NSURLRequest URL http://127.0.0.1:8000/favicon.ico, main document URL http://127.0.0.1:8000/security/XFrameOptions/x-frame-options-deny-meta-tag-in-body.html, http method GET> redirectResponse (null)

This is probably innocuous?

[Robert Hogan]

Created attachment 55478 [details]
Patch

[Robert Hogan]

Was going to see about supporting dumpDOMAsWebArchive in order to pass LayoutTests/webarchive/test-link-rel-icon.html too but notice that only Mac supports it. Also noticed that Chromium has:


CONSOLE MESSAGE: line 8: Uncaught TypeError: Object [object Object] has no method 'dumpDOMAsWebArchive'
layer at (0,0) size 800x600
  RenderView at (0,0) size 800x600
layer at (0,0) size 800x600
  RenderBlock {HTML} at (0,0) size 800x600
    RenderBody {BODY} at (8,8) size 784x584

as the expected results on Win and Linux for this test. So no intention to support the format it seems.

Should Qt do the same?

[Jakub Wieczorek]

> +        * platform/qt/http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-expected.txt: Added. Qt returns 5 instead of -999.
> +        * platform/qt/http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-in-body-expected.txt: Added. Qt returns 5 instead of -999.
> +        * platform/qt/http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-parent-same-origin-deny-expected.txt: Added. Qt returns 5 instead of -999.

Instead of adding separate test results, you could change FrameLoaderClientQt to rewrite 5 to -999 in the code that prints these callbacks. It should be acceptable as the Windows port seems to be doing a similar thing in their DRT:

// Convert the winsock error code to an NSURLError code.
if (code == WSAEADDRNOTAVAIL)
    code = -1004; // NSURLErrorCannotConnectToHose;

(In reply to comment #2)
> Was going to see about supporting dumpDOMAsWebArchive in order to pass
> LayoutTests/webarchive/test-link-rel-icon.html too but notice that only Mac
> supports it. Also noticed that Chromium has:
> 
> CONSOLE MESSAGE: line 8: Uncaught TypeError: Object [object Object] has no
> method 'dumpDOMAsWebArchive'
> layer at (0,0) size 800x600
>   RenderView at (0,0) size 800x600
> layer at (0,0) size 800x600
>   RenderBlock {HTML} at (0,0) size 800x600
>     RenderBody {BODY} at (8,8) size 784x584
> 
> as the expected results on Win and Linux for this test. So no intention to
> support the format it seems.
> 
> Should Qt do the same?

I don't think there's a point in running tests for a feature that is not implemented and/or completely disabled. On the other hand, it might at least be useful to make sure that they don't crash.

[Robert Hogan]

(In reply to comment #3)
> > +        * platform/qt/http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-expected.txt: Added. Qt returns 5 instead of -999.
> > +        * platform/qt/http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-in-body-expected.txt: Added. Qt returns 5 instead of -999.
> > +        * platform/qt/http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-parent-same-origin-deny-expected.txt: Added. Qt returns 5 instead of -999.
> 
> Instead of adding separate test results, you could change FrameLoaderClientQt to rewrite 5 to -999 in the code that prints these callbacks. It should be acceptable as the Windows port seems to be doing a similar thing in their DRT:
> 
> // Convert the winsock error code to an NSURLError code.
> if (code == WSAEADDRNOTAVAIL)
>     code = -1004; // NSURLErrorCannotConnectToHose;
> 

I think platform-specific results are preferable here. 

> (In reply to comment #2)
> > Was going to see about supporting dumpDOMAsWebArchive in order to pass
> > LayoutTests/webarchive/test-link-rel-icon.html too but notice that only Mac
> > supports it. Also noticed that Chromium has:
> > 
> > CONSOLE MESSAGE: line 8: Uncaught TypeError: Object [object Object] has no
> > method 'dumpDOMAsWebArchive'
> > layer at (0,0) size 800x600
> >   RenderView at (0,0) size 800x600
> > layer at (0,0) size 800x600
> >   RenderBlock {HTML} at (0,0) size 800x600
> >     RenderBody {BODY} at (8,8) size 784x584
> > 
> > as the expected results on Win and Linux for this test. So no intention to
> > support the format it seems.
> > 
> > Should Qt do the same?
> 
> I don't think there's a point in running tests for a feature that is not implemented and/or completely disabled. On the other hand, it might at least be useful to make sure that they don't crash.

Presumably that's where the chromium guys are coming from.

This fix will also unskip LayoutTests/http/tests/misc/favicon-loads-with-images-disabled.html.

[Kenneth Rohde Christiansen]

Comment on attachment 55478 [details]
Patch

r- after talking with Robert on irc.

[Robert Hogan]

Created attachment 56256 [details]
Patch

[Robert Hogan]

Committed r59623: <http://trac.webkit.org/changeset/59623>

[Antonio Gomes]

just a sidenote: it is good when in the  changelog there is a match between the "bug title" and the "fix title"

[Qt] XFrameOptions/x-frame-options-deny-meta-tag* tests have extra favicon.ico request VS [Qt] Disable Icon Database by default in Qt DRT

the later in bugzilla could had catch more attention from others ...

you also did not just disabled IconDatabase but added support for image autoloading ...

(...)
+        [Qt] Disable Icon Database by default in Qt DRT
+
(...)
+
+        https://bugs.webkit.org/show_bug.cgi?id=37382

other than that looks really good to me too

[Robert Hogan]

(In reply to comment #8)
> just a sidenote: it is good when in the  changelog there is a match between the "bug title" and the "fix title"
> 
> [Qt] XFrameOptions/x-frame-options-deny-meta-tag* tests have extra favicon.ico request VS [Qt] Disable Icon Database by default in Qt DRT
> 
> the later in bugzilla could had catch more attention from others ...
> 
> you also did not just disabled IconDatabase but added support for image autoloading ...
> 
> (...)
> +        [Qt] Disable Icon Database by default in Qt DRT
> +
> (...)
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=37382
> 
> other than that looks really good to me too

All points taken. Thanks Antonio!