Fix for CAPTCHA problem in Release channel (1.16.x - already fixed in Chromium 87 upgrade) #6993
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
antonok-edm
reviewed
Oct 31, 2020
…Mode Chromium change: https://chromium.googlesource.com/chromium/src/+/b1917744868247ea5103eb2ca903be8c3a27b51f commit b1917744868247ea5103eb2ca903be8c3a27b51f Author: Christian Dullweber <dullweber@chromium.org> Date: Thu Sep 3 17:55:57 2020 +0000 Reland "Remove BlockThirdPartyCookies preference" This is a reland of 1180542ba81582ea964e4ee7fe32f439462fb245 The internal CL is rolled in now. Let's try again... Original change's description: > Remove BlockThirdPartyCookies preference > > The boolean BlockThirdPartyCookies preference was replaced by > CookieControlsMode enum. Existing settings were migrated since M83. > Since all usage of the preference has been removed, the preference > can be removed and existing settings cleared. > > Bug: 1104836 > Change-Id: Ie6e38b0a424981395c627459d684030ea84a58b7 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2387805 > Reviewed-by: Balazs Engedy <engedy@chromium.org> > Reviewed-by: Gabriel Charette <gab@chromium.org> > Commit-Queue: Christian Dullweber <dullweber@chromium.org> > Cr-Commit-Position: refs/heads/master@{#803844} Tbr: engedy@chromium.org, gab@chromium.org Bug: 1104836, 1124326
The override was added in ea29650 due to build errors like gen/third_party/blink/renderer/bindings/modules/v8/v8_shared_worker_global_scope.cc:4614:34: error: no member named 'isReportingObserversEnabled' in 'blink::ContextFeatureSettings' context_feature_settings->isReportingObserversEnabled())) { ~~~~~~~~~~~~~~~~~~~~~~~~ ^ 1 error generated. As it turns out, the error were due to us making ReportingObservers ContextEnabled in this change: https://github.com/brave/brave-core/pull/4578/files To fix added missing methods to blink::ContextFeatureSettings similarly to how MojoJS is handled.
b4d375a to
6da803a
Compare
The new IDL compiler removes the ../../ prefix for include files in an attempt to "canonicalize the paths heuristically", which works well for modules under //third_party/blink/renderer/modules. Unfortunately, that does not work well with our Brave module since it lives under //brave/third_party/blink/renderer/modules, and the prefix needs to be kept so that header files can be correctly included from the generated sources' directory. This patch adapts the python code that does this adjustment so that it does not remove the prefix for Brave-specific files.
With the new IDL compiler, every source file related to a Blink module
needs to be listed here so that the right symbols get properly exposed
and we don't find build errors like the following one while linking:
ld.lld: error: undefined symbol: blink::Brave::wrapper_type_info_
>>> referenced by navigator_brave.cc
>>> obj/brave/third_party/blink/renderer/modules/brave/brave/navigator_brave.o:(blink::Brave::GetWrapperTypeInfo() const)
This patch adds v8_brave.{h,cc} to the list so that we can keep
building the Brave module with the new IDL compiler enabled, which
is the default upstream now.
mariospr
approved these changes
Nov 2, 2020
There was a problem hiding this comment.
I was not aware of this being an issue but, now I saw this PR, it reminded me that lately I "had the feeling" that I was seeing more captcha challenges than what I recalled in the past... and this PR fixed that for me as well (at least in the cases I normally saw them, in www.correos.es > "Localizar Envío"). LGTM!
iefremov
approved these changes
Nov 2, 2020
|
Can't test it myself - don't have a paypal account - but changes look fine |
antonok-edm
approved these changes
Nov 2, 2020
This was referenced Nov 2, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes brave/brave-browser#12359
Test I tried when looking for a fix
I worked back on previous versions of Brave using a simple test:
After login, you are either shown:
a. SMS 2FA screen (I consider a success)
b. Challenge screen (I consider this a failure)
When did this problem start?
Using this test, I was able to trace the first Release version that broke down to
1.15.72. Using Beta/Dev versions I was able to further narrow that down to:1.15.56does NOT have the problem1.15.59DOES have the problemWhat this PR includes (and why)
What landed between those versions was Chromium 86 with #6195.
In Nightly, the overall problem seems to be gone and Nightly was recently upgraded to Chromium 87 with #6554
In Chromium 86, I saw this commit which caught my eye:
ea29650
In Chromium 87, this was revisited and fixed properly:
0b06833
This PR pulled that in along with the following commit from Chromium 87:
aadceeb
Steps to verify fix
brave-browser1.16.xbranchbsc-1.16.x)npm run init && npm run build -- Release --gn=is_official_build:trueBrave-Browserprofile (if you have one) so that you have a clean profilenpm start -- ReleaseSubmitter Checklist:
npm run lint,npm run gn_check)git rebase master(if needed).git rebase -ito squash commits (if needed).QA/YesorQA/No) to the associated issuerelease-notes/includeorrelease-notes/exclude) to the associated issueTest Plan:
If it works, you should see reCAPTCHA underneath the login form:
If it fails, you'll see this error message after submitting the login form:
Reviewer Checklist:
After-merge Checklist:
changes has landed on.