New issue
Advanced search Search tips

Issue 844178 link

Starred by 6 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 268640



Sign in to add a comment

Avoid spamming the console with CORB warning for 204s, empty responses, etc.

Project Member Reported by lukasza@chromium.org, May 17 2018

Issue description

REPRO STEPS:
1. Visit https://vnexpress.net/
2. Look at the DevTools javascript console

EXPECTED BEHAVIOR: not too much noise

ACTUAL BEHAVIOR: slow, but constant trickle of CORB messages, most of which are benign (either a 204 or a Content-Length: 0).
 
Note that CORB should still block 204s and Content-Length: 0 responses (since CORB needs to strip down the response headers), but it seems that reporting such responses to the DevTools console is excessive.
Cc: creis@chromium.org pfeldman@chromium.org
+pfeldman@ who has kindly helped with r526624 (no good deed goes unpunished :-)

If we want to suppress the CORB console message in some cases, I think we can do the following:

1. Extract the code for calculating the need to report the blocking into network::CrossOriginReadBlocking::ResponseAnalyzer which already has an opportunity to learn about the responses status (206?) and headers (Content-Length: 0)?

2. Actually change whether the console message is emitted or not.  Options I see:

2.1. Change when info->set_blocked_cross_site_document(true) is called from CrossSiteDocumentResourceHandler::OnReadCompleted (and/or from URLLoader::NotifyCompleted in the future - see  issue 792546 ).  This is probably simplest to implement, but it seems undesirable to set blocked_cross_site_document to false if 1) blocking (of response headers) actually took plance, and 2) we don't want to emit a console message (because the response body was empty and so wasn't affected by CORB blocking).

2.2. Same as 2.1, but rename 2.1 to should_report_corb_blocking.  This is probably better, but requires changing quite a bit of places in code to plumb through the newly named field/value.

2.3. Check 206 status and Content-Length inside third_party/blink/renderer/devtools/front_end/sdk/NetworkManager.js, in _finishNetworkRequest.  This is tricky to implement, because CORB strips out Content-Length header today (I think this is desirable behavior going forward, despite [CORS-focused!] issue 841308).

3. We can also consider word-smithing the console message a bit more.
   - Current msg: Blocked current origin from receiving cross-site document at %s with MIME type %s.
   - Problem: "site" is inaccurate
   - Problem: we stopped using "document" in the explainer because of feedback opposing resource-vs-document distinction
   - Problem: no follow-up;  maybe we can link to explainer / fetch spec / future-what-can-i-do-if-corb-errors-appear-on-my-web-page page?  fetch spec URL would probably be the most stable of these options...


WDYT?  I guess 1 + 2.2 + 3 is the way to go (we can consider splitting 3 into a separate CL, but meh...).  If this sounds good, then can we agree on the new field name before I go and make changes in all the files?  Does |should_report_corb_blocking| look okay?
Labels: Target-68
Status: Started (was: Assigned)
WIP CL @ https://crrev.com/c/1070551

