Console line numbers are missing with --site-per-process for some http/tests/security/postMessage layout tests |
|||||
Issue descriptionFailing 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.
,
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
,
May 24 2016
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?
,
May 24 2016
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.
,
May 24 2016
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.
,
May 24 2016
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.
,
May 24 2016
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)
,
May 24 2016
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.
,
May 24 2016
#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.
,
May 24 2016
#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}.
,
May 24 2016
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?
,
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.
,
May 24 2016
#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.
,
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
,
May 26 2016
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by lukasza@chromium.org
, May 24 2016