Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2432933004:
Adjust HTTP-bad console messages (Closed)
DescriptionAdjust HTTP-bad console messages
When I added a console message originally, I had missed a later comment
in the bug suggesting that we use different messages for before and
after launch. Thus this CL uses one console message for when the omnibox
warning is actually showing, and one for when it's not but will be in
the future.
BUG=647561
Committed: https://crrev.com/19e9536a6e51332042bc9824121ca0b37fef41ae
Cr-Commit-Position: refs/heads/master@{#426659}
Patch Set 1 #
Total comments: 12
Patch Set 2 : elawrence comments #
Total comments: 2
Patch Set 3 : elawrence comment #
Total comments: 1
Messages
Total messages: 29 (17 generated)
The CQ bit was checked by estark@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...
estark@chromium.org changed reviewers: + elawrence@chromium.org
elawrence, PTAL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2432933004/diff/1/chrome/browser/ssl/chrome_s... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2432933004/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client.cc:338: security_state::SecurityStateModel::HTTP_SHOW_WARNING) { Is the reason that we don't check displayed_private_user_data_input_on_http here that the only way we light up HTTP_SHOW_WARNING today is when that's set? If so, will this change in the future? Should we assert that expectation e.g. DCHECK(displayed_private_user_data_input_on_http)? https://codereview.chromium.org/2432933004/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client.cc:341: "This page includes a password or credit card input over HTTP. A " I worry that "over HTTP" is unnecessarily specific; maybe "in a non-secure context" to cover cases of an HTTPS iframe hosted by a HTTP/FTP/FILE/DATA-sourced page? https://codereview.chromium.org/2432933004/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client.cc:343: "https://goo.gl/zmWq3m. For feedback, please use " What kind of "feedback" are we looking for here? Leaving it open could well mean the bug immediately gets spammed with all manner of junk and we have to close it for comments? As I understand things from EmilySch, we're going to point the goo.gl link at the Developer Guidance doc, and we can update that to include the link to the CRBug? https://codereview.chromium.org/2432933004/diff/1/chrome/browser/ssl/chrome_s... File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc (right): https://codereview.chromium.org/2432933004/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:1166: // of being too brittle) but rather just a keyword ("not secure"). This comment is no longer correct. Given the shortness of the test, maybe just say "To avoid brittleness, we look for keywords in the warning rather than the exact text" or something? https://codereview.chromium.org/2432933004/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:1178: // a keyword ("not secure" and "M56"). This comment is no longer correct. Given the shortness of the test, maybe just say "To avoid brittleness, we look for keywords in the warning rather than the exact text" or something? https://codereview.chromium.org/2432933004/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:1310: // in one additional console message. Can we assert that this second trigger on the same page (without navigation) does not add any message to the console? E.g. by changing this code to: // Fire a second notification contents->OnCreditCardInputShownOnHttp(); // Ensure it didn't add another console warning ASSERT_NO_FATAL_FAILURE(CheckForOneFutureHttpWarningConsoleMessage(delegate)); delegate->ClearConsoleMessages(); That way there's no chance that the warning we're looking for from loading /title2.html actually came from the second trigger on the first page.
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Thanks, Eric. https://codereview.chromium.org/2432933004/diff/1/chrome/browser/ssl/chrome_s... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2432933004/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client.cc:338: security_state::SecurityStateModel::HTTP_SHOW_WARNING) { On 2016/10/20 17:18:29, elawrence wrote: > Is the reason that we don't check displayed_private_user_data_input_on_http here > that the only way we light up HTTP_SHOW_WARNING today is when that's set? If so, > will this change in the future? > > Should we assert that expectation e.g. > DCHECK(displayed_private_user_data_input_on_http)? It initially seemed redundant to me, but I think you're right that we should check it to be future-proof. Now I'm early-returning if displayed_private_user_data_input_on_http is false. https://codereview.chromium.org/2432933004/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client.cc:341: "This page includes a password or credit card input over HTTP. A " On 2016/10/20 17:18:29, elawrence wrote: > I worry that "over HTTP" is unnecessarily specific; maybe "in a non-secure > context" to cover cases of an HTTPS iframe hosted by a > HTTP/FTP/FILE/DATA-sourced page? Done. https://codereview.chromium.org/2432933004/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client.cc:343: "https://goo.gl/zmWq3m. For feedback, please use " On 2016/10/20 17:18:29, elawrence wrote: > What kind of "feedback" are we looking for here? Leaving it open could well mean > the bug immediately gets spammed with all manner of junk and we have to close it > for comments? > > As I understand things from EmilySch, we're going to point the goo.gl link at > the Developer Guidance doc, and we can update that to include the link to the > CRBug? So you mean that we should delete the bug link because the goo.gl link will link to it? If so, done. https://codereview.chromium.org/2432933004/diff/1/chrome/browser/ssl/chrome_s... File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc (right): https://codereview.chromium.org/2432933004/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:1166: // of being too brittle) but rather just a keyword ("not secure"). On 2016/10/20 17:18:29, elawrence wrote: > This comment is no longer correct. Given the shortness of the test, maybe just > say "To avoid brittleness, we look for keywords in the warning rather than the > exact text" or something? Ugh, my comment about not being brittle was itself brittle. Done. https://codereview.chromium.org/2432933004/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:1178: // a keyword ("not secure" and "M56"). On 2016/10/20 17:18:29, elawrence wrote: > This comment is no longer correct. Given the shortness of the test, maybe just > say "To avoid brittleness, we look for keywords in the warning rather than the > exact text" or something? Done. https://codereview.chromium.org/2432933004/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:1310: // in one additional console message. On 2016/10/20 17:18:29, elawrence wrote: > Can we assert that this second trigger on the same page (without navigation) > does not add any message to the console? E.g. by changing this code to: > > // Fire a second notification > contents->OnCreditCardInputShownOnHttp(); > > // Ensure it didn't add another console warning > ASSERT_NO_FATAL_FAILURE(CheckForOneFutureHttpWarningConsoleMessage(delegate)); > > delegate->ClearConsoleMessages(); > > That way there's no chance that the warning we're looking for from loading > /title2.html actually came from the second trigger on the first page. That would be giving us a false sense of correctness, I think. The notification about a console message being added is async, so checking for another console warning immediately after firing the notification isn't useful -- even if the second sensitive input had triggered a console message, the test WebContentsDelegate wouldn't have heard about it until the async "console message added" arrives. And there's no way for the test to wait to check that that message *never* arrives. I think to really test this properly, we'd have to add something to content::MockRenderProcessHost to expose all the console message IPCs that have been sent, so that we can synchronously test that a console message has or has not been sent. Which is doable, but not worth blocking this CL on, IMO.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm. https://codereview.chromium.org/2432933004/diff/20001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2432933004/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:356: "more information, see https://goo.gl/zmWq3m."); Is this more clear, or less? string sWarning; switch (security_info.security_level) { case security_state::SecurityStateModel::NONE: sWarning = ".... will be added..."; break; case security_state::SecurityStateModel::HTTP_SHOW_WARNING: sWarning = "... has been added..."; break; default: return; } web_contents_->GetMainFrame()->AddMessageToConsole( content::CONSOLE_MESSAGE_LEVEL_WARNING, sWarning); logged_http_warning_on_current_navigation_ = true;
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 estark@chromium.org to run a CQ dry run
estark@chromium.org changed reviewers: + lgarron@chromium.org
lgarron, PTAL? https://codereview.chromium.org/2432933004/diff/20001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2432933004/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:356: "more information, see https://goo.gl/zmWq3m."); On 2016/10/20 18:56:13, elawrence wrote: > Is this more clear, or less? > > string sWarning; > > switch (security_info.security_level) { > case security_state::SecurityStateModel::NONE: > sWarning = ".... will be added..."; > break; > case security_state::SecurityStateModel::HTTP_SHOW_WARNING: > sWarning = "... has been added..."; > break; > default: > return; > } > > web_contents_->GetMainFrame()->AddMessageToConsole( > content::CONSOLE_MESSAGE_LEVEL_WARNING, > sWarning); > logged_http_warning_on_current_navigation_ = true; Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM, although I can only honestly say I understand 2/3 of the test code well. https://codereview.chromium.org/2432933004/diff/40001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2432933004/diff/40001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:343: warning = I was going to complain that we shouldn't define these strings inline, but we seem to be doing so throughout in this file. So #yolo?
On 2016/10/21 00:01:12, lgarron wrote: > LGTM, although I can only honestly say I understand 2/3 of the test code well. Is there anything in particular I can clarify? > > https://codereview.chromium.org/2432933004/diff/40001/chrome/browser/ssl/chro... > File chrome/browser/ssl/chrome_security_state_model_client.cc (right): > > https://codereview.chromium.org/2432933004/diff/40001/chrome/browser/ssl/chro... > chrome/browser/ssl/chrome_security_state_model_client.cc:343: warning = > I was going to complain that we shouldn't define these strings inline, but we > seem to be doing so throughout in this file. So #yolo?
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from elawrence@chromium.org Link to the patchset: https://codereview.chromium.org/2432933004/#ps40001 (title: "elawrence comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
On 2016/10/21 at 00:35:01, commit-bot wrote: > Committed patchset #3 (id:40001) Nah, I just don't know much about RunLoops, and I don't know enough about tab strip control to find anything wrong with the test code. But someone who knows them better might. ;-)
Message was sent while issue was closed.
On 2016/10/21 01:10:19, lgarron wrote: > On 2016/10/21 at 00:35:01, commit-bot wrote: > > Committed patchset #3 (id:40001) > > Nah, I just don't know much about RunLoops, and I don't know enough about tab > strip control to find anything wrong with the test code. But someone who knows > them better might. ;-) It follows the same template as the other console message tests I added in https://codereview.chromium.org/2410043003/ which somehow ended up with about 2000 reviewers, so I feel okay about it. The gist of it is that when ChromeSSMClient logs a console message, it is sent as an IPC to Blink, which logs the message to the console and then sends a notification back up to the browser process that a console message has been added. This notification back to the browser process eventually gets routed to the WebContentsDelegate interface, which is implemented by Browser. So these tests subclass Browser to create a new WebContentsDelegate that records the console messages that are added. The tests create a new tab (which is what gets inserted into the TabStripModel) to set as its delegate one of these subclassed WebContentsDelegate that spy on console messages. Then the tests trigger navigations and check that the expected console message has been added. The RunLoops are basically waiting for these async notifications that a console message has been added. (Also see https://crbug.com/658099 which I filed based on the discussion with Eric earlier in this CL, at https://codereview.chromium.org/2432933004/diff/1/chrome/browser/ssl/chrome_s.... That would be a nice cleanup if either of you want to grab it before I get to it.)
Message was sent while issue was closed.
Description was changed from ========== Adjust HTTP-bad console messages When I added a console message originally, I had missed a later comment in the bug suggesting that we use different messages for before and after launch. Thus this CL uses one console message for when the omnibox warning is actually showing, and one for when it's not but will be in the future. BUG=647561 ========== to ========== Adjust HTTP-bad console messages When I added a console message originally, I had missed a later comment in the bug suggesting that we use different messages for before and after launch. Thus this CL uses one console message for when the omnibox warning is actually showing, and one for when it's not but will be in the future. BUG=647561 Committed: https://crrev.com/19e9536a6e51332042bc9824121ca0b37fef41ae Cr-Commit-Position: refs/heads/master@{#426659} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/19e9536a6e51332042bc9824121ca0b37fef41ae Cr-Commit-Position: refs/heads/master@{#426659} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
