Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicitly cancel blocked requests (attempt 2) #7030

Merged
merged 4 commits into from
Nov 9, 2020
Merged

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Nov 3, 2020

Manual re-opening of #6632

This was merged and then reverted with #7028
Fixes brave/brave-browser#10063


This reverts commit 12ea1cd, reversing
changes made to fbce153.


Submitter Checklist:

Test Plan:

The below pages have all been observed to crash with OOM errors about 20 seconds after loading without this change, but should not crash once this change is in place:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@bsclifton bsclifton self-assigned this Nov 3, 2020
This reverts commit 12ea1cd, reversing
changes made to fbce153.
Copy link
Collaborator

@antonok-edm antonok-edm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also updated the adblocking tests to use the more modern EvalJs instead of ExecuteScriptAndExtractBool, which made it a little easier to debug.

if (ctx_->cancel_request_explicitly) {
OnRequestError(network::URLLoaderCompletionStatus(net::ERR_ABORTED));
if (!ctx_->ShouldMockRequest()) {
OnRequestError(network::URLLoaderCompletionStatus(net::ERR_BLOCKED_BY_CLIENT));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bsclifton I just had to change net::ERR_ABORTED to net::ERR_BLOCKED_BY_CLIENT. The 4 test failures were occurring because the XMLHttpRequests were having their onabort handlers triggered instead of onerror.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice 😄👍

The XHRs from the 4 failing tests were triggering `onabort`, not
`onerror`.
@antonok-edm
Copy link
Collaborator

These tests had to be updated as well, since they also use the blocking.html test page:

PerfPredictorTabHelperTest.NoBlockNoSavings
PerfPredictorTabHelperTest.ScriptBlockHasSavings
PerfPredictorTabHelperTest.NewNavigationStoresSavings

@bsclifton
Copy link
Member Author

cc: @iefremov for re-review 😄

Was originally approved in PR by @spinda but full CI didn't run since it was in a fork

@iefremov
Copy link
Contributor

iefremov commented Nov 4, 2020

Thanks for fixing the tests!

@bsclifton I just had to change net::ERR_ABORTED to net::ERR_BLOCKED_BY_CLIENT. The 4 test failures were occurring because the XMLHttpRequests were having their onabort handlers triggered instead of onerror.

Wondering, does this change (from onabort to onerror) bring any potential webcompat risks?

@antonok-edm
Copy link
Collaborator

Wondering, does this change (from onabort to onerror) bring any potential webcompat risks?

@iefremov webcompat was the motivating reason behind this change actually. Adblocking extensions only have the ability to do net::ERR_BLOCKED_BY_CLIENT, so they design their adblock lists and scriptlets accordingly. We've run into a handful of issues where the difference makes us less compatible, given that we're using the same rules as uBlock Origin.

@antonok-edm
Copy link
Collaborator

@bsclifton anything left blocking this, or can we merge?

@antonok-edm
Copy link
Collaborator

CI failures are known, merging.

@antonok-edm antonok-edm merged commit 9719465 into master Nov 9, 2020
@antonok-edm antonok-edm deleted the bsc-reland-6632 branch November 9, 2020 17:28
@antonok-edm antonok-edm added this to the 1.18.x - Nightly milestone Nov 9, 2020
brave-builds pushed a commit that referenced this pull request Nov 9, 2020
@iefremov iefremov restored the bsc-reland-6632 branch November 10, 2020 06:18
iefremov pushed a commit that referenced this pull request Nov 10, 2020
@antonok-edm antonok-edm deleted the bsc-reland-6632 branch November 10, 2020 15:14
@kjozwiak
Copy link
Member

kjozwiak commented Nov 10, 2020

Verification PASSED on macOS 10.15.7 x64 using the following build:

Brave | 1.18.46 Chromium: 87.0.4280.49 (Official Build) unknown (x86_64)
-- | --
Revision | f77f85899646b42a1d3c8ff36794e00becab9171-refs/branch-heads/4280@{#1115}
OS | macOS Version 10.15.7 (Build 19H2)

Verification PASSED on Samsung S10+ on Android 10 using 1.18.46 CR: 87.0.4280.49

SergeyZhukovsky pushed a commit that referenced this pull request Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants