Silencing console messages during some layout tests.

Silencing console messages during some layout tests.

The console messages emitted during the modified tests were different
with and without --site-per-process flag.  The tests are able to verify
the expected behavior without depending on presence of absence of the
console message, and therefore the console output has been silenced to
unify test output between runs with and without --site-per-process flag.

BUG=617288
Committed: https://crrev.com/85230becec08b0d0c120f700d8372b2ec204aaba
Cr-Commit-Position: refs/heads/master@{#398423}

[Łukasz Anforowicz, 2016-06-06 21:21:00]

lukasza@chromium.org changed reviewers:
+ alexmos@chromium.org

[Łukasz Anforowicz, 2016-06-06 21:21:01]

Alex, can you take a look please?  AFAICT, even without looking at the console
messages, both tests continue to verify that the right navigations took place.

[alexmos, 2016-06-07 00:02:16]

https://codereview.chromium.org/2042043002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/frameNavigation/not-opener-expected.txt
(left):

https://codereview.chromium.org/2042043002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/frameNavigation/not-opener-expected.txt:1:
CONSOLE ERROR: line 4: Unsafe JavaScript attempt to initiate navigation for
frame with URL
'http://127.0.0.1:8000/security/frameNavigation/resources/ready.html' from frame
with URL
'http://localhost:8000/security/frameNavigation/resources/not-opener-helper.html'.
The frame attempting navigation is neither same-origin with the target, nor is
it the target's parent or opener.
Hmm, after looking closer, I think we might need to do a bit more work before
removing this.  The test is currently set up with three windows:
A (not-opener.html) opens A2 (ready.html) and B (not-opener-helper.html).  Then
B tries to navigate A2 to ready.html (confusingly, the page it's already on). 
If the navigation is denied, we should open a new window, which will load
ready.html and send back the "pass" message to not-opener-helper.html (window
B), which triggers cleanup.  If we regress in the future, however, and this
navigation is allowed, we will reload ready.html into A2, which will also send
back the "pass" message to B and perform the same cleanup (except there will be
three windows instead of four, so one of the close() calls will fail - probably
transparently to the test).  So this message was really the only indication that
the navigation failed (unless I missed something?).

Could we add an additional check to not-opener-helper.html that there are four
windows, similarly to what the xss-DENIED-targeted-link-navigation.html test is
already doing?  I think that should give us equivalent coverage.  And/or, check
from window A that A2 didn't navigate (meaning that B will probably need to use
another target URL).

There isn't something like dumpAllOpenedWindowsAsText, is there?

[Łukasz Anforowicz, 2016-06-07 22:54:41]

Thanks Alex - can you take another look please?

https://codereview.chromium.org/2042043002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/frameNavigation/not-opener-expected.txt
(left):

https://codereview.chromium.org/2042043002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/frameNavigation/not-opener-expected.txt:1:
CONSOLE ERROR: line 4: Unsafe JavaScript attempt to initiate navigation for
frame with URL
'http://127.0.0.1:8000/security/frameNavigation/resources/ready.html' from frame
with URL
'http://localhost:8000/security/frameNavigation/resources/not-opener-helper.html'.
The frame attempting navigation is neither same-origin with the target, nor is
it the target's parent or opener.
On 2016/06/07 00:02:15, alexmos wrote:
> Hmm, after looking closer, I think we might need to do a bit more work before
> removing this.  The test is currently set up with three windows:
> A (not-opener.html) opens A2 (ready.html) and B (not-opener-helper.html). 
Then
> B tries to navigate A2 to ready.html (confusingly, the page it's already on). 
> If the navigation is denied, we should open a new window, which will load
> ready.html and send back the "pass" message to not-opener-helper.html (window
> B), which triggers cleanup.  If we regress in the future, however, and this
> navigation is allowed, we will reload ready.html into A2, which will also send
> back the "pass" message to B and perform the same cleanup (except there will
be
> three windows instead of four, so one of the close() calls will fail -
probably
> transparently to the test).  So this message was really the only indication
that
> the navigation failed (unless I missed something?).

Thanks for catching this - much appreciated.

> Could we add an additional check to not-opener-helper.html that there are four
> windows, similarly to what the xss-DENIED-targeted-link-navigation.html test
is
> already doing?  I think that should give us equivalent coverage.  And/or,
check
> from window A that A2 didn't navigate (meaning that B will probably need to
use
> another target URL).

I've added
log("There are currently " + testRunner.windowCount() + " windows opened.");
to not-opener.html

This should be sufficient to verify that the last step you pointed out above has
not reused one of the old windows.
> 
> There isn't something like dumpAllOpenedWindowsAsText, is there?

There actually is testRunner.dumpCreateView which dumps a line whenever
WebViewClient::createView is called during a test, but in case of
not-opener.html this only covers creation of the 2 helper windows at the start
of the test.

[alexmos, 2016-06-07 23:01:53]

LGTM

[Łukasz Anforowicz, 2016-06-07 23:04:35]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-06-07 23:04:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2042043002/20001

[commit-bot: I haz the power, 2016-06-07 23:59:35]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-08 00:00:47]

Description was changed from

==========
Silencing console messages during some layout tests.

The console messages emitted during the modified tests were different
with and without --site-per-process flag.  The tests are able to verify
the expected behavior without depending on presence of absence of the
console message, and therefore the console output has been silenced to
unify test output between runs with and without --site-per-process flag.

BUG=617288
==========

to

==========
Silencing console messages during some layout tests.

The console messages emitted during the modified tests were different
with and without --site-per-process flag.  The tests are able to verify
the expected behavior without depending on presence of absence of the
console message, and therefore the console output has been silenced to
unify test output between runs with and without --site-per-process flag.

BUG=617288

Committed: https://crrev.com/85230becec08b0d0c120f700d8372b2ec204aaba
Cr-Commit-Position: refs/heads/master@{#398423}
==========

[commit-bot: I haz the power, 2016-06-08 00:00:48]

Patchset 2 (id:??) landed as
https://crrev.com/85230becec08b0d0c120f700d8372b2ec204aaba
Cr-Commit-Position: refs/heads/master@{#398423}