I have some trouble with writing a test - I would appreciate any hints how one can author a browser test that verifies presence of warnings in DevTools console (ConsoleObserverDelegate can be used for console.log messages, but AFAICT won't intercept messages generated by DevTools frontend).
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 6 2018

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

commit bf576b0f6991a855a989864b1e79fcb209dd75a1
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Wed Jun 06 17:47:31 2018

Don't spam the console with CORB warning for 204s and empty responses.

After this CL, the CORB warning should be suppressed for:
- Responses with 'Content-Length: 0' response header
- Responses with 204 http status.

Bug:  844178 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I320c9e221d94c3deeef26b96a5ff70b41ea1c2a8
Reviewed-on: https://chromium-review.googlesource.com/1070551
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Pavel Feldman <pfeldman@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564956}
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/content/browser/loader/cross_site_document_blocking_browsertest.cc
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/content/browser/loader/cross_site_document_resource_handler.cc
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/content/browser/loader/cross_site_document_resource_handler.h
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/content/browser/loader/mojo_async_resource_handler.cc
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/content/browser/loader/resource_request_info_impl.cc
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/content/browser/loader/resource_request_info_impl.h
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/content/renderer/loader/web_url_loader_impl.cc
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/content/renderer/loader/web_url_loader_impl_unittest.cc
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/services/network/cross_origin_read_blocking.cc
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/services/network/cross_origin_read_blocking.h
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/services/network/public/cpp/network_ipc_param_traits.h
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/services/network/public/cpp/resource_response.cc
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/services/network/public/cpp/resource_response_info.cc
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/services/network/public/cpp/resource_response_info.h
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/services/network/public/cpp/url_loader_completion_status.cc
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/services/network/public/cpp/url_loader_completion_status.h
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/services/network/url_loader.cc
[delete] https://crrev.com/76e7ac4c055ddd315dac46827b7db248c1369a2d/third_party/WebKit/LayoutTests/flag-specific/site-per-process/http/tests/inspector-protocol/network/block_cross_site_document_load-expected.txt
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/block_cross_site_document_load-expected.txt
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/block_cross_site_document_load.js
[add] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/resources/204.pl
[add] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/resources/404.pl
[add] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/resources/content-length-0.pl
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/resources/nosniff.pl
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/blink/public/platform/web_url_loader_client.h
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/blink/renderer/core/inspector/browser_protocol.pdl
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/blink/renderer/core/inspector/inspector_network_agent.cc
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/blink/renderer/core/inspector/inspector_network_agent.h
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/blink/renderer/core/inspector/inspector_trace_events.cc
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/blink/renderer/core/inspector/inspector_trace_events.h
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/blink/renderer/core/loader/frame_fetch_context.cc
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/blink/renderer/core/loader/frame_fetch_context.h
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/blink/renderer/core/loader/worker_fetch_context.cc
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/blink/renderer/core/loader/worker_fetch_context.h
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/blink/renderer/core/probe/CoreProbes.pidl
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/blink/renderer/devtools/front_end/sdk/NetworkManager.js
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/blink/renderer/platform/loader/fetch/fetch_context.h
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.h
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/blink/renderer/platform/loader/fetch/resource_loader.cc
[modify] https://crrev.com/bf576b0f6991a855a989864b1e79fcb209dd75a1/third_party/blink/renderer/platform/loader/fetch/resource_loader.h

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 9 2018

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

commit 0b7134640709f1ed918ddd8f8d2e7ec0c382c4d9
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Sat Jun 09 00:57:21 2018

Don't spam the console with CORB warning for 4xx and 5xx responses.

Bug:  844178 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I603897af8c6b4223c55ccefa0e70aa77bf17cd41
Reviewed-on: https://chromium-review.googlesource.com/1089120
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565803}
[modify] https://crrev.com/0b7134640709f1ed918ddd8f8d2e7ec0c382c4d9/services/network/cross_origin_read_blocking.cc
[modify] https://crrev.com/0b7134640709f1ed918ddd8f8d2e7ec0c382c4d9/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/block_cross_site_document_load-expected.txt
[modify] https://crrev.com/0b7134640709f1ed918ddd8f8d2e7ec0c382c4d9/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/block_cross_site_document_load.js

Status: Fixed (was: Started)
I think this can be marked as fixed.  In theory we might also want to avoid the warning for empty responses that do *not* have a "Content-Length: 0" response header, but 1) such responses seem infrequent (armchair analysis / no hard data) and 2) detecting such responses means extra complexity and latency (e.g. in case when CORB decides to block purely based on the headers, without looking at [and waiting for] the response body).
 Issue 859258  has been merged into this issue.
 Issue 870225  has been merged into this issue.
Labels: -Target-68 Target-69
 Issue 873909  has been merged into this issue.
 Issue 875947  has been merged into this issue.

Sign in to add a comment