Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2831183003:
Fix loading success.html at the end of chrome://chrome-signin flow (Closed)
DescriptionFix loading success.html at the end of chrome://chrome-signin flow
This CL lists success.html as an webview accessible resource for
the sign-in extension.
This CL adds an exception for sign-in extension when for loading
webview accessible resources.
BUG=709117
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2831183003
Cr-Commit-Position: refs/heads/master@{#467739}
Committed: https://chromium.googlesource.com/chromium/src/+/8fdab62089307f5fe2307335dd1c71c4c6661867
Patch Set 1 #Patch Set 2 : Allow loading success.html at the end of the chrome://chrome-signin flow #Patch Set 3 : Revert LOG messages #Patch Set 4 : Fix comment #
Total comments: 16
Patch Set 5 : Add code review #
Total comments: 2
Patch Set 6 : Have a hack in a single fine #Patch Set 7 : Add browser test #
Total comments: 5
Patch Set 8 : Add missing expect #
Messages
Total messages: 44 (23 generated)
Description was changed from ========== Hack for chrome://chrome-signin BUG= ========== to ========== Hack for chrome://chrome-signin BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Hack for chrome://chrome-signin BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix loading success.html at the end of chrome://chrome-signin flow BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Fix loading success.html at the end of chrome://chrome-signin flow BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix loading success.html at the end of chrome://chrome-signin flow This CL lists success.html as an webview accessible resource for the sign-in extension. This CL adds an exception for sign-in extension when for loading webview accessible resources. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Fix loading success.html at the end of chrome://chrome-signin flow This CL lists success.html as an webview accessible resource for the sign-in extension. This CL adds an exception for sign-in extension when for loading webview accessible resources. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix loading success.html at the end of chrome://chrome-signin flow This CL lists success.html as an webview accessible resource for the sign-in extension. This CL adds an exception for sign-in extension when for loading webview accessible resources. BUG=709117 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
msarda@chromium.org changed reviewers: + creis@chromium.org, lfg@chromium.org
Please take a look and let me know if this fix is acceptable.
creis@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
[+Devlin to review as well] Thanks for putting together the fix! I'm ok with this as a short term option, as long as we have a path to remove it. Curious if Devlin thinks it's better to just remove the owner_extension check for now, though. Lucas: Can you confirm that this whitelisting won't make us start granting extension APIs to the page inside the webview? I presume it won't. Given the priority of this regression, is it possible to include a test that the webview can load the success.html page so it won't regress again on a future security check? (I'm sure it's hard to test the whole signin flow, but we could make sure this page still works, since it's not a common path to embed an extension this way.) https://codereview.chromium.org/2831183003/diff/60001/extensions/browser/url_... File extensions/browser/url_request_util.cc (right): https://codereview.chromium.org/2831183003/diff/60001/extensions/browser/url_... extensions/browser/url_request_util.cc:160: // even if the webview that is loading the resource does not belog to the nit: belong https://codereview.chromium.org/2831183003/diff/60001/extensions/browser/url_... extensions/browser/url_request_util.cc:163: extension && extension->id() == "mfffpogegjflfpflabcdkioaeobkgjik"; Is there a constant we can use here rather than a magic string? Or is it in a layer we can't access? https://codereview.chromium.org/2831183003/diff/60001/extensions/browser/url_... extensions/browser/url_request_util.cc:164: if (owner_extension != extension && !is_signin_extension) { Devlin: What do you think about whitelisting signin vs removing the whole owner_extension check for now (which you suggested on the bug)? I wasn't in that review, so I'm not sure how important the check is.
https://codereview.chromium.org/2831183003/diff/60001/extensions/browser/url_... File extensions/browser/url_request_util.cc (right): https://codereview.chromium.org/2831183003/diff/60001/extensions/browser/url_... extensions/browser/url_request_util.cc:164: if (owner_extension != extension && !is_signin_extension) { On 2017/04/25 16:52:54, Charlie Reis wrote: > Devlin: What do you think about whitelisting signin vs removing the whole > owner_extension check for now (which you suggested on the bug)? I wasn't in > that review, so I'm not sure how important the check is. Lucas knows more about the history of this check than I do. I was worried that this might be something other apps are legitimately doing, but it seems like maybe not? If that's the case, I'm fine with a whitelist approach. https://codereview.chromium.org/2831183003/diff/60001/extensions/common/api/_... File extensions/common/api/_manifest_features.json (right): https://codereview.chromium.org/2831183003/diff/60001/extensions/common/api/_... extensions/common/api/_manifest_features.json:6: // See chrome/common/extensions/api/_features.md to understand this file, as See the directions here for how to whitelist an id, though maybe moot with below. https://codereview.chromium.org/2831183003/diff/60001/extensions/common/api/_... extensions/common/api/_manifest_features.json:335: "webview": [ If we accept the fact that the signin extension is special and we need to special-case it in the permission checks already, maybe it just makes more sense to directly check everything there? (i.e., check the extension id + resource path). That way, if/when we remove it, there's only one place we need to change. And this isn't really buying us much if we have to go through the effort of treating it differently elsewhere anyway.
msarda@chromium.org changed reviewers: + xiyuan@chromium.org
xiyuan@chromium.org: Please review changes in chrome/browser/resources/gaia_auth/manifest.json https://codereview.chromium.org/2831183003/diff/60001/extensions/browser/url_... File extensions/browser/url_request_util.cc (right): https://codereview.chromium.org/2831183003/diff/60001/extensions/browser/url_... extensions/browser/url_request_util.cc:160: // even if the webview that is loading the resource does not belog to the On 2017/04/25 16:52:54, Charlie Reis wrote: > nit: belong Done. https://codereview.chromium.org/2831183003/diff/60001/extensions/browser/url_... extensions/browser/url_request_util.cc:163: extension && extension->id() == "mfffpogegjflfpflabcdkioaeobkgjik"; On 2017/04/25 16:52:54, Charlie Reis wrote: > Is there a constant we can use here rather than a magic string? Or is it in a > layer we can't access? The constant is defined in https://cs.chromium.org/chromium/src/chrome/browser/extensions/signin/gaia_au... , but that is not accessible in //extensions/. I think having the string here makes more sense than having a client/provider API as I do not think this should be something configurable by the user of extensions. https://codereview.chromium.org/2831183003/diff/60001/extensions/browser/url_... extensions/browser/url_request_util.cc:164: if (owner_extension != extension && !is_signin_extension) { On 2017/04/26 01:59:37, Devlin wrote: > On 2017/04/25 16:52:54, Charlie Reis wrote: > > Devlin: What do you think about whitelisting signin vs removing the whole > > owner_extension check for now (which you suggested on the bug)? I wasn't in > > that review, so I'm not sure how important the check is. > > Lucas knows more about the history of this check than I do. I was worried that > this might be something other apps are legitimately doing, but it seems like > maybe not? If that's the case, I'm fine with a whitelist approach. Lucas: Please advise on whether to keep this code as is or remove the it. https://codereview.chromium.org/2831183003/diff/60001/extensions/common/api/_... File extensions/common/api/_manifest_features.json (right): https://codereview.chromium.org/2831183003/diff/60001/extensions/common/api/_... extensions/common/api/_manifest_features.json:6: // See chrome/common/extensions/api/_features.md to understand this file, as On 2017/04/26 01:59:37, Devlin wrote: > See the directions here for how to whitelist an id, though maybe moot with > below. Done. https://codereview.chromium.org/2831183003/diff/60001/extensions/common/api/_... extensions/common/api/_manifest_features.json:335: "webview": [ On 2017/04/26 01:59:37, Devlin wrote: > If we accept the fact that the signin extension is special and we need to > special-case it in the permission checks already, maybe it just makes more sense > to directly check everything there? (i.e., check the extension id + resource > path). That way, if/when we remove it, there's only one place we need to > change. And this isn't really buying us much if we have to go through the > effort of treating it differently elsewhere anyway. I think the 2 things are different, so I prefer the current approach: * the whitelist in here just allows the signin extension to load success.html in a webview by making it accessible. * the hack in https://codereview.chromium.org/2831183003/diff/60001/extensions/browser/url_... is used bacause the webview that is loading success.html does not belong to the extension (this is my understanding at least). Note that this approach was proposed by Lucas in CL https://codereview.chromium.org/2838013003/ I am not an expert on extension and storage partition, so I will defer to your decision. I would just ask you to be as explicit as possible if you prefer the other option.
I am looking at how I could add a unit/browser test for this case. Just FYI, this bug was actually caught by the sync_integration_tests suite (see https://bugs.chromium.org/p/chromium/issues/detail?id=709117). But I think we do not run them on the bots with external sync service configuration.
It looks like the unit test is not trivial, I'll continue to get something but I am not sure we can have a simple test that does not require us to have a local test GAIA working. Please take a look at the current CL and let me know if it looks good. Thank you.
https://codereview.chromium.org/2831183003/diff/60001/extensions/common/api/_... File extensions/common/api/_manifest_features.json (right): https://codereview.chromium.org/2831183003/diff/60001/extensions/common/api/_... extensions/common/api/_manifest_features.json:335: "webview": [ On 2017/04/26 11:14:35, msarda wrote: > On 2017/04/26 01:59:37, Devlin wrote: > > If we accept the fact that the signin extension is special and we need to > > special-case it in the permission checks already, maybe it just makes more > sense > > to directly check everything there? (i.e., check the extension id + resource > > path). That way, if/when we remove it, there's only one place we need to > > change. And this isn't really buying us much if we have to go through the > > effort of treating it differently elsewhere anyway. > > I think the 2 things are different, so I prefer the current approach: > * the whitelist in here just allows the signin extension to load success.html in > a webview by making it accessible. > * the hack in > https://codereview.chromium.org/2831183003/diff/60001/extensions/browser/url_... > is used bacause the webview that is loading success.html does not belong to the > extension (this is my understanding at least). > Note that this approach was proposed by Lucas in CL > https://codereview.chromium.org/2838013003/ > > I am not an expert on extension and storage partition, so I will defer to your > decision. I would just ask you to be as explicit as possible if you prefer the > other option. I understand they affect different bits, but my point was more that in either case, we need to have super-special-hacks in place for the signin extension in the permission check, so it might make sense to only have one super-special-hack instead of one super-special-hack and one moderately-special-hack. :) Right now, the logic in url_request_util.cc is: // The only exception is the sign-in extension loaded by // chrome://chrome-signin, which is allowed to load web-accessible resources // even if the webview that is loading the resource does not belong to the // extension. bool is_signin_extension = extension && extension->id() == "mfffpogegjflfpflabcdkioaeobkgjik"; if (owner_extension != extension && !is_signin_extension) { *allowed = false; return true; } *allowed = WebviewInfo::IsResourceWebviewAccessible(extension, partition_id, resource_path); return true; This already requires knowledge of the signin extension and the exception it causes. We could alternatively do: bool is_signin_extension = extension && extension->id() == "mfffpogegjflfpflabcdkioaeobkgjik"; if (is_signin_extension && resource_path == "success.html") { *allowed = true; return true; } if (owner_extension != extension) { ... I don't really see it as being any less hacky than what we currently have, and it isolates a TODO to remove these in a single place.
I've patched in your CL, and today the server-side GAIA issue we were seeing yesterday seems to be fixed and I was able to successfully login with your patch. I'll defer to Devlin for the whitelisting vs hardcoding success.html, I think both points have advantages and disadvantages, but ultimately, both should work. Other than that, lgtm. https://codereview.chromium.org/2831183003/diff/60001/extensions/browser/url_... File extensions/browser/url_request_util.cc (right): https://codereview.chromium.org/2831183003/diff/60001/extensions/browser/url_... extensions/browser/url_request_util.cc:164: if (owner_extension != extension && !is_signin_extension) { On 2017/04/26 11:14:34, msarda wrote: > On 2017/04/26 01:59:37, Devlin wrote: > > On 2017/04/25 16:52:54, Charlie Reis wrote: > > > Devlin: What do you think about whitelisting signin vs removing the whole > > > owner_extension check for now (which you suggested on the bug)? I wasn't in > > > that review, so I'm not sure how important the check is. > > > > Lucas knows more about the history of this check than I do. I was worried > that > > this might be something other apps are legitimately doing, but it seems like > > maybe not? If that's the case, I'm fine with a whitelist approach. > > Lucas: Please advise on whether to keep this code as is or remove the it. We should use the whitelist approach instead of removing the check. Removing the owners check would mean that any app could load resources listed in the manifest from any other app. https://codereview.chromium.org/2831183003/diff/80001/extensions/browser/url_... File extensions/browser/url_request_util.cc (right): https://codereview.chromium.org/2831183003/diff/80001/extensions/browser/url_... extensions/browser/url_request_util.cc:158: // The only exception is the sign-in extension loaded by Please, add a TODO that this should be removed once issue 688565 is fixed.
I'd be happy with Devlin's proposal, which sounds like it would make this a more targeted hack. If that works, let's go with that. Thanks! https://codereview.chromium.org/2831183003/diff/60001/extensions/browser/url_... File extensions/browser/url_request_util.cc (right): https://codereview.chromium.org/2831183003/diff/60001/extensions/browser/url_... extensions/browser/url_request_util.cc:164: if (owner_extension != extension && !is_signin_extension) { On 2017/04/26 16:24:18, lfg wrote: > On 2017/04/26 11:14:34, msarda wrote: > > On 2017/04/26 01:59:37, Devlin wrote: > > > On 2017/04/25 16:52:54, Charlie Reis wrote: > > > > Devlin: What do you think about whitelisting signin vs removing the whole > > > > owner_extension check for now (which you suggested on the bug)? I wasn't > in > > > > that review, so I'm not sure how important the check is. > > > > > > Lucas knows more about the history of this check than I do. I was worried > > that > > > this might be something other apps are legitimately doing, but it seems like > > > maybe not? If that's the case, I'm fine with a whitelist approach. > > > > Lucas: Please advise on whether to keep this code as is or remove the it. > > We should use the whitelist approach instead of removing the check. Removing the > owners check would mean that any app could load resources listed in the manifest > from any other app. Acknowledged. https://codereview.chromium.org/2831183003/diff/60001/extensions/common/api/_... File extensions/common/api/_manifest_features.json (right): https://codereview.chromium.org/2831183003/diff/60001/extensions/common/api/_... extensions/common/api/_manifest_features.json:335: "webview": [ On 2017/04/26 16:02:30, Devlin wrote: > On 2017/04/26 11:14:35, msarda wrote: > > On 2017/04/26 01:59:37, Devlin wrote: > > > If we accept the fact that the signin extension is special and we need to > > > special-case it in the permission checks already, maybe it just makes more > > sense > > > to directly check everything there? (i.e., check the extension id + > resource > > > path). That way, if/when we remove it, there's only one place we need to > > > change. And this isn't really buying us much if we have to go through the > > > effort of treating it differently elsewhere anyway. > > > > I think the 2 things are different, so I prefer the current approach: > > * the whitelist in here just allows the signin extension to load success.html > in > > a webview by making it accessible. > > * the hack in > > > https://codereview.chromium.org/2831183003/diff/60001/extensions/browser/url_... > > is used bacause the webview that is loading success.html does not belong to > the > > extension (this is my understanding at least). > > Note that this approach was proposed by Lucas in CL > > https://codereview.chromium.org/2838013003/ > > > > I am not an expert on extension and storage partition, so I will defer to your > > decision. I would just ask you to be as explicit as possible if you prefer the > > other option. > > I understand they affect different bits, but my point was more that in either > case, we need to have super-special-hacks in place for the signin extension in > the permission check, so it might make sense to only have one super-special-hack > instead of one super-special-hack and one moderately-special-hack. :) > > Right now, the logic in url_request_util.cc is: > > // The only exception is the sign-in extension loaded by > // chrome://chrome-signin, which is allowed to load web-accessible resources > // even if the webview that is loading the resource does not belong to the > // extension. > bool is_signin_extension = > extension && extension->id() == "mfffpogegjflfpflabcdkioaeobkgjik"; > if (owner_extension != extension && !is_signin_extension) { > *allowed = false; > return true; > } > > *allowed = WebviewInfo::IsResourceWebviewAccessible(extension, partition_id, > resource_path); > return true; > > This already requires knowledge of the signin extension and the exception it > causes. We could alternatively do: > > bool is_signin_extension = > extension && extension->id() == "mfffpogegjflfpflabcdkioaeobkgjik"; > if (is_signin_extension && resource_path == "success.html") { > *allowed = true; > return true; > } > > if (owner_extension != extension) { > ... > > I don't really see it as being any less hacky than what we currently have, and > it isolates a TODO to remove these in a single place. I like this proposal-- thanks for elaborating on what it would look like.
The CQ bit was checked by msarda@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2831183003/diff/60001/extensions/common/api/_... File extensions/common/api/_manifest_features.json (right): https://codereview.chromium.org/2831183003/diff/60001/extensions/common/api/_... extensions/common/api/_manifest_features.json:335: "webview": [ On 2017/04/26 20:10:25, Charlie Reis wrote: > On 2017/04/26 16:02:30, Devlin wrote: > > On 2017/04/26 11:14:35, msarda wrote: > > > On 2017/04/26 01:59:37, Devlin wrote: > > > > If we accept the fact that the signin extension is special and we need to > > > > special-case it in the permission checks already, maybe it just makes more > > > sense > > > > to directly check everything there? (i.e., check the extension id + > > resource > > > > path). That way, if/when we remove it, there's only one place we need to > > > > change. And this isn't really buying us much if we have to go through the > > > > effort of treating it differently elsewhere anyway. > > > > > > I think the 2 things are different, so I prefer the current approach: > > > * the whitelist in here just allows the signin extension to load > success.html > > in > > > a webview by making it accessible. > > > * the hack in > > > > > > https://codereview.chromium.org/2831183003/diff/60001/extensions/browser/url_... > > > is used bacause the webview that is loading success.html does not belong to > > the > > > extension (this is my understanding at least). > > > Note that this approach was proposed by Lucas in CL > > > https://codereview.chromium.org/2838013003/ > > > > > > I am not an expert on extension and storage partition, so I will defer to > your > > > decision. I would just ask you to be as explicit as possible if you prefer > the > > > other option. > > > > I understand they affect different bits, but my point was more that in either > > case, we need to have super-special-hacks in place for the signin extension in > > the permission check, so it might make sense to only have one > super-special-hack > > instead of one super-special-hack and one moderately-special-hack. :) > > > > Right now, the logic in url_request_util.cc is: > > > > // The only exception is the sign-in extension loaded by > > // chrome://chrome-signin, which is allowed to load web-accessible resources > > // even if the webview that is loading the resource does not belong to the > > // extension. > > bool is_signin_extension = > > extension && extension->id() == "mfffpogegjflfpflabcdkioaeobkgjik"; > > if (owner_extension != extension && !is_signin_extension) { > > *allowed = false; > > return true; > > } > > > > *allowed = WebviewInfo::IsResourceWebviewAccessible(extension, partition_id, > > resource_path); > > return true; > > > > This already requires knowledge of the signin extension and the exception it > > causes. We could alternatively do: > > > > bool is_signin_extension = > > extension && extension->id() == "mfffpogegjflfpflabcdkioaeobkgjik"; > > if (is_signin_extension && resource_path == "success.html") { > > *allowed = true; > > return true; > > } > > > > if (owner_extension != extension) { > > ... > > > > I don't really see it as being any less hacky than what we currently have, and > > it isolates a TODO to remove these in a single place. > > I like this proposal-- thanks for elaborating on what it would look like. Done. https://codereview.chromium.org/2831183003/diff/80001/extensions/browser/url_... File extensions/browser/url_request_util.cc (right): https://codereview.chromium.org/2831183003/diff/80001/extensions/browser/url_... extensions/browser/url_request_util.cc:158: // The only exception is the sign-in extension loaded by On 2017/04/26 16:24:18, lfg wrote: > Please, add a TODO that this should be removed once issue 688565 is fixed. Done.
lgtm. Just checking - you've confirmed this is sufficient to fix the bug?
On 2017/04/27 13:43:58, Devlin wrote: > lgtm. Just checking - you've confirmed this is sufficient to fix the bug? Yes.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by msarda@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
msarda@chromium.org changed reviewers: + rogerta@chromium.org
Roger: May I ask you to take a look at the change in chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc ? Thank you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2831183003/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc (right): https://codereview.chromium.org/2831183003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc:890: ASSERT_TRUE(content::ExecuteScriptAndExtractString( I don't think this waits for 'loadcommit' handler to send back message. It would return after the script is executed. We need to manually wait for the expected message to return. i.e. content::DOMMessageQueue message_queue; ASSERT_TRUE(content::ExecuteScriptAndExtractString(...)); std::string message; do { ASSERT_TRUE(message_queue.WaitForMessage(&message)); } while (message != "\"success_page_loaded\"");
https://codereview.chromium.org/2831183003/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc (right): https://codereview.chromium.org/2831183003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc:890: ASSERT_TRUE(content::ExecuteScriptAndExtractString( On 2017/04/27 15:55:45, xiyuan wrote: > I don't think this waits for 'loadcommit' handler to send back message. It would > return after the script is executed. We need to manually wait for the expected > message to return. > > i.e. > content::DOMMessageQueue message_queue; > ASSERT_TRUE(content::ExecuteScriptAndExtractString(...)); > std::string message; > do { > ASSERT_TRUE(message_queue.WaitForMessage(&message)); > } while (message != "\"success_page_loaded\""); > It does wait to get a response message in the content::DOMMessageQueue (see https://cs.chromium.org/chromium/src/content/public/test/browser_test_utils.c...). In my tests, this waits as expected. This is the same approach we're using for WaitUntilUIReady(browser());
https://codereview.chromium.org/2831183003/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc (right): https://codereview.chromium.org/2831183003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc:890: ASSERT_TRUE(content::ExecuteScriptAndExtractString( On 2017/04/27 15:59:39, msarda wrote: > On 2017/04/27 15:55:45, xiyuan wrote: > > I don't think this waits for 'loadcommit' handler to send back message. It > would > > return after the script is executed. We need to manually wait for the expected > > message to return. > > > > i.e. > > content::DOMMessageQueue message_queue; > > ASSERT_TRUE(content::ExecuteScriptAndExtractString(...)); > > std::string message; > > do { > > ASSERT_TRUE(message_queue.WaitForMessage(&message)); > > } while (message != "\"success_page_loaded\""); > > > > It does wait to get a response message in the content::DOMMessageQueue (see > https://cs.chromium.org/chromium/src/content/public/test/browser_test_utils.c...). > In my tests, this waits as expected. > > This is the same approach we're using for WaitUntilUIReady(browser()); Okay. At least, we should add an ASSERT_EQ to guarantee |message| is the expected "success_page_loaded".
LGTM! https://codereview.chromium.org/2831183003/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc (right): https://codereview.chromium.org/2831183003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc:890: ASSERT_TRUE(content::ExecuteScriptAndExtractString( On 2017/04/27 16:16:24, xiyuan wrote: > On 2017/04/27 15:59:39, msarda wrote: > > On 2017/04/27 15:55:45, xiyuan wrote: > > > I don't think this waits for 'loadcommit' handler to send back message. It > > would > > > return after the script is executed. We need to manually wait for the > expected > > > message to return. > > > > > > i.e. > > > content::DOMMessageQueue message_queue; > > > ASSERT_TRUE(content::ExecuteScriptAndExtractString(...)); > > > std::string message; > > > do { > > > ASSERT_TRUE(message_queue.WaitForMessage(&message)); > > > } while (message != "\"success_page_loaded\""); > > > > > > > It does wait to get a response message in the content::DOMMessageQueue (see > > > https://cs.chromium.org/chromium/src/content/public/test/browser_test_utils.c...). > > In my tests, this waits as expected. > > > > This is the same approach we're using for WaitUntilUIReady(browser()); > > Okay. At least, we should add an ASSERT_EQ to guarantee |message| is the > expected "success_page_loaded". Agreed-- no need to loop, but verifying the right message came through is worthwhile.
https://codereview.chromium.org/2831183003/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc (right): https://codereview.chromium.org/2831183003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc:890: ASSERT_TRUE(content::ExecuteScriptAndExtractString( On 2017/04/27 16:16:24, xiyuan wrote: > On 2017/04/27 15:59:39, msarda wrote: > > On 2017/04/27 15:55:45, xiyuan wrote: > > > I don't think this waits for 'loadcommit' handler to send back message. It > > would > > > return after the script is executed. We need to manually wait for the > expected > > > message to return. > > > > > > i.e. > > > content::DOMMessageQueue message_queue; > > > ASSERT_TRUE(content::ExecuteScriptAndExtractString(...)); > > > std::string message; > > > do { > > > ASSERT_TRUE(message_queue.WaitForMessage(&message)); > > > } while (message != "\"success_page_loaded\""); > > > > > > > It does wait to get a response message in the content::DOMMessageQueue (see > > > https://cs.chromium.org/chromium/src/content/public/test/browser_test_utils.c...). > > In my tests, this waits as expected. > > > > This is the same approach we're using for WaitUntilUIReady(browser()); > > Okay. At least, we should add an ASSERT_EQ to guarantee |message| is the > expected "success_page_loaded". Done (I had it locally and I think I removed that by mistake. Thank you for catching it.)
The CQ bit was checked by msarda@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by msarda@chromium.org
The CQ bit was checked by msarda@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lfg@chromium.org, rdevlin.cronin@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2831183003/#ps140001 (title: "Add missing expect")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493314425832200,
"parent_rev": "04181130ec65e64d013c43d3e1bee71237c5fa75", "commit_rev":
"8fdab62089307f5fe2307335dd1c71c4c6661867"}
Message was sent while issue was closed.
Description was changed from ========== Fix loading success.html at the end of chrome://chrome-signin flow This CL lists success.html as an webview accessible resource for the sign-in extension. This CL adds an exception for sign-in extension when for loading webview accessible resources. BUG=709117 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix loading success.html at the end of chrome://chrome-signin flow This CL lists success.html as an webview accessible resource for the sign-in extension. This CL adds an exception for sign-in extension when for loading webview accessible resources. BUG=709117 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2831183003 Cr-Commit-Position: refs/heads/master@{#467739} Committed: https://chromium.googlesource.com/chromium/src/+/8fdab62089307f5fe2307335dd1c... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/8fdab62089307f5fe2307335dd1c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
