Introduce AsyncDocumentSubresourceFilter.

components/subresource_filter/content/browser/async_document_subresource_filter.cc

Index: components/subresource_filter/content/browser/async_document_subresource_filter.cc
diff --git a/components/subresource_filter/content/browser/async_document_subresource_filter.cc b/components/subresource_filter/content/browser/async_document_subresource_filter.cc
new file mode 100644
index 0000000000000000000000000000000000000000..90c987b97f416a5d5b7136471f5e500e57d79533
--- /dev/null
+++ b/components/subresource_filter/content/browser/async_document_subresource_filter.cc
@@ -0,0 +1,145 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/subresource_filter/content/browser/async_document_subresource_filter.h"
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/callback.h"
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/task_runner_util.h"
+#include "base/threading/sequenced_task_runner_handle.h"
+#include "components/subresource_filter/core/common/memory_mapped_ruleset.h"
+
+namespace subresource_filter {
+
+// AsyncDocumentSubresourceFilter::InitializationParams ------------------------
+
+using InitializationParams =
+    AsyncDocumentSubresourceFilter::InitializationParams;
+
+InitializationParams::InitializationParams() = default;
+
+InitializationParams::InitializationParams(GURL document_url,
+                                           ActivationLevel activation_level,
+                                           bool measure_performance)
+    : document_url(std::move(document_url)),
+      parent_activation_state(activation_level) {
+  DCHECK_NE(ActivationLevel::DISABLED, activation_level);
+  parent_activation_state.measure_performance = measure_performance;
+}
+
+InitializationParams::InitializationParams(
+    GURL document_url,
+    url::Origin parent_document_origin,
+    ActivationState parent_activation_state)
+    : document_url(std::move(document_url)),
+      parent_document_origin(std::move(parent_document_origin)),
+      parent_activation_state(parent_activation_state) {
+  DCHECK_NE(ActivationLevel::DISABLED,
+            parent_activation_state.activation_level);
+}
+
+InitializationParams::~InitializationParams() = default;
+InitializationParams::InitializationParams(InitializationParams&&) = default;
+InitializationParams& InitializationParams::operator=(InitializationParams&&) =
+    default;
+
+// AsyncDocumentSubresourceFilter ----------------------------------------------
+
+AsyncDocumentSubresourceFilter::AsyncDocumentSubresourceFilter(
+    VerifiedRuleset::Handle* ruleset_handle,

[engedy, 2017/02/14 15:13:13]

Based on how Charles is planning to use this, have you thought about taking a
unique_ptr<VR::Handle> here?

[pkalinnikov, 2017/02/14 18:47:13]

Charles, can you please tell how you are going to use ruleset handles?

My intention was: Let us have a single ruleset handle (which is created as soon
as the main-frame throttle sees a possibility to use it in the close future, and
which then lives throughout the whole page load unless the throttle sees that it
won't actually be used), and use it for all the subframes to create ADSFs. PRO:
all the subframes get the same version of the ruleset.

Your proposal seems different, because it transfers ownership from handle to
ADSF. And the solution when we grab a new handle for each subframe doesn't have
the above mentioned PRO.

What do you think?

[Charlie Harrison, 2017/02/14 19:31:06]

Your solution sounds good to me. I was thinking that the page-level structure
would get a single ruleset *dealer* handle, out of which all ASDFs would
generate their handles. Wouldn't this achieve the same pro? In any case I think
I prefer your approach because generating the handle's is not free and generates
extra initialization tasks on the blocking task queue.

[engedy, 2017/02/14 20:33:31]

Currently the verification state is persisted on the VRDealer, so there should
be only one instance of that per browser process to avoid multiple verifications
taking place. This is also more consistent with the dealer being a per-process
singleton in the renderers. 

Other than that, the only important thing is that we should not have any
VR::Handle instances created unless there is a good chance of page-level
activation. If we can nicely maintain this shared instance on the page level,
I'm fine with that.

I agree that both using the same version of the ruleset across all frames in
page, as well as avoiding initializing per-frame VR::Handles are nice, but
neither of them are hard constraints.

+    InitializationParams params,
+    base::Callback<void(ActivationState)> activation_state_callback,
+    base::OnceClosure first_disallowed_load_callback)
+    : task_runner_(ruleset_handle->task_runner()),
+      core_(new Core(), base::OnTaskRunnerDeleter(task_runner_)),
+      first_disallowed_load_callback_(
+          std::move(first_disallowed_load_callback)) {

[engedy, 2017/02/14 15:13:13]

nit: #include <utility> for std::move.

[pkalinnikov, 2017/02/14 18:47:13]

Done.

+  DCHECK_NE(ActivationLevel::DISABLED,
+            params.parent_activation_state.activation_level);
+
+  // Note: It is safe to post |ruleset_handle|'s VerifiedRuleset pointer,
+  // because a task to delete it can only be posted to (and, therefore,
+  // processed by) |task_runner| after this method returns, hence after the
+  // below task is posted.
+  base::PostTaskAndReplyWithResult(
+      task_runner_, FROM_HERE,
+      base::Bind(&Core::Initialize, base::Unretained(core_.get()),
+                 base::Passed(&params), ruleset_handle->ruleset_.get()),
+      std::move(activation_state_callback));
+}
+
+AsyncDocumentSubresourceFilter::~AsyncDocumentSubresourceFilter() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+}
+
+void AsyncDocumentSubresourceFilter::GetLoadPolicyForSubdocument(
+    const GURL& subdocument_url,
+    LoadPolicyCallback result_callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  auto compute_load_policy = [](AsyncDocumentSubresourceFilter::Core* core,
+                                const GURL& subdocument_url) {
+    DCHECK(core);
+    DocumentSubresourceFilter* filter = core->filter();

[engedy, 2017/02/14 15:13:13]

nit: Do you expect it to have a measurable performance impact if I really wanted
to save some lines by just inlining core->filter()? Same below.

[pkalinnikov, 2017/02/14 18:47:13]

Funny thing is that it doesn't save the line, because the one below becomes too
long and gets split :)
I don't see other benefits, so I propose to stick with the current code. Same
concerns the lambda below.

[engedy, 2017/02/14 20:33:31]

Fair enough. ;-)

+    return filter ? filter->GetLoadPolicyForSubdocument(subdocument_url)
+                  : LoadPolicy::Allow;
+  };
+
+  // TODO(pkalinnikov): Think about avoiding copy of |subdocument_url| if it is
+  // too big and won't be allowed anyway (e.g., it's a data: URI).
+  base::PostTaskAndReplyWithResult(
+      task_runner_, FROM_HERE,
+      base::Bind(compute_load_policy, core_.get(), subdocument_url),
+      std::move(result_callback));
+}
+
+void AsyncDocumentSubresourceFilter::ReportDisallowedLoad() {
+  if (!first_disallowed_load_callback_.is_null())
+    std::move(first_disallowed_load_callback_).Run();
+
+  auto report_disallowed_load = [](AsyncDocumentSubresourceFilter::Core* core) {

[engedy, 2017/02/14 15:13:13]

nit: How would it look if we inlined this lambda (provided it works with
base::Bind)?

[pkalinnikov, 2017/02/14 18:47:14]

Okay, this doesn't look too bad :)

+    DCHECK(core);
+    DocumentSubresourceFilter* filter = core->filter();
+    if (filter)
+      filter->reportDisallowedLoad();
+  };
+  task_runner_->PostTask(FROM_HERE,
+                         base::Bind(report_disallowed_load, core_.get()));
+}
+
+// AsyncDocumentSubresourceFilter::Core ----------------------------------------
+
+AsyncDocumentSubresourceFilter::Core::Core() {
+  thread_checker_.DetachFromThread();
+}
+
+AsyncDocumentSubresourceFilter::Core::~Core() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+}
+
+ActivationState AsyncDocumentSubresourceFilter::Core::Initialize(
+    InitializationParams params,
+    VerifiedRuleset* verified_ruleset) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(verified_ruleset);
+
+  const MemoryMappedRuleset* ruleset = verified_ruleset->Get();

[engedy, 2017/02/14 15:13:13]

nit: Same here?

[pkalinnikov, 2017/02/14 18:47:13]

Done.

+  if (!ruleset)
+    return ActivationState(ActivationLevel::DISABLED);
+
+  ActivationState activation_state =
+      ComputeActivationState(params.document_url, params.parent_document_origin,
+                             params.parent_activation_state, ruleset);
+
+  DCHECK_NE(ActivationLevel::DISABLED, activation_state.activation_level);
+  filter_.emplace(url::Origin(params.document_url), activation_state, ruleset,
+                  base::OnceClosure());

[engedy, 2017/02/14 15:13:13]

Let's add a comment to explain why this callback is never used.

[pkalinnikov, 2017/02/14 18:47:14]

Done.

+
+  return activation_state;
+}
+
+}  // namespace subresource_filter