New issue
Advanced search Search tips

Issue 614413 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: May 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Console line numbers are missing with --site-per-process for some http/tests/security/postMessage layout tests

Project Member Reported by lukasza@chromium.org, May 24 2016

Issue description

Failing tests:

http/tests/security/postMessage/target-origin.html
http/tests/security/postMessage/invalid-origin-throws-exception.html

This has started in:

https://build.chromium.org/p/chromium.fyi/builders/Site%20Isolation%20Linux/builds/9282

Suspecting this CL from dgozman@:

https://codereview.chromium.org/1997293002

The failure is similar to  issue 602421  - we probably want a similar fix to consistently avoid line numbers in the output of layout tests.

 
Cc: alex...@chromium.org
Actually, this seems different from the timing-related  issue 602421 .  In this case, the line numbers are lost during IPC / are not present in FrameMsg_PostMessage_Params.

Callstack printing a message in the working case, without --site-per-process:

blink::ChromeClientImpl::addMessageToConsole()
blink::FrameConsole::reportMessageToClient()
blink::LocalDOMWindow::dispatchMessageEventWithOriginCheck()
blink::LocalDOMWindow::postMessageTimerFired()
blink::PostMessageTimer::fired()
blink::TimerBase::runInternal()
blink::TimerBase::CancellableTimerTask::run()

Callstack printing a message in the broken case / without line numbers (when running with --site-per-process):

blink::ChromeClientImpl::addMessageToConsole()
blink::FrameConsole::reportMessageToClient()
blink::LocalDOMWindow::dispatchMessageEventWithOriginCheck()
blink::WebLocalFrameImpl::dispatchMessageEventWithOriginCheck()
content::RenderFrameImpl::OnPostMessageEvent()
Project Member

Comment 2 by bugdroid1@chromium.org, May 24 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ca6b0a7d56f763870651e53974cc9b2e08cc6642

commit ca6b0a7d56f763870651e53974cc9b2e08cc6642
Author: lukasza <lukasza@chromium.org>
Date: Tue May 24 18:11:01 2016

Suppress missing line number failures under http/tests/security/postMessage.

BUG= 614413 
TBR=dgozman@chromium.org
NOTRY=true

