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

Fix confusing content settings #11

Merged
merged 2 commits into from
Dec 29, 2017
Merged

Conversation

bbondy
Copy link
Member

@bbondy bbondy commented Dec 28, 2017

@@ -0,0 +1,18 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
Copy link
Collaborator

Choose a reason for hiding this comment

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

since they are a kind of sub-content type, I'm thinking that we should just call this file brave_shields/common/brave_shield_constants.h to be consistent with other constants files

Copy link
Member Author

@bbondy bbondy Dec 29, 2017

Choose a reason for hiding this comment

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

Will do and merge with shield_types.h


if (setting == CONTENT_SETTING_DEFAULT) {
if (resource_identifier == "ads") {
return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

should the top level shield settings be preferences?

Copy link
Member Author

Choose a reason for hiding this comment

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

will come in the future, will add a comment for now

Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

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

++

@bridiver bridiver merged commit 9fe9732 into master Dec 29, 2017
@bsclifton bsclifton deleted the content-settings-fix-confusing branch June 18, 2018 06:25
NejcZdovc pushed a commit that referenced this pull request Dec 10, 2018
bbondy pushed a commit that referenced this pull request Feb 18, 2019
antonok-edm added a commit that referenced this pull request Mar 10, 2021
Update adblock-rust to 0.2.1 with optional CSS validation
mariospr added a commit that referenced this pull request Jun 25, 2021
Don't initialize callbacks from HistogramsBraveizer's constructor.

Initializing all the StatisticsRecorder::ScopedHistogramSampleObserver
objects for each histogram from the constructor would be too early and
will cause crashes on DCHECK-enabled builds, like the following one:

  [2883544:2883544:0624/122638.724689:FATAL:bind_internal.h(843)]
    Check failed: receiver->HasAtLeastOneRef(). base::Bind{Once,Repeating}()
    refuses to create the first reference to ref-counted objects. That typically
    happens around PostTask() in their constructor, and such objects can be
    destroyed before `new` returns if the task resolves fast enough.

  #0 0x7faa83f3c119 base::debug::CollectStackTrace()
  #1 0x7faa83e389d3 base::debug::StackTrace::StackTrace()
  #2 0x7faa83e59cc4 logging::LogMessage::~LogMessage()
  #3 0x7faa83e5a70e logging::LogMessage::~LogMessage()
  #4 0x563a89be9924 _ZN4base8internal34BanUnconstructedRefCountedReceiverIMN10[...]
  #5 0x563a8a4e6f40 base::internal::BindState<>::Create<>()
  #6 0x563a8a4e6a91 brave::HistogramsBraveizer::InitCallbacks()
  #7 0x563a8a96f0bf BraveBrowserProcessImpl::BraveBrowserProcessImpl()
  #8 0x563a8a73eaee ChromeBrowserMainParts::PreEarlyInitialization()
  #9 0x563a8aad13e5 ChromeBrowserMainPartsPosix::PreEarlyInitialization()
  #10 0x7faa817a6952 content::BrowserMainLoop::EarlyInitialization()
  #11 0x7faa817aaa2b content::BrowserMainRunnerImpl::Initialize()
  [...]

Therefore, this patch delays the initialization of the callbacks
until the HistogramsBraveizer object is properly constructed, by
using an static Create method instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants