Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1383483007:
Add scheme exceptions for isSecureContext (Closed)
|
Created:
5 years, 2 months ago by jww Modified:
5 years, 2 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/src@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd scheme exceptions for isSecureContext
The Privileged Context spec lists an algorithm for whether a powerful
feature should be allowed in a given context. This check verifies that
the full check from the document that makes the request through all the
parent embedders are secure origins. However, we need to provide an
exception to this algorithm for extensions, which should be allowed to
bypass the Web security model.
This CL adds support for a whitelist of schemes (currently just
chrome-extension:) that should be allowed to access powerful features,
even if the full chain of embedders is not all secure. This extensions
the Document::isSecureContext() method to check this whitelist to see if
the current origin should bypass.
BUG=530507
Committed: https://crrev.com/23d6c8471b207d00a8cf65336067a0891c9a936a
Cr-Commit-Position: refs/heads/master@{#354650}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Update check for sandbox #
Total comments: 8
Patch Set 3 : Add browser test #Patch Set 4 : Nits and fixes #
Total comments: 18
Patch Set 5 : Fixes #
Total comments: 32
Patch Set 6 : Fix Rob and Devlin comments #
Total comments: 2
Patch Set 7 : Rebase on ToT #Patch Set 8 : Address latest comments #
Total comments: 2
Patch Set 9 : Simplify LoadExtension call #
Total comments: 2
Patch Set 10 : Update comment #Messages
Total messages: 34 (4 generated)
jww@chromium.org changed reviewers: + mkwst@chromium.org, rdevlin.cronin@chromium.org, rob@robwu.nl
Hi Rob. Can you verify that this meets your expectations? Devlin, can you also do just a basic review here? Also, I'd love to add a browser test to verify the expected extension behavior, but I'm a bit at a loss of where to put it and what it should look like. Finally, Mike, can you review the Blink side of things? Thanks!
On 2015/10/03 01:13:27, jww wrote: > I'd love to add a browser test to verify the expected extension > behavior, but I'm a bit at a loss of where to put it and what it > should look like. The chrome.desktopCapture API generates user media that can be used by a tab. If you add a test that uses gUM with this API, then we get test coverage for isSecureContext and for desktopCapture. Here is existing test code with a mock for the media chooser: https://cs.chromium.org/%22IN_PROC_BROWSER_TEST_F(DesktopCaptureApiTest%22 Here is a extension that was manually used to verify that desktopCapture performs as expected when the media is requested in a tab: https://crbug.com/425344#c13 https://codereview.chromium.org/1383483007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1383483007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:5669: Putting this check here implies that the origin also needs to pass isPotentiallyTrustworthy (chrome-extension: does pass it). Was this requirement intentional? The check seems insufficient for <iframe src="page.html" sandbox="allow-scripts allow-same-origin"></iframe>. Try this (which also solves the issue I mentioned above): if (SecurityContext::isSandboxed(SandboxOrigin)) { RefPtr<SecurityOrigin> origin = SecurityOrigin::create(context->url()); if (SecurityPolicy::shouldOriginBypassSecureContextCheck(*origin)) return true; if (!origin->isPotentiallyTrustworthy(errorMessage)) return false; } else { if (SecurityPolicy::shouldOriginBypassSecureContextCheck(*securityOrigin())) return true; if (!securityOrigin()->isPotentiallyTrustworthy(errorMessage)) return false; }
Thanks for the browser_test suggestion. I'll take a look at it. https://codereview.chromium.org/1383483007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1383483007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:5669: On 2015/10/03 10:25:08, robwu wrote: > Putting this check here implies that the origin also needs to pass > isPotentiallyTrustworthy (chrome-extension: does pass it). Was this requirement > intentional? Yes, this is intentional because an origin should be trustworthy before we're willing to allow a bypass of the ancestor check. > > The check seems insufficient for <iframe src="page.html" sandbox="allow-scripts > allow-same-origin"></iframe>. Try this (which also solves the issue I mentioned > above): > > if (SecurityContext::isSandboxed(SandboxOrigin)) { > RefPtr<SecurityOrigin> origin = SecurityOrigin::create(context->url()); > if (SecurityPolicy::shouldOriginBypassSecureContextCheck(*origin)) > return true; > if (!origin->isPotentiallyTrustworthy(errorMessage)) > return false; > } else { > if > (SecurityPolicy::shouldOriginBypassSecureContextCheck(*securityOrigin())) > return true; > if (!securityOrigin()->isPotentiallyTrustworthy(errorMessage)) > return false; > } Is your concern is a chrome-extensions: frame wanting to make a sandboxed iframe? If that's the case, then I'd love to get Devlin's opinion on doing this, since the general idea was to avoid children of chrome-extension: frames getting an exception, but I agree that in this very specific case (a direct sandboxed child) it does make sense to have an exception.
https://codereview.chromium.org/1383483007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1383483007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:5669: On 2015/10/03 17:15:06, jww wrote: > On 2015/10/03 10:25:08, robwu wrote: > > Putting this check here implies that the origin also needs to pass > > isPotentiallyTrustworthy (chrome-extension: does pass it). Was this > requirement > > intentional? > Yes, this is intentional because an origin should be trustworthy before we're > willing to allow a bypass of the ancestor check. > > > > The check seems insufficient for <iframe src="page.html" > sandbox="allow-scripts > > allow-same-origin"></iframe>. Try this (which also solves the issue I > mentioned > > above): > > > > if (SecurityContext::isSandboxed(SandboxOrigin)) { > > RefPtr<SecurityOrigin> origin = > SecurityOrigin::create(context->url()); > > if (SecurityPolicy::shouldOriginBypassSecureContextCheck(*origin)) > > return true; > > if (!origin->isPotentiallyTrustworthy(errorMessage)) > > return false; > > } else { > > if > > (SecurityPolicy::shouldOriginBypassSecureContextCheck(*securityOrigin())) > > return true; > > if (!securityOrigin()->isPotentiallyTrustworthy(errorMessage)) > > return false; > > } > Is your concern is a chrome-extensions: frame wanting to make a sandboxed > iframe? If that's the case, then I'd love to get Devlin's opinion on doing this, > since the general idea was to avoid children of chrome-extension: frames getting > an exception, but I agree that in this very specific case (a direct sandboxed > child) it does make sense to have an exception. Yes, with the sandboxed frame being at the chrome-extension: scheme AND allow-same-origin set. Access is still blocked at other schemes (including about:srcdoc/blank) are still blocked. From the web dev/extension author's perspective, <iframe> and <iframe sandbox=allow-same-origin> should behave identically for origin-based decisions, right?
Updated the check, but I'll work on the browser test on Monday. https://codereview.chromium.org/1383483007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1383483007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:5669: On 2015/10/03 17:27:09, robwu wrote: > On 2015/10/03 17:15:06, jww wrote: > > On 2015/10/03 10:25:08, robwu wrote: > > > Putting this check here implies that the origin also needs to pass > > > isPotentiallyTrustworthy (chrome-extension: does pass it). Was this > > requirement > > > intentional? > > Yes, this is intentional because an origin should be trustworthy before we're > > willing to allow a bypass of the ancestor check. > > > > > > The check seems insufficient for <iframe src="page.html" > > sandbox="allow-scripts > > > allow-same-origin"></iframe>. Try this (which also solves the issue I > > mentioned > > > above): > > > > > > if (SecurityContext::isSandboxed(SandboxOrigin)) { > > > RefPtr<SecurityOrigin> origin = > > SecurityOrigin::create(context->url()); > > > if (SecurityPolicy::shouldOriginBypassSecureContextCheck(*origin)) > > > return true; > > > if (!origin->isPotentiallyTrustworthy(errorMessage)) > > > return false; > > > } else { > > > if > > > (SecurityPolicy::shouldOriginBypassSecureContextCheck(*securityOrigin())) > > > return true; > > > if (!securityOrigin()->isPotentiallyTrustworthy(errorMessage)) > > > return false; > > > } > > Is your concern is a chrome-extensions: frame wanting to make a sandboxed > > iframe? If that's the case, then I'd love to get Devlin's opinion on doing > this, > > since the general idea was to avoid children of chrome-extension: frames > getting > > an exception, but I agree that in this very specific case (a direct sandboxed > > child) it does make sense to have an exception. > > Yes, with the sandboxed frame being at the chrome-extension: scheme AND > allow-same-origin set. Access is still blocked at other schemes (including > about:srcdoc/blank) are still blocked. > > From the web dev/extension author's perspective, <iframe> and <iframe > sandbox=allow-same-origin> should behave identically for origin-based decisions, > right? I *think* that makes sense, although I'm still going to leave the isPotentiallyTrustworthy check first, since we should never allow anything sensitive on an untrustworthy origin. I'll update the CL, but we'll need confirmation from Devlin and Mike.
https://codereview.chromium.org/1383483007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1383483007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:5669: On 2015/10/03 17:56:59, jww wrote: > On 2015/10/03 17:27:09, robwu wrote: > > Yes, with the sandboxed frame being at the chrome-extension: scheme AND > > allow-same-origin set. Access is still blocked at other schemes (including > > about:srcdoc/blank) are still blocked. > > > > From the web dev/extension author's perspective, <iframe> and <iframe > > sandbox=allow-same-origin> should behave identically for origin-based > decisions, > > right? > > I *think* that makes sense, although I'm still going to leave the > isPotentiallyTrustworthy check first, since we should never allow anything > sensitive on an untrustworthy origin. I'll update the CL, but we'll need > confirmation from Devlin and Mike. When I wrote my comment, I mistakenly assumed that the if-block is for sandbox=allow-same-origin, and the else-block for everything else (including sandbox with unique origin). Now I see that the first one means "sandbox with unique origin", and the second one is for everything else, my argument about developer's expectations is no longer valid. But the change (patchset 2) is still okay, because it follows the spirit of the Powerful features spec by ignoring the allow-same-origin sandbox flag (5.1, step 1.2. https://codereview.chromium.org/1383483007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1383483007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:5661: // themselves and to immediate sanbox frame descendants, but *not* to sanbox -> sandbox https://codereview.chromium.org/1383483007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:5664: // <iframe sandbox srcdoc="..."></iframe> This does not pass the check because the URL of the frame is about:srcdoc, not scheme://this-origin-has-exception. If you want to allow about:srcdoc and about:blank frames to pass the check, then you have to traverse the frame tree in the loop below.
https://codereview.chromium.org/1383483007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/weborigin/SecurityPolicy.cpp (right): https://codereview.chromium.org/1383483007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/weborigin/SecurityPolicy.cpp:49: using SchemeSet = HashSet<String>; I'd prefer to see this done via `Source/platform/weborigin/SchemeRegistry.h`; that's already a collection of scheme-based exclusion lists, so let's keep those kinds of things together. https://codereview.chromium.org/1383483007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/weborigin/SecurityPolicyTest.cpp (right): https://codereview.chromium.org/1383483007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/weborigin/SecurityPolicyTest.cpp:205: EXPECT_TRUE(SecurityPolicy::shouldOriginBypassSecureContextCheck(*origin3)); I'd like to see some tests verifying the nesting behavior. You can take a look at `Source/web/tests/WebDocumentTest.cpp` (the firstPartyForCookies tests) for some examples.
https://codereview.chromium.org/1383483007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1383483007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:5661: // themselves and to immediate sanbox frame descendants, but *not* to On 2015/10/03 19:28:51, robwu wrote: > sanbox -> sandbox Done. https://codereview.chromium.org/1383483007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:5664: // <iframe sandbox srcdoc="..."></iframe> On 2015/10/03 19:28:51, robwu wrote: > This does not pass the check because the URL of the frame is about:srcdoc, not > scheme://this-origin-has-exception. If you want to allow about:srcdoc and > about:blank frames to pass the check, then you have to traverse the frame tree > in the loop below. Hm, no, I don't want that to pass; I'm not sure what I was getting at with this note. I'll simplify it. https://codereview.chromium.org/1383483007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/weborigin/SecurityPolicy.cpp (right): https://codereview.chromium.org/1383483007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/weborigin/SecurityPolicy.cpp:49: using SchemeSet = HashSet<String>; On 2015/10/06 07:22:34, Mike West (slow) wrote: > I'd prefer to see this done via `Source/platform/weborigin/SchemeRegistry.h`; > that's already a collection of scheme-based exclusion lists, so let's keep those > kinds of things together. Done. https://codereview.chromium.org/1383483007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/weborigin/SecurityPolicyTest.cpp (right): https://codereview.chromium.org/1383483007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/weborigin/SecurityPolicyTest.cpp:205: EXPECT_TRUE(SecurityPolicy::shouldOriginBypassSecureContextCheck(*origin3)); On 2015/10/06 07:22:34, Mike West (slow) wrote: > I'd like to see some tests verifying the nesting behavior. You can take a look > at `Source/web/tests/WebDocumentTest.cpp` (the firstPartyForCookies tests) for > some examples. Can you clarify what test you'd like to see for the nested behavior? Do you mean you want tests for isSecureContext() explicitly? Or are you talking more specifically about for extensions (which I've added)?
On 2015/10/06 21:53:56, jww (traveling- slow) wrote: > https://codereview.chromium.org/1383483007/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/Document.cpp (right): > > https://codereview.chromium.org/1383483007/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/Document.cpp:5661: // themselves and to > immediate sanbox frame descendants, but *not* to > On 2015/10/03 19:28:51, robwu wrote: > > sanbox -> sandbox > > Done. > > https://codereview.chromium.org/1383483007/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/Document.cpp:5664: // <iframe sandbox > srcdoc="..."></iframe> > On 2015/10/03 19:28:51, robwu wrote: > > This does not pass the check because the URL of the frame is about:srcdoc, not > > scheme://this-origin-has-exception. If you want to allow about:srcdoc and > > about:blank frames to pass the check, then you have to traverse the frame tree > > in the loop below. > > Hm, no, I don't want that to pass; I'm not sure what I was getting at with this > note. I'll simplify it. > > https://codereview.chromium.org/1383483007/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/weborigin/SecurityPolicy.cpp (right): > > https://codereview.chromium.org/1383483007/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/weborigin/SecurityPolicy.cpp:49: using > SchemeSet = HashSet<String>; > On 2015/10/06 07:22:34, Mike West (slow) wrote: > > I'd prefer to see this done via `Source/platform/weborigin/SchemeRegistry.h`; > > that's already a collection of scheme-based exclusion lists, so let's keep > those > > kinds of things together. > > Done. > > https://codereview.chromium.org/1383483007/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/weborigin/SecurityPolicyTest.cpp > (right): > > https://codereview.chromium.org/1383483007/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/weborigin/SecurityPolicyTest.cpp:205: > EXPECT_TRUE(SecurityPolicy::shouldOriginBypassSecureContextCheck(*origin3)); > On 2015/10/06 07:22:34, Mike West (slow) wrote: > > I'd like to see some tests verifying the nesting behavior. You can take a look > > at `Source/web/tests/WebDocumentTest.cpp` (the firstPartyForCookies tests) for > > some examples. > > Can you clarify what test you'd like to see for the nested behavior? Do you mean > you want tests for isSecureContext() explicitly? Or are you talking more > specifically about for extensions (which I've added)? A friendly ping on this review.
https://codereview.chromium.org/1383483007/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc (right): https://codereview.chromium.org/1383483007/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:73: // Flaky on Windows: http://crbug.com/301887 Why are newly-added tests already flaky? :) https://codereview.chromium.org/1383483007/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:90: ASSERT_TRUE(RunExtensionTest("webrtc_from_web_accessible_resource")) For funzies, we should also add PermissionBubbleManager::FromWebContents(web_contents) ->set_auto_response_for_test(PermissionBubbleManager::REJECT_ALL); ASSERT_FALSE(RunExtensionTest("webrtc_from_web_accessible_resource")); To make sure that if the user refuses, it doesn't get access. (This probably required clearing the permissions for the web contents, or navigating to a different page.) https://codereview.chromium.org/1383483007/diff/60001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/manifest.json (right): https://codereview.chromium.org/1383483007/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/manifest.json:17: "desktopCapture", Don't need this, right? https://codereview.chromium.org/1383483007/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1383483007/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:5687: if (SecurityContext::isSandboxed(SandboxOrigin)) { I don't like that we duplicate the logic, just passing in a different security origin, now that it's a little more complicated. Unfortunately, the RefPtr vs raw ptr makes it a bit of a pain to clean up, so I'm not sure this is any better... RefPtr<SecurityOrigin> originWrapper; SecurityOrigin* origin = nullptr; if (SecurityContext::isSandboxed(SandboxOrigin)) { originWrapper = SecurityOrigin::create(url()); origin = originWrapper.get(); } else { origin = securityOrigin(); } if (origin->IsPotentiallyTrustworthy(errorMessage)) return false; if (SecurityOrigin::shouldOriginBypassSecureContextCheck(*origin) return true; (Depending on how performance-sensitive this code is, you could also just say you don't care about the added ref, and the top part becomes simply: RefPtr<SecurityOrigin> originWrapper = SecurityContext::isSandboxed(SandboxOrigin) ? SecurityOrigin::create(url()) : securityOrigin(); ) I'll let you decide whichever you prefer (including yours).
https://codereview.chromium.org/1383483007/diff/60001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/manifest.json (right): https://codereview.chromium.org/1383483007/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/manifest.json:15: "web_accessible_resources": ["iframe-content.html", "iframe.js"], iframe.js does not need to be listed, only the frame itself should be here. https://codereview.chromium.org/1383483007/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/manifest.json:18: "tabs", "http://a.com/*" The extension does not need any permissions. - chrome.tabs.create does not require the tabs permission. - desktopCapture is not used - Declaring a content script is sufficient to get the script to run. https://codereview.chromium.org/1383483007/diff/60001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/test.js (left): https://codereview.chromium.org/1383483007/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/test.js:6: var iframe = document.createElement('iframe'); This variable seems unused, remove it. And you could ditch this background page if you use the following: // Right after ASSERT_TRUE(RunExtensionTest(... ui_test_utils::NavigateToURL(browser(), embedded_test_server()->GetURL("/extensions/test_file.html")); https://codereview.chromium.org/1383483007/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/weborigin/SchemeRegistryTest.cpp (right): https://codereview.chromium.org/1383483007/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/weborigin/SchemeRegistryTest.cpp:67: EXPECT_TRUE(SchemeRegistry::schemeShouldBypassSecureContextCheck(scheme3)); Add some tests for the uppercase random scheme, just in case. https://codereview.chromium.org/1383483007/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/weborigin/SecurityPolicy.cpp (right): https://codereview.chromium.org/1383483007/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/weborigin/SecurityPolicy.cpp:35: #include "platform/weborigin/SchemeRegistry.h" This include is no longer needed.
https://codereview.chromium.org/1383483007/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc (right): https://codereview.chromium.org/1383483007/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:73: // Flaky on Windows: http://crbug.com/301887 On 2015/10/08 16:48:32, Devlin wrote: > Why are newly-added tests already flaky? :) Whoops, leftovers from when I first started trying to build off the desktop_capture_apitest. Removing the flaky comment and macros. https://codereview.chromium.org/1383483007/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:90: ASSERT_TRUE(RunExtensionTest("webrtc_from_web_accessible_resource")) On 2015/10/08 16:48:32, Devlin wrote: > For funzies, we should also add > PermissionBubbleManager::FromWebContents(web_contents) > ->set_auto_response_for_test(PermissionBubbleManager::REJECT_ALL); > ASSERT_FALSE(RunExtensionTest("webrtc_from_web_accessible_resource")); > > To make sure that if the user refuses, it doesn't get access. > > (This probably required clearing the permissions for the web contents, or > navigating to a different page.) Good call. Added. https://codereview.chromium.org/1383483007/diff/60001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/manifest.json (right): https://codereview.chromium.org/1383483007/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/manifest.json:15: "web_accessible_resources": ["iframe-content.html", "iframe.js"], On 2015/10/08 21:25:18, robwu wrote: > iframe.js does not need to be listed, only the frame itself should be here. Done. https://codereview.chromium.org/1383483007/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/manifest.json:17: "desktopCapture", On 2015/10/08 16:48:32, Devlin wrote: > Don't need this, right? Done. https://codereview.chromium.org/1383483007/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/manifest.json:18: "tabs", "http://a.com/*" On 2015/10/08 21:25:18, robwu wrote: > The extension does not need any permissions. > - chrome.tabs.create does not require the tabs permission. > - desktopCapture is not used > - Declaring a content script is sufficient to get the script to run. Yup, that was from an old version of the test. Nice catch. https://codereview.chromium.org/1383483007/diff/60001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/test.js (left): https://codereview.chromium.org/1383483007/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/test.js:6: var iframe = document.createElement('iframe'); On 2015/10/08 21:25:18, robwu wrote: > This variable seems unused, remove it. > > And you could ditch this background page if you use the following: > > // Right after ASSERT_TRUE(RunExtensionTest(... > ui_test_utils::NavigateToURL(browser(), > embedded_test_server()->GetURL("/extensions/test_file.html")); I cleaned this up a bit, but it's now necessary for a different reason. A comment explains it in the latest patch, but basically we need the background page to 'pass' once to sort of kickstart the test, even though the true success and failure are in the content script. https://codereview.chromium.org/1383483007/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1383483007/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:5687: if (SecurityContext::isSandboxed(SandboxOrigin)) { On 2015/10/08 16:48:32, Devlin wrote: > I don't like that we duplicate the logic, just passing in a different security > origin, now that it's a little more complicated. Unfortunately, the RefPtr vs > raw ptr makes it a bit of a pain to clean up, so I'm not sure this is any > better... > > RefPtr<SecurityOrigin> originWrapper; > SecurityOrigin* origin = nullptr; > if (SecurityContext::isSandboxed(SandboxOrigin)) { > originWrapper = SecurityOrigin::create(url()); > origin = originWrapper.get(); > } else { > origin = securityOrigin(); > } > if (origin->IsPotentiallyTrustworthy(errorMessage)) > return false; > if (SecurityOrigin::shouldOriginBypassSecureContextCheck(*origin) > return true; > > (Depending on how performance-sensitive this code is, you could also just say > you don't care about the added ref, and the top part becomes simply: > RefPtr<SecurityOrigin> originWrapper = > SecurityContext::isSandboxed(SandboxOrigin) ? SecurityOrigin::create(url()) : > securityOrigin(); > ) > > I'll let you decide whichever you prefer (including yours). I actually considered these approaches before, and went with this one, because I thought it was less confusing that mixing up the raw pointers and ref pointers. Agreed that it's ugly though. https://codereview.chromium.org/1383483007/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/weborigin/SchemeRegistryTest.cpp (right): https://codereview.chromium.org/1383483007/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/weborigin/SchemeRegistryTest.cpp:67: EXPECT_TRUE(SchemeRegistry::schemeShouldBypassSecureContextCheck(scheme3)); On 2015/10/08 21:25:18, robwu wrote: > Add some tests for the uppercase random scheme, just in case. Done. https://codereview.chromium.org/1383483007/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/weborigin/SecurityPolicy.cpp (right): https://codereview.chromium.org/1383483007/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/weborigin/SecurityPolicy.cpp:35: #include "platform/weborigin/SchemeRegistry.h" On 2015/10/08 21:25:18, robwu wrote: > This include is no longer needed. Done.
Ping of Rob and Devlin. --Joel On 2015/10/09 21:39:26, jww (traveling- slow) wrote: > https://codereview.chromium.org/1383483007/diff/60001/chrome/browser/extensio... > File > chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc > (right): > > https://codereview.chromium.org/1383483007/diff/60001/chrome/browser/extensio... > chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:73: > // Flaky on Windows: http://crbug.com/301887 > On 2015/10/08 16:48:32, Devlin wrote: > > Why are newly-added tests already flaky? :) > > Whoops, leftovers from when I first started trying to build off the > desktop_capture_apitest. Removing the flaky comment and macros. > > https://codereview.chromium.org/1383483007/diff/60001/chrome/browser/extensio... > chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:90: > ASSERT_TRUE(RunExtensionTest("webrtc_from_web_accessible_resource")) > On 2015/10/08 16:48:32, Devlin wrote: > > For funzies, we should also add > > PermissionBubbleManager::FromWebContents(web_contents) > > ->set_auto_response_for_test(PermissionBubbleManager::REJECT_ALL); > > ASSERT_FALSE(RunExtensionTest("webrtc_from_web_accessible_resource")); > > > > To make sure that if the user refuses, it doesn't get access. > > > > (This probably required clearing the permissions for the web contents, or > > navigating to a different page.) > > Good call. Added. > > https://codereview.chromium.org/1383483007/diff/60001/chrome/test/data/extens... > File > chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/manifest.json > (right): > > https://codereview.chromium.org/1383483007/diff/60001/chrome/test/data/extens... > chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/manifest.json:15: > "web_accessible_resources": ["iframe-content.html", "iframe.js"], > On 2015/10/08 21:25:18, robwu wrote: > > iframe.js does not need to be listed, only the frame itself should be here. > > Done. > > https://codereview.chromium.org/1383483007/diff/60001/chrome/test/data/extens... > chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/manifest.json:17: > "desktopCapture", > On 2015/10/08 16:48:32, Devlin wrote: > > Don't need this, right? > > Done. > > https://codereview.chromium.org/1383483007/diff/60001/chrome/test/data/extens... > chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/manifest.json:18: > "tabs", "http://a.com/*" > On 2015/10/08 21:25:18, robwu wrote: > > The extension does not need any permissions. > > - chrome.tabs.create does not require the tabs permission. > > - desktopCapture is not used > > - Declaring a content script is sufficient to get the script to run. > > Yup, that was from an old version of the test. Nice catch. > > https://codereview.chromium.org/1383483007/diff/60001/chrome/test/data/extens... > File > chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/test.js > (left): > > https://codereview.chromium.org/1383483007/diff/60001/chrome/test/data/extens... > chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/test.js:6: > var iframe = document.createElement('iframe'); > On 2015/10/08 21:25:18, robwu wrote: > > This variable seems unused, remove it. > > > > And you could ditch this background page if you use the following: > > > > // Right after ASSERT_TRUE(RunExtensionTest(... > > ui_test_utils::NavigateToURL(browser(), > > embedded_test_server()->GetURL("/extensions/test_file.html")); > > I cleaned this up a bit, but it's now necessary for a different reason. A > comment explains it in the latest patch, but basically we need the background > page to 'pass' once to sort of kickstart the test, even though the true success > and failure are in the content script. > > https://codereview.chromium.org/1383483007/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/Document.cpp (right): > > https://codereview.chromium.org/1383483007/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/Document.cpp:5687: if > (SecurityContext::isSandboxed(SandboxOrigin)) { > On 2015/10/08 16:48:32, Devlin wrote: > > I don't like that we duplicate the logic, just passing in a different security > > origin, now that it's a little more complicated. Unfortunately, the RefPtr vs > > raw ptr makes it a bit of a pain to clean up, so I'm not sure this is any > > better... > > > > RefPtr<SecurityOrigin> originWrapper; > > SecurityOrigin* origin = nullptr; > > if (SecurityContext::isSandboxed(SandboxOrigin)) { > > originWrapper = SecurityOrigin::create(url()); > > origin = originWrapper.get(); > > } else { > > origin = securityOrigin(); > > } > > if (origin->IsPotentiallyTrustworthy(errorMessage)) > > return false; > > if (SecurityOrigin::shouldOriginBypassSecureContextCheck(*origin) > > return true; > > > > (Depending on how performance-sensitive this code is, you could also just say > > you don't care about the added ref, and the top part becomes simply: > > RefPtr<SecurityOrigin> originWrapper = > > SecurityContext::isSandboxed(SandboxOrigin) ? SecurityOrigin::create(url()) : > > securityOrigin(); > > ) > > > > I'll let you decide whichever you prefer (including yours). > > I actually considered these approaches before, and went with this one, because I > thought it was less confusing that mixing up the raw pointers and ref pointers. > Agreed that it's ugly though. > > https://codereview.chromium.org/1383483007/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/weborigin/SchemeRegistryTest.cpp > (right): > > https://codereview.chromium.org/1383483007/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/weborigin/SchemeRegistryTest.cpp:67: > EXPECT_TRUE(SchemeRegistry::schemeShouldBypassSecureContextCheck(scheme3)); > On 2015/10/08 21:25:18, robwu wrote: > > Add some tests for the uppercase random scheme, just in case. > > Done. > > https://codereview.chromium.org/1383483007/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/weborigin/SecurityPolicy.cpp (right): > > https://codereview.chromium.org/1383483007/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/weborigin/SecurityPolicy.cpp:35: #include > "platform/weborigin/SchemeRegistry.h" > On 2015/10/08 21:25:18, robwu wrote: > > This include is no longer needed. > > Done.
Looks good overall! https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc (right): https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:88: GetUserMediaInWebAccessibleResourceSuccess) { Could you also add a test for <iframe sandbox src=chrome-extension: ...>? https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/iframe.js (right): https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/iframe.js:5: var extensionId = "knldjmfmopnpolahpmmgbagdohdnhkik"; extensionId seems unused. https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/iframe.js:12: console.log('failing...'); Could you reword this to be a more descriptive? (2x) E.g. "Expecting getUserMedia to fail..." https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/iframe.js:13: successCallback = chrome.test.fail; Note that the success callback of gUM is invoked with a MediaStream object. Upon failure you would see "[object MediaStream]" instead of some useful error message. Try successCallback = chrome.test.fail.bind(chrome.test, 'gUM unexpectedly invoked the success callback'); or successCallback = function() { chrome.test.fail('gUM unexpectedly invoked the success callback'); };
Mostly small stuff https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc (right): https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:9: #include "chrome/common/chrome_switches.h" Do you need all three of these switches.h files? https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:27: message_loop_runner_(new content::MessageLoopRunner) { nitty nit: though it doesn't matter for MessageLoopRunner, prefer value-initialization for types (i.e., new MessageLoopRunner()) to avoid a random nasty pitfall. Though this is probably moot with below. https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:35: void Wait() { message_loop_runner_->Run(); } It's usually better to have a check that the request hasn't been shown yet (if code executes synchronously in the future, it could be) before running the message loop, to avoid running endlessly. That said, I don't think this method is actually used? (Which means it and the message loop runner can be removed). https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:65: // This test expects to run with fake devices. nitty nit: this comment tells us nothing that the code doesn't. :) It should either be expanded, or removed (I'd probably just remove it). https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:96: ui_test_utils::NavigateToURL(browser(), url); Is there a race condition in these steps? If we navigate to the url and the content script executes, then isn't there a chance gUM will be called before we set the auto response and set up the result catcher? Or am I missing something? (The fix would just be to set the auto response and initialize the result catcher above the navigation.) https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/content-script.js (right): https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/content-script.js:1: // Copyright 2015 The Chromium Authors. All rights reserved. prefer content_script.js over content-script.js https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/iframe-content.html (right): https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/iframe-content.html:1: <html> iframe_content.html https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/test.js (right): https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/test.js:5: chrome.test.getConfig(function(config) { Do we really need this step at all?
https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc (right): https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:9: #include "chrome/common/chrome_switches.h" On 2015/10/14 15:24:45, Devlin wrote: > Do you need all three of these switches.h files? Nope, removed chrome_switches.h and content_switches.h. https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:27: message_loop_runner_(new content::MessageLoopRunner) { On 2015/10/14 15:24:45, Devlin wrote: > nitty nit: though it doesn't matter for MessageLoopRunner, prefer > value-initialization for types (i.e., new MessageLoopRunner()) to avoid a random > nasty pitfall. > > Though this is probably moot with below. Acknowledged. https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:35: void Wait() { message_loop_runner_->Run(); } On 2015/10/14 15:24:45, Devlin wrote: > It's usually better to have a check that the request hasn't been shown yet (if > code executes synchronously in the future, it could be) before running the > message loop, to avoid running endlessly. > > That said, I don't think this method is actually used? (Which means it and the > message loop runner can be removed). Yup, I copied this from some WebRTC code and, apparently, didn't go over it carefully enough :-P Sorry! Removing. https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:65: // This test expects to run with fake devices. On 2015/10/14 15:24:45, Devlin wrote: > nitty nit: this comment tells us nothing that the code doesn't. :) It should > either be expanded, or removed (I'd probably just remove it). Done. https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:88: GetUserMediaInWebAccessibleResourceSuccess) { On 2015/10/14 09:05:35, robwu wrote: > Could you also add a test for <iframe sandbox src=chrome-extension: ...>? Unfortunately, I can't, but that's not a change from the status quo. You can't use getUserMedia in a sandbox because MediaStreamDispatchHost::IsURLAllowed returns false for sandboxed iframes (unless 'allow-same-origin' is given): https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:96: ui_test_utils::NavigateToURL(browser(), url); On 2015/10/14 15:24:45, Devlin wrote: > Is there a race condition in these steps? If we navigate to the url and the > content script executes, then isn't there a chance gUM will be called before we > set the auto response and set up the result catcher? Or am I missing something? > > (The fix would just be to set the auto response and initialize the result > catcher above the navigation.) I believe that's not a problem because it will sit and wait for a PermissionBubbleManager response, which we give below. https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/content-script.js (right): https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/content-script.js:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/10/14 15:24:45, Devlin wrote: > prefer content_script.js over content-script.js Done. https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/iframe-content.html (right): https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/iframe-content.html:1: <html> On 2015/10/14 15:24:45, Devlin wrote: > iframe_content.html Done. https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/iframe.js (right): https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/iframe.js:5: var extensionId = "knldjmfmopnpolahpmmgbagdohdnhkik"; On 2015/10/14 09:05:35, robwu wrote: > extensionId seems unused. Done. https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/iframe.js:12: console.log('failing...'); On 2015/10/14 09:05:35, robwu wrote: > Could you reword this to be a more descriptive? (2x) > E.g. "Expecting getUserMedia to fail..." Instead, I'll just remove all of these unnecessary console logs that I accidentally left in from debugging :-) https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/iframe.js:13: successCallback = chrome.test.fail; On 2015/10/14 09:05:35, robwu wrote: > Note that the success callback of gUM is invoked with a MediaStream object. > > Upon failure you would see "[object MediaStream]" instead of some useful error > message. > Try > > successCallback = chrome.test.fail.bind(chrome.test, 'gUM unexpectedly invoked > the success callback'); > > or > > successCallback = function() { > chrome.test.fail('gUM unexpectedly invoked the success callback'); > }; Done. https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/test.js (right): https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/test.js:5: chrome.test.getConfig(function(config) { On 2015/10/14 15:24:45, Devlin wrote: > Do we really need this step at all? Well, we don't need the getConfig() (so I'm removing), but as the comment below mentions, we do need this initial notifyPass() just to get the tests started. This is a pattern that I've seen in a number of other tests.
Rob & Devlin, can you take another look? Thanks!
(Just responding to responses. :)) https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc (right): https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:96: ui_test_utils::NavigateToURL(browser(), url); On 2015/10/15 18:19:41, jww (traveling- slow) wrote: > On 2015/10/14 15:24:45, Devlin wrote: > > Is there a race condition in these steps? If we navigate to the url and the > > content script executes, then isn't there a chance gUM will be called before > we > > set the auto response and set up the result catcher? Or am I missing > something? > > > > (The fix would just be to set the auto response and initialize the result > > catcher above the navigation.) > > I believe that's not a problem because it will sit and wait for a > PermissionBubbleManager response, which we give below. It will sit and wait, but set_auto_response_for_test() doesn't seem to do anything for existing requests. So my worry is that if we actually get to the point of asking for permission before we set the auto response, then the prompt will just sit there indefinitely. Is this incorrect? https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:103: PermissionRequestObserver permissionRequestObserver(web_contents); as I was responding to the comment above, I realized this is camelcase. Prefer unix_hacker_style for variables. https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/test.js (right): https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/test.js:5: chrome.test.getConfig(function(config) { On 2015/10/15 18:19:41, jww (traveling- slow) wrote: > On 2015/10/14 15:24:45, Devlin wrote: > > Do we really need this step at all? > > Well, we don't need the getConfig() (so I'm removing), but as the comment below > mentions, we do need this initial notifyPass() just to get the tests started. > This is a pattern that I've seen in a number of other tests. But why do we need this instead of just, say, LoadExtension()? That should load the extension, and even wait for associated views to load.
https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc (right): https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:96: ui_test_utils::NavigateToURL(browser(), url); On 2015/10/15 18:34:11, Devlin wrote: > On 2015/10/15 18:19:41, jww (traveling- slow) wrote: > > On 2015/10/14 15:24:45, Devlin wrote: > > > Is there a race condition in these steps? If we navigate to the url and the > > > content script executes, then isn't there a chance gUM will be called before > > we > > > set the auto response and set up the result catcher? Or am I missing > > something? > > > > > > (The fix would just be to set the auto response and initialize the result > > > catcher above the navigation.) > > > > I believe that's not a problem because it will sit and wait for a > > PermissionBubbleManager response, which we give below. > > It will sit and wait, but set_auto_response_for_test() doesn't seem to do > anything for existing requests. So my worry is that if we actually get to the > point of asking for permission before we set the auto response, then the prompt > will just sit there indefinitely. > > Is this incorrect? Devlin's comment seems to be right. To avoid any races, the content script should not call gUM until the autoresponder has been set up. This can be achieved as follows: // contentscript.js chrome.test.sendMessage('onReady', function() { // Start gUM test here }); // webrtc_from_web_accessible_resource_browsertest.cc ExtensionTestMessageListener listener("onReady", false); ... RunExtensionTest, Navigate, set up autoresponder, etc... // Now allow the content script to proceed. EXPECT_TRUE(listener.WaitUntilSatisfied()); listener.Reply(""); ... Wait until completion + run assertions... https://codereview.chromium.org/1383483007/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1383483007/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:5660: // schemes. The exceptions are applied only to the origin special scheme I think that you forgot to remove "origin" here.
https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc (right): https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:96: ui_test_utils::NavigateToURL(browser(), url); On 2015/10/15 20:56:11, robwu wrote: > On 2015/10/15 18:34:11, Devlin wrote: > > On 2015/10/15 18:19:41, jww (traveling- slow) wrote: > > > On 2015/10/14 15:24:45, Devlin wrote: > > > > Is there a race condition in these steps? If we navigate to the url and > the > > > > content script executes, then isn't there a chance gUM will be called > before > > > we > > > > set the auto response and set up the result catcher? Or am I missing > > > something? > > > > > > > > (The fix would just be to set the auto response and initialize the result > > > > catcher above the navigation.) > > > > > > I believe that's not a problem because it will sit and wait for a > > > PermissionBubbleManager response, which we give below. > > > > It will sit and wait, but set_auto_response_for_test() doesn't seem to do > > anything for existing requests. So my worry is that if we actually get to the > > point of asking for permission before we set the auto response, then the > prompt > > will just sit there indefinitely. > > > > Is this incorrect? > > Devlin's comment seems to be right. > > To avoid any races, the content script should not call gUM until the > autoresponder has been set up. This can be achieved as follows: > > > // contentscript.js > chrome.test.sendMessage('onReady', function() { > // Start gUM test here > }); > > > // webrtc_from_web_accessible_resource_browsertest.cc > ExtensionTestMessageListener listener("onReady", false); > ... RunExtensionTest, Navigate, set up autoresponder, etc... > // Now allow the content script to proceed. > EXPECT_TRUE(listener.WaitUntilSatisfied()); > listener.Reply(""); > ... Wait until completion + run assertions... A simpler solution might be to just set the auto response (and initialize the result catcher) before navigating (assuming autoresponses aren't cleared on navigation - I don't think they are). But either one works - whatever's easiest.
Latest version. Let me know if it looks good. https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc (right): https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:96: ui_test_utils::NavigateToURL(browser(), url); On 2015/10/15 21:21:02, Devlin wrote: > On 2015/10/15 20:56:11, robwu wrote: > > On 2015/10/15 18:34:11, Devlin wrote: > > > On 2015/10/15 18:19:41, jww (traveling- slow) wrote: > > > > On 2015/10/14 15:24:45, Devlin wrote: > > > > > Is there a race condition in these steps? If we navigate to the url and > > the > > > > > content script executes, then isn't there a chance gUM will be called > > before > > > > we > > > > > set the auto response and set up the result catcher? Or am I missing > > > > something? > > > > > > > > > > (The fix would just be to set the auto response and initialize the > result > > > > > catcher above the navigation.) > > > > > > > > I believe that's not a problem because it will sit and wait for a > > > > PermissionBubbleManager response, which we give below. > > > > > > It will sit and wait, but set_auto_response_for_test() doesn't seem to do > > > anything for existing requests. So my worry is that if we actually get to > the > > > point of asking for permission before we set the auto response, then the > > prompt > > > will just sit there indefinitely. > > > > > > Is this incorrect? > > > > Devlin's comment seems to be right. > > > > To avoid any races, the content script should not call gUM until the > > autoresponder has been set up. This can be achieved as follows: > > > > > > // contentscript.js > > chrome.test.sendMessage('onReady', function() { > > // Start gUM test here > > }); > > > > > > // webrtc_from_web_accessible_resource_browsertest.cc > > ExtensionTestMessageListener listener("onReady", false); > > ... RunExtensionTest, Navigate, set up autoresponder, etc... > > // Now allow the content script to proceed. > > EXPECT_TRUE(listener.WaitUntilSatisfied()); > > listener.Reply(""); > > ... Wait until completion + run assertions... > > A simpler solution might be to just set the auto response (and initialize the > result catcher) before navigating (assuming autoresponses aren't cleared on > navigation - I don't think they are). But either one works - whatever's > easiest. Yup, I took Devlin's suggested approach. https://codereview.chromium.org/1383483007/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:103: PermissionRequestObserver permissionRequestObserver(web_contents); On 2015/10/15 18:34:11, Devlin wrote: > as I was responding to the comment above, I realized this is camelcase. Prefer > unix_hacker_style for variables. Done. https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/test.js (right): https://codereview.chromium.org/1383483007/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrtc_from_web_accessible_resource/test.js:5: chrome.test.getConfig(function(config) { On 2015/10/15 18:34:11, Devlin wrote: > On 2015/10/15 18:19:41, jww (traveling- slow) wrote: > > On 2015/10/14 15:24:45, Devlin wrote: > > > Do we really need this step at all? > > > > Well, we don't need the getConfig() (so I'm removing), but as the comment > below > > mentions, we do need this initial notifyPass() just to get the tests started. > > This is a pattern that I've seen in a number of other tests. > > But why do we need this instead of just, say, LoadExtension()? That should load > the extension, and even wait for associated views to load. Because I'm ignorant and don't understand how extension testing works ;-) I've changed it to just use load extension. https://codereview.chromium.org/1383483007/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1383483007/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:5660: // schemes. The exceptions are applied only to the origin special scheme On 2015/10/15 20:56:11, robwu wrote: > I think that you forgot to remove "origin" here. Done.
lgtm
LGTM, thanks for the functionality! https://codereview.chromium.org/1383483007/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc (right): https://codereview.chromium.org/1383483007/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:81: LoadExtension(extension_path); The whole function (or its callers) can be replaced with ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII("webrtc_from_web_accessible_resource"))); This is defined in ExtensionBrowserTest::SetUpCommandLine and ExtensionApiTest::SetUpCommandLine.
jww@chromium.org changed reviewers: + thestig@chromium.org
mkwst@, can you look at the Blink files? thestig@, can you take a look at the chrome/common and chrome/renderer files? https://codereview.chromium.org/1383483007/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc (right): https://codereview.chromium.org/1383483007/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc:81: LoadExtension(extension_path); On 2015/10/15 22:52:45, robwu wrote: > The whole function (or its callers) can be replaced with > ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII("webrtc_from_web_accessible_resource"))); > > This is defined in ExtensionBrowserTest::SetUpCommandLine and > ExtensionApiTest::SetUpCommandLine. Ah, cool, that's much better :-)
chrome/ lgtm https://codereview.chromium.org/1383483007/diff/160001/chrome/common/secure_o... File chrome/common/secure_origin_whitelist.h (right): https://codereview.chromium.org/1383483007/diff/160001/chrome/common/secure_o... chrome/common/secure_origin_whitelist.h:22: std::set<std::string>* schemes); Might be good to mention |schemes| is an in-out parameter. Same for |origins| above.
https://codereview.chromium.org/1383483007/diff/160001/chrome/common/secure_o... File chrome/common/secure_origin_whitelist.h (right): https://codereview.chromium.org/1383483007/diff/160001/chrome/common/secure_o... chrome/common/secure_origin_whitelist.h:22: std::set<std::string>* schemes); On 2015/10/15 23:17:27, Lei Zhang wrote: > Might be good to mention |schemes| is an in-out parameter. Same for |origins| > above. Done.
blink LGTM.
The CQ bit was checked by jww@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, rob@robwu.nl, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/1383483007/#ps180001 (title: "Update comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1383483007/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1383483007/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/23d6c8471b207d00a8cf65336067a0891c9a936a Cr-Commit-Position: refs/heads/master@{#354650} |
