Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2574063003:
Move ResourceHandler deferred actions ahead of external protocol handling. (Closed)
DescriptionMove ResourceHandler deferred actions ahead of external protocol handling.
While working on moving mixed-content checks to the browser
(https://codereview.chromium.org/1905033002) I found out that external protocol
handling happened too early in the processing of request start and redirects
done by ResourceLoader; namely before ResourceHandler implementations are
notified and able to do their work in deferred tasks.
This CL changes that order to allow the upcoming mixed-content
NavigationThrottle implementation to properly check the navigation start or
redirect ahead of external protocol dispatching.
For the record, the failing layout test that brought this change to light was:
http/tests/security/mixedContent/nonwebby-scheme-in-iframe-allowed.https.html
BUG=576270
Committed: https://crrev.com/94bd59f28dcf81289f09c39d189128254631b275
Cr-Commit-Position: refs/heads/master@{#439923}
Patch Set 1 #Patch Set 2 : Reverts URLRequest changes; redirect bugfix; improved unit tests. #
Total comments: 10
Patch Set 3 : Adds external protocol handling tests for sync and async cases. #
Total comments: 3
Messages
Total messages: 40 (21 generated)
The CQ bit was checked by carlosk@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...
carlosk@chromium.org changed reviewers: + jam@chromium.org
jam@: PTAL.
On 2016/12/14 03:28:42, carlosk wrote: > jam@: PTAL. I don't want to add more monitoring-intermediary-state APIs to URLRequest. It's led us into a lot of dark and scary places with URLRequestJob::Status. Can we just have ResourceLoader cache the URL? Also, what ResourceThrottle/ResourceHandler code is supposed to rewrite the URL in the failing test? I'm not even sure they can do so. NetworkDelegate can rewrite redirect, but it happens before the ResourceLoader is informed of the redirect. URLRequest has no methods on it to change the URL of a request, nor does the URLRequest::Delegate have that ability, I believe.
On 2016/12/14 03:52:24, mmenke wrote: > On 2016/12/14 03:28:42, carlosk wrote: > > jam@: PTAL. > > I don't want to add more monitoring-intermediary-state APIs to URLRequest. It's > led us into a lot of dark and scary places with URLRequestJob::Status. Can we > just have ResourceLoader cache the URL? Oh, right, the bug indicates something is changing the URL, but I don't see how this can happen. > Also, what ResourceThrottle/ResourceHandler code is supposed to rewrite the URL > in the failing test? I'm not even sure they can do so. NetworkDelegate can > rewrite redirect, but it happens before the ResourceLoader is informed of the > redirect. URLRequest has no methods on it to change the URL of a request, nor > does the URLRequest::Delegate have that ability, I believe.
On 2016/12/14 03:53:20, mmenke wrote: > On 2016/12/14 03:52:24, mmenke wrote: > > On 2016/12/14 03:28:42, carlosk wrote: > > > jam@: PTAL. > > > > I don't want to add more monitoring-intermediary-state APIs to URLRequest. > It's > > led us into a lot of dark and scary places with URLRequestJob::Status. Can we > > just have ResourceLoader cache the URL? I'll look into that. But just making sure I understand: your concern is with the "getter" for RedirectInfo that I added, correct? > > Oh, right, the bug indicates something is changing the URL, but I don't see how > this can happen. > > > Also, what ResourceThrottle/ResourceHandler code is supposed to rewrite the > URL > > in the failing test? I'm not even sure they can do so. NetworkDelegate can > > rewrite redirect, but it happens before the ResourceLoader is informed of the > > redirect. URLRequest has no methods on it to change the URL of a request, nor > > does the URLRequest::Delegate have that ability, I believe. What I meant to say is that the deferred redirect with change the URL that will finally be passed to the external protocol handler. It won't change anything in the URLRequest.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry for the delay. Didn't get an email about your comment. Do feel free to ping me with another comment on the bug or via IM. On 2016/12/14 04:04:43, carlosk wrote: > On 2016/12/14 03:53:20, mmenke wrote: > > On 2016/12/14 03:52:24, mmenke wrote: > > > On 2016/12/14 03:28:42, carlosk wrote: > > > > jam@: PTAL. > > > > > > I don't want to add more monitoring-intermediary-state APIs to URLRequest. > > It's > > > led us into a lot of dark and scary places with URLRequestJob::Status. Can > we > > > just have ResourceLoader cache the URL? > > I'll look into that. But just making sure I understand: your concern is with the > "getter" for RedirectInfo that I added, correct? Yes, it's the additional getter. The same approach with caching the URL in the ResourceLoader would satisfy that concern. > > Oh, right, the bug indicates something is changing the URL, but I don't see > how > > this can happen. > > > > > Also, what ResourceThrottle/ResourceHandler code is supposed to rewrite the > > URL > > > in the failing test? I'm not even sure they can do so. NetworkDelegate can > > > rewrite redirect, but it happens before the ResourceLoader is informed of > the > > > redirect. URLRequest has no methods on it to change the URL of a request, > nor > > > does the URLRequest::Delegate have that ability, I believe. > > What I meant to say is that the deferred redirect with change the URL that will > finally be passed to the external protocol handler. It won't change anything in > the URLRequest. Ah, so we're using the original (pre-redirect) URL? The old HandleExternalProtocol(this, redirect_info.new_url) looks like it's using the right URL. We should have a browsertest for this. This would also help me better understand what's being fixed.
On 2016/12/15 16:20:04, mmenke (Out Dec 17 to Jan 2) wrote: > Sorry for the delay. Didn't get an email about your comment. Do feel free to > ping me with another comment on the bug or via IM. > > On 2016/12/14 04:04:43, carlosk wrote: > > On 2016/12/14 03:53:20, mmenke wrote: > > > On 2016/12/14 03:52:24, mmenke wrote: > > > > On 2016/12/14 03:28:42, carlosk wrote: > > > > > jam@: PTAL. > > > > > > > > I don't want to add more monitoring-intermediary-state APIs to URLRequest. > > > > It's > > > > led us into a lot of dark and scary places with URLRequestJob::Status. > Can > > we > > > > just have ResourceLoader cache the URL? > > > > I'll look into that. But just making sure I understand: your concern is with > the > > "getter" for RedirectInfo that I added, correct? > > Yes, it's the additional getter. The same approach with caching the URL in the > ResourceLoader would satisfy that concern. > > > > Oh, right, the bug indicates something is changing the URL, but I don't see > > how > > > this can happen. > > > > > > > Also, what ResourceThrottle/ResourceHandler code is supposed to rewrite > the > > > URL > > > > in the failing test? I'm not even sure they can do so. NetworkDelegate > can > > > > rewrite redirect, but it happens before the ResourceLoader is informed of > > the > > > > redirect. URLRequest has no methods on it to change the URL of a request, > > nor > > > > does the URLRequest::Delegate have that ability, I believe. > > > > What I meant to say is that the deferred redirect with change the URL that > will > > finally be passed to the external protocol handler. It won't change anything > in > > the URLRequest. > > Ah, so we're using the original (pre-redirect) URL? The old > HandleExternalProtocol(this, redirect_info.new_url) looks like it's using the > right URL. > > We should have a browsertest for this. This would also help me better > understand what's being fixed. Or better, a unit test, if one can reasonably test for the behavior we want. I don't know what API is being passed the wrong data, and a unit tests would be better at making that clear.
Description was changed from ========== Change when ResourceLoader handles external protocols. While working on moving mixed-content checks to the browser (https://codereview.chromium.org/1905033002) a layout test was failing due to external protocol handling happening too early in the request processing done by ResourceLoader; namely before deferred redirects that could change the URL to be handled. This CL changes that order to allow deferred actions to happen before external protocol handling. For the record, the failing layout test was: http/tests/security/mixedContent/nonwebby-scheme-in-iframe-allowed.https.html BUG=576270 ========== to ========== Change when ResourceLoader handles external protocols. While working on moving mixed-content checks to the browser (https://codereview.chromium.org/1905033002) a layout test was failing due to external protocol handling happening too early in the request processing done by ResourceLoader; namely before deferred redirects that could change the URL to be handled. This CL changes that order to allow deferred actions to happen before external protocol handling. For the record, the failing layout test was: http/tests/security/mixedContent/nonwebby-scheme-in-iframe-allowed.https.html BUG=576270 ==========
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
And on a side note, figured out why I didn't see your response - commenting on a CL you're CCed on seems to remove you from the CC list, without adding you as a review. That's happened to me before. :(
Description was changed from ========== Change when ResourceLoader handles external protocols. While working on moving mixed-content checks to the browser (https://codereview.chromium.org/1905033002) a layout test was failing due to external protocol handling happening too early in the request processing done by ResourceLoader; namely before deferred redirects that could change the URL to be handled. This CL changes that order to allow deferred actions to happen before external protocol handling. For the record, the failing layout test was: http/tests/security/mixedContent/nonwebby-scheme-in-iframe-allowed.https.html BUG=576270 ========== to ========== Move ResourceHandler deferred actions ahead of external protocol handling. While working on moving mixed-content checks to the browser (https://codereview.chromium.org/1905033002) I found out that external protocol handling happened too early in the processing of request start and redirects done by ResourceLoader; namely before ResourceHandler implementations are notified and able to do their work in deferred tasks. This CL changes that order to allow the upcoming mixed-content NavigationThrottle implementation to properly check the navigation start or redirect ahead of external protocol dispatching. For the record, the failing layout test that brought this change to light was: http/tests/security/mixedContent/nonwebby-scheme-in-iframe-allowed.https.html BUG=576270 ==========
Description was changed from ========== Move ResourceHandler deferred actions ahead of external protocol handling. While working on moving mixed-content checks to the browser (https://codereview.chromium.org/1905033002) I found out that external protocol handling happened too early in the processing of request start and redirects done by ResourceLoader; namely before ResourceHandler implementations are notified and able to do their work in deferred tasks. This CL changes that order to allow the upcoming mixed-content NavigationThrottle implementation to properly check the navigation start or redirect ahead of external protocol dispatching. For the record, the failing layout test that brought this change to light was: http/tests/security/mixedContent/nonwebby-scheme-in-iframe-allowed.https.html BUG=576270 ========== to ========== Move ResourceHandler deferred actions ahead of external protocol handling. While working on moving mixed-content checks to the browser (https://codereview.chromium.org/1905033002) I found out that external protocol handling happened too early in the processing of request start and redirects done by ResourceLoader; namely before ResourceHandler implementations are notified and able to do their work in deferred tasks. This CL changes that order to allow the upcoming mixed-content NavigationThrottle implementation to properly check the navigation start or redirect ahead of external protocol dispatching. For the record, the failing layout test that brought this change to light was: http/tests/security/mixedContent/nonwebby-scheme-in-iframe-allowed.https.html BUG=576270 ==========
The CQ bit was checked by carlosk@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...
Thanks! Have you filed a crbug for that crrev bug? ;) My initial CL description was incorrect and I just updated it. The motivation was less because of redirects and more in regards to ResourceHandler implementations being able to do their work ahead of external protocol handling. For just a bit more details see: - https://codereview.chromium.org/1905033002/diff/440001/content/browser/loader... - https://codereview.chromium.org/1905033002/#msg22 While working with the unit tests I also found a bug in my previous implementation where the external protocol handler would not be called when it should while dealing with a redirect. It's now fixed and verified by tests. And finally I realized this change brings one behavioral change: if the ResourceHandler decides to cancel the request (by returning false from OnWillStart or OnRequestRedirected) the external protocol will not be handled anymore, whereas before it would. A couple of unit tests that now cover this behavior change are SyncCancelOnWillStart and SyncCancelOnRequestRedirected. The expectation for the external protocol handling counter there would have to be increased by 1 in both cases if this behavior should be brought back. This new behavior makes sense to me and there are no tests that fail because of it. But I'm not sure this is acceptable and bringing it back would be simple simple. So WDYT?
On 2016/12/16 02:23:04, carlosk wrote: > Thanks! Have you filed a crbug for that crrev bug? ;) Since we're moving off rietveld "real soon now", doesn't seem worth the effort. > My initial CL description was incorrect and I just updated it. The motivation > was less because of redirects and more in regards to ResourceHandler > implementations being able to do their work ahead of external protocol handling. > For just a bit more details see: > https://codereview.chromium.org/1905033002/diff/440001/content/browser/loader... > - https://codereview.chromium.org/1905033002/#msg22 Ok, that makes a lot more sense to me! Thanks for the explanation. > While working with the unit tests I also found a bug in my previous > implementation where the external protocol handler would not be called when it > should while dealing with a redirect. It's now fixed and verified by tests. > > And finally I realized this change brings one behavioral change: if the > ResourceHandler decides to cancel the request (by returning false from > OnWillStart or OnRequestRedirected) the external protocol will not be handled > anymore, whereas before it would. A couple of unit tests that now cover this > behavior change are SyncCancelOnWillStart and SyncCancelOnRequestRedirected. The > expectation for the external protocol handling counter there would have to be > increased by 1 in both cases if this behavior should be brought back. > > This new behavior makes sense to me and there are no tests that fail because of > it. But I'm not sure this is acceptable and bringing it back would be simple > simple. So WDYT? I think this sounds reasonable. Most ResourceThrottles/ResourceHandlers shouldn't care about such redirects, but this does give things like safebrowsing a chance to look at it (I assume it currently only checks HTTP/HTTPS URLs, anyways, but that could theoretically change in the future). It does mean we send the URL to the renderer, and give it a chance to block the redirect as well - no clue what goes on there. Anyhow, I'll review tomorrow.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/16 03:45:38, mmenke (Out Dec 17 to Jan 2) wrote: > On 2016/12/16 02:23:04, carlosk wrote: > > Thanks! Have you filed a crbug for that crrev bug? ;) > > Since we're moving off rietveld "real soon now", doesn't seem worth the effort. > > > My initial CL description was incorrect and I just updated it. The motivation > > was less because of redirects and more in regards to ResourceHandler > > implementations being able to do their work ahead of external protocol > handling. > > For just a bit more details see: > > > https://codereview.chromium.org/1905033002/diff/440001/content/browser/loader... > > - https://codereview.chromium.org/1905033002/#msg22 > > Ok, that makes a lot more sense to me! Thanks for the explanation. > > > While working with the unit tests I also found a bug in my previous > > implementation where the external protocol handler would not be called when it > > should while dealing with a redirect. It's now fixed and verified by tests. > > > > And finally I realized this change brings one behavioral change: if the > > ResourceHandler decides to cancel the request (by returning false from > > OnWillStart or OnRequestRedirected) the external protocol will not be handled > > anymore, whereas before it would. A couple of unit tests that now cover this > > behavior change are SyncCancelOnWillStart and SyncCancelOnRequestRedirected. > The > > expectation for the external protocol handling counter there would have to be > > increased by 1 in both cases if this behavior should be brought back. > > > > This new behavior makes sense to me and there are no tests that fail because > of > > it. But I'm not sure this is acceptable and bringing it back would be simple > > simple. So WDYT? > > I think this sounds reasonable. Most ResourceThrottles/ResourceHandlers > shouldn't care about such redirects, but this does give things like safebrowsing > a chance to look at it (I assume it currently only checks HTTP/HTTPS URLs, > anyways, but that could theoretically change in the future). > > It does mean we send the URL to the renderer, and give it a chance to block the > redirect as well - no clue what goes on there. I don't understand what you meant here. Could you clarify? > Anyhow, I'll review tomorrow.
https://codereview.chromium.org/2574063003/diff/20001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2574063003/diff/20001/content/browser/loader/... content/browser/loader/resource_loader.cc:474: // At this point any possible deferred start is already over. I don't understand this comment, or what it has to do with the DCHECK. https://codereview.chromium.org/2574063003/diff/20001/content/browser/loader/... content/browser/loader/resource_loader.cc:482: if (!request_->status().is_success()) { Not quite sure what this is checking (Hrm...), but it should presumably be before the HandleExternalProtocol call - if something cancelled the request, we don't want to try to try to handle external protocols. https://codereview.chromium.org/2574063003/diff/20001/content/browser/loader/... content/browser/loader/resource_loader.cc:545: deferred_redirect_url_ = GURL(); Should clear this before calling FollowDeferredRedirect() - it shouldn't matter, as I don't believe |this| can be deleted, or another redirect seen, before FollowDeferredRedirect returns, but best to be safe against future refactors. https://codereview.chromium.org/2574063003/diff/20001/content/browser/loader/... File content/browser/loader/resource_loader.h (right): https://codereview.chromium.org/2574063003/diff/20001/content/browser/loader/... content/browser/loader/resource_loader.h:144: GURL deferred_redirect_url_; need to include url.h https://codereview.chromium.org/2574063003/diff/20001/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/2574063003/diff/20001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:511: return false; Do we have any tests where HandleExternalProtocol returns true? If not, should have tests for all relevant paths: 1) Sync OnWillStart successful completion -> HandleExternalProtocol returns true -> cancels request, don't see other events (Other than the complete event. 2) Same for sync OnWillRedirect successful completion. 3+4) Async variants of 1) and 2) Think we already have good coverage of all the cancellation cases, with the extra EXPECTS you added.
The CQ bit was checked by carlosk@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...
Thanks. Please note my comment in the unit test file. https://codereview.chromium.org/2574063003/diff/20001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2574063003/diff/20001/content/browser/loader/... content/browser/loader/resource_loader.cc:474: // At this point any possible deferred start is already over. On 2016/12/16 16:19:36, mmenke (Out Dec 17 to Jan 2) wrote: > I don't understand this comment, or what it has to do with the DCHECK. It was just meant to clarify that once this method is call all actions (if any) from a deferred start (if one occurred) are now completed. Bad positioning above the DCHECK. I moved and rephrased a bit. WDYT? https://codereview.chromium.org/2574063003/diff/20001/content/browser/loader/... content/browser/loader/resource_loader.cc:482: if (!request_->status().is_success()) { On 2016/12/16 16:19:36, mmenke (Out Dec 17 to Jan 2) wrote: > Not quite sure what this is checking (Hrm...), but it should presumably be > before the HandleExternalProtocol call - if something cancelled the request, we > don't want to try to try to handle external protocols. Done. It was not the case before my change, but either would deferred actions have taken place before the HEP call. So I agree it makes sense. https://codereview.chromium.org/2574063003/diff/20001/content/browser/loader/... content/browser/loader/resource_loader.cc:545: deferred_redirect_url_ = GURL(); On 2016/12/16 16:19:36, mmenke (Out Dec 17 to Jan 2) wrote: > Should clear this before calling FollowDeferredRedirect() - it shouldn't matter, > as I don't believe |this| can be deleted, or another redirect seen, before > FollowDeferredRedirect returns, but best to be safe against future refactors. Done. https://codereview.chromium.org/2574063003/diff/20001/content/browser/loader/... File content/browser/loader/resource_loader.h (right): https://codereview.chromium.org/2574063003/diff/20001/content/browser/loader/... content/browser/loader/resource_loader.h:144: GURL deferred_redirect_url_; On 2016/12/16 16:19:36, mmenke (Out Dec 17 to Jan 2) wrote: > need to include url.h Done. https://codereview.chromium.org/2574063003/diff/20001/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/2574063003/diff/20001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:511: return false; On 2016/12/16 16:19:36, mmenke (Out Dec 17 to Jan 2) wrote: > Do we have any tests where HandleExternalProtocol returns true? If not, should > have tests for all relevant paths: > > 1) Sync OnWillStart successful completion -> HandleExternalProtocol returns > true -> cancels request, don't see other events (Other than the complete event. > 2) Same for sync OnWillRedirect successful completion. > 3+4) Async variants of 1) and 2) > > Think we already have good coverage of all the cancellation cases, with the > extra EXPECTS you added. Done. https://codereview.chromium.org/2574063003/diff/40001/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/2574063003/diff/40001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:1317: EXPECT_EQ(1, did_received_redirect_); In the process I created the async tests I noticed a difference in call order between the handling of request start and redirects. This might be WAI but I wanted to bring it up here. On request start: 1 handler_->OnWillStart then delegate_->HandleExternalProtocol 2 delegate_->DidStartRequest On redirect: 1 delegate_->DidReceiveRedirect 2 handler_->OnRequestRedirected then delegate_->HandleExternalProtocol WDYT?
lgtm
https://codereview.chromium.org/2574063003/diff/40001/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/2574063003/diff/40001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:1317: EXPECT_EQ(1, did_received_redirect_); On 2016/12/20 19:23:31, carlosk wrote: > In the process I created the async tests I noticed a difference in call order > between the handling of request start and redirects. This might be WAI but I > wanted to bring it up here. > > On request start: > 1 handler_->OnWillStart then delegate_->HandleExternalProtocol > 2 delegate_->DidStartRequest > > On redirect: > 1 delegate_->DidReceiveRedirect > 2 handler_->OnRequestRedirected then delegate_->HandleExternalProtocol > > WDYT? I suspect the order didn't matter and it wasn't intended. Seems orthogonal from this cl though?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2574063003/diff/40001/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/2574063003/diff/40001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:1317: EXPECT_EQ(1, did_received_redirect_); On 2016/12/20 20:55:56, jam wrote: > On 2016/12/20 19:23:31, carlosk wrote: > > In the process I created the async tests I noticed a difference in call order > > between the handling of request start and redirects. This might be WAI but I > > wanted to bring it up here. > > > > On request start: > > 1 handler_->OnWillStart then delegate_->HandleExternalProtocol > > 2 delegate_->DidStartRequest > > > > On redirect: > > 1 delegate_->DidReceiveRedirect > > 2 handler_->OnRequestRedirected then delegate_->HandleExternalProtocol > > > > WDYT? > > I suspect the order didn't matter and it wasn't intended. Seems orthogonal from > this cl though? It is orthogonal; I was just curious. Thanks.
The CQ bit was checked by carlosk@chromium.org
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": 40001, "attempt_start_ts": 1482272524013520,
"parent_rev": "1c9ccf07d793d092fac523a0817f9895b6320474", "commit_rev":
"fdeb419ca8957b31fdfb45954aaf3fe5a342ad83"}
Message was sent while issue was closed.
Description was changed from ========== Move ResourceHandler deferred actions ahead of external protocol handling. While working on moving mixed-content checks to the browser (https://codereview.chromium.org/1905033002) I found out that external protocol handling happened too early in the processing of request start and redirects done by ResourceLoader; namely before ResourceHandler implementations are notified and able to do their work in deferred tasks. This CL changes that order to allow the upcoming mixed-content NavigationThrottle implementation to properly check the navigation start or redirect ahead of external protocol dispatching. For the record, the failing layout test that brought this change to light was: http/tests/security/mixedContent/nonwebby-scheme-in-iframe-allowed.https.html BUG=576270 ========== to ========== Move ResourceHandler deferred actions ahead of external protocol handling. While working on moving mixed-content checks to the browser (https://codereview.chromium.org/1905033002) I found out that external protocol handling happened too early in the processing of request start and redirects done by ResourceLoader; namely before ResourceHandler implementations are notified and able to do their work in deferred tasks. This CL changes that order to allow the upcoming mixed-content NavigationThrottle implementation to properly check the navigation start or redirect ahead of external protocol dispatching. For the record, the failing layout test that brought this change to light was: http/tests/security/mixedContent/nonwebby-scheme-in-iframe-allowed.https.html BUG=576270 Review-Url: https://codereview.chromium.org/2574063003 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Move ResourceHandler deferred actions ahead of external protocol handling. While working on moving mixed-content checks to the browser (https://codereview.chromium.org/1905033002) I found out that external protocol handling happened too early in the processing of request start and redirects done by ResourceLoader; namely before ResourceHandler implementations are notified and able to do their work in deferred tasks. This CL changes that order to allow the upcoming mixed-content NavigationThrottle implementation to properly check the navigation start or redirect ahead of external protocol dispatching. For the record, the failing layout test that brought this change to light was: http/tests/security/mixedContent/nonwebby-scheme-in-iframe-allowed.https.html BUG=576270 Review-Url: https://codereview.chromium.org/2574063003 ========== to ========== Move ResourceHandler deferred actions ahead of external protocol handling. While working on moving mixed-content checks to the browser (https://codereview.chromium.org/1905033002) I found out that external protocol handling happened too early in the processing of request start and redirects done by ResourceLoader; namely before ResourceHandler implementations are notified and able to do their work in deferred tasks. This CL changes that order to allow the upcoming mixed-content NavigationThrottle implementation to properly check the navigation start or redirect ahead of external protocol dispatching. For the record, the failing layout test that brought this change to light was: http/tests/security/mixedContent/nonwebby-scheme-in-iframe-allowed.https.html BUG=576270 Committed: https://crrev.com/94bd59f28dcf81289f09c39d189128254631b275 Cr-Commit-Position: refs/heads/master@{#439923} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/94bd59f28dcf81289f09c39d189128254631b275 Cr-Commit-Position: refs/heads/master@{#439923}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2597563002/ by shimazu@chromium.org. The reason for reverting is: This patch made two layout tests TIMEOUT: virtual/mojo-loading/http/tests/misc/redirect-to-about-blank.html http/tests/misc/redirect-to-about-blank.html. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