Review-Url: https://codereview.chromium.org/2010543004
Cr-Commit-Position: refs/heads/master@{#395640}

[modify] https://crrev.com/ca6b0a7d56f763870651e53974cc9b2e08cc6642/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process

Yes, for #1, in the first case the postMessage is dispatched locally within the same process, and in the second case it's forwarded across processes.  In the latter case, we don't have the line number available (the frame that sent the message is remote) and just pass null for source location from blink::WebLocalFrameImpl::dispatchMessageEventWithOriginCheck.

It would be nice if these console messages were the same with --site-per-process and without.  In some similar cases we've gotten rid of line numbers altogether for consistency (see https://codereview.chromium.org/1550723003, specifically https://codereview.chromium.org/1550723003/diff/280001/third_party/WebKit/Source/core/loader/MixedContentChecker.cpp#newcode377).  Also, I don't know if forwarding the source location is even possible, but thinking about the cross-process case, it actually seems undesirable to forward it: we'd be unnecessarily revealing something about the script source of the source frame to the target frame's process.

dgozman@: what are your thoughts on this?  Would it be ok to keep the old behavior of not printing line numbers for postMessage origin mismatch errors?

I see the point about not leaking information, but regressing the regular same-process usecase seems strange. Let's instead organize the tests differently: either have different expectations or not dump line number.
Cc: lukasza@chromium.org
Owner: ----
Status: Available (was: Started)
I am not sure what you mean by "regressing the regular same-process usecase".  It seems that you recent CL changed the expectations for same-process usecase - no line numbers were expected in 

http/tests/security/postMessage/invalid-origin-throws-exception-expected.txt
http/tests/security/postMessage/target-origin-expected.txt

before https://codereview.chromium.org/1997293002.

Given above (and given the precedent alexmos@ pointed out in #c3 in mixed content checks) I think I would also vote for avoiding the line numbers in both the same-process and cross-process cases.
I can't quite agree that blocking the positive changes (and you do think that reporting correct location is a good thing in itself, don't you?) is a right choice.

Why can't we have different expectations? Does it harm anybody? Not having the correct location definitely harms developers trying to debug their code.

It's unfortunate that we had to settle for suboptimal reporting in mixed content case. Maybe different expectations could work there as well? I'm not familiar enough to judge in that area though.
Is there a mechanism that layout tests can use to declare different expected test output depending on command line flags?  (i.e. depending on whether --additional-drt-flag=--site-per-process was passed to run-webkit-tests)
I'm not an expert here. There are different expectations for virtual/ test suites, which leave in LayoutTests/platform/<platform>/virtual/<test-suite>. Not sure if that helps. It's interesting that we never needed different expectations for site-per-process before.
#6: I agree the line numbers can be useful for developers.  But keep in mind that this will fundamentally give an inconsistent experience: sometimes the line numbers will be there and sometimes they won't be, depending on whether site isolation is enabled for the source/target sites.  It would make sense if the line numbers showed up for same-origin frames and didn't show up for cross-origin frames, but it seems this particular error is only possible when the source and target frames are cross-origin (right?).  Having the behavior differ based on whether the cross-origin frame is in process or not does not seem good.

#7: I see that run-webkit-tests can take a --additional-platform-directory, which we can point to a path like FlagExpectations/site-per-process/ and put site-per-process-specific baselines there.  I haven't tried it though, and we'd need to change our bot recipe, but it should be possible.  The issue with that, though, is having to maintain mostly duplicated baselines for the same tests, especially when we start experimenting with different policies on what sites to isolate.

Comment 10 by creis@chromium.org, May 24 2016

Cc: creis@chromium.org
#9: Yeah, I'm concerned about the maintenance burden of keeping separate test expectations for every Site Isolation policy (beyond just the behavior difference to users when OOPIFs are present).

In the long run (e.g., the next phase of DevTools support for OOPIFs where we merge them into one inspector window), this feels like something that could be solved by having the browser process piece together the console message.  That would let the error message show the right line number (and any other info about the source frame) without ever revealing it to the target frame's renderer process.  I'm guessing that's difficult at the moment.

Not sure what the right answer is here, but I'm skeptical that we want test output to differ based on whether we're using --isolate-extensions, --site-per-process, --top-document-isolation, or --isolate-sites-for-testing={some set of sites}.
Cc: caseq@chromium.org
What about muting/adjusting console output in these tests? Then expectations would match.
@caseq recently mentioned another usecase for muting console in tests. Do we have it now?

Comment 12 by caseq@chromium.org, May 24 2016

yup, it's just landed as https://chromium.googlesource.com/chromium/src/+/cb2227837d5d0e6a59e73eb62e56d53534a69386 -- basically, you can now use testRunner.setDumpConsoleMessages(false) to mute console messages.
#11: yeah, I think I'd be ok with this.  Long-term, the approach Charlie described in #10 seems like the best solution, but for now, this would keep the tests enabled.

Looking at these tests though, I'm concerned that we'll lose important coverage if we just remove console messages.  These tests are the only two tests in the whole codebase that verify that this console message is printed at all -- without this, someone could go rip out the error completely and we'd never notice.  We might be able to fix that if we split target-origin.html into two tests: one for the localhost:8000 target with no console messages (this will have OOPIFs with --site-per-process) and one for the 127.0.0.1:8000 (win127) target with console messages (this will never have OOPIFs, as the two frames are same-site), and make sure the latter has some invalid target origins.

Project Member

Comment 14 by bugdroid1@chromium.org, May 26 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6b50e7bb8127b82d23787cb4ee1975a6e3758c82

commit 6b50e7bb8127b82d23787cb4ee1975a6e3758c82
Author: dgozman <dgozman@chromium.org>
Date: Thu May 26 00:34:53 2016

Resolve console output in postMessage tests with site-per-process.

Regular tests do not dump console output anymore, and special same-site test does it.

BUG= 614413 
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2015713002
Cr-Commit-Position: refs/heads/master@{#396055}

[modify] https://crrev.com/6b50e7bb8127b82d23787cb4ee1975a6e3758c82/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process
[modify] https://crrev.com/6b50e7bb8127b82d23787cb4ee1975a6e3758c82/third_party/WebKit/LayoutTests/http/tests/security/postMessage/invalid-origin-throws-exception-expected.txt
[modify] https://crrev.com/6b50e7bb8127b82d23787cb4ee1975a6e3758c82/third_party/WebKit/LayoutTests/http/tests/security/postMessage/invalid-origin-throws-exception.html
[modify] https://crrev.com/6b50e7bb8127b82d23787cb4ee1975a6e3758c82/third_party/WebKit/LayoutTests/http/tests/security/postMessage/target-origin-expected.txt
[add] https://crrev.com/6b50e7bb8127b82d23787cb4ee1975a6e3758c82/third_party/WebKit/LayoutTests/http/tests/security/postMessage/target-origin-same-site-expected.txt
[add] https://crrev.com/6b50e7bb8127b82d23787cb4ee1975a6e3758c82/third_party/WebKit/LayoutTests/http/tests/security/postMessage/target-origin-same-site.html
[modify] https://crrev.com/6b50e7bb8127b82d23787cb4ee1975a6e3758c82/third_party/WebKit/LayoutTests/http/tests/security/postMessage/target-origin.html

Status: Fixed (was: Available)

Sign in to add a comment