New issue
Advanced search Search tips

Issue 852058 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 12
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 696822



Sign in to add a comment

Declarative Net Request: Handle flatbuffer ruleset corruption.

Project Member Reported by karandeepb@chromium.org, Jun 12 2018

Issue description

For the Declarative Net Request, we persist an indexed flatbuffer ruleset file on disk when the extension is installed. We should handle the case when the indexed flatbuffer ruleset file is corrupted on disk.
 
Currently, the extension would not load the declarative ruleset on corruption. We can instead do the following:

- Try to reindex the rules from the JSON rules file.
- If that fails as well, disable the extension.

Let me know if any one has any thoughts regarding this.
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 20 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/af014ee5332bcf11e0b83549084f9b5402c4f696

commit af014ee5332bcf11e0b83549084f9b5402c4f696
Author: Karan Bhatia <karandeepb@chromium.org>
Date: Wed Jun 20 22:21:49 2018

Data decoder service: Allow process sharing in SafeJsonParser.

This CL adds SafeJsonParser::ParseBatch which also takes in a |batch_id|
parameter. This allows clients to potentially use the same process for multiple
usages of data decoder service.

BUG= 852058 

Change-Id: If23f0cad2e819a6d3a2c0a136aab57284937f20c
Reviewed-on: https://chromium-review.googlesource.com/1105565
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Jay Civelli <jcivelli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569048}
[modify] https://crrev.com/af014ee5332bcf11e0b83549084f9b5402c4f696/chrome/browser/safe_json_parser_browsertest.cc
[modify] https://crrev.com/af014ee5332bcf11e0b83549084f9b5402c4f696/services/data_decoder/public/cpp/safe_json_parser.cc
[modify] https://crrev.com/af014ee5332bcf11e0b83549084f9b5402c4f696/services/data_decoder/public/cpp/safe_json_parser.h
[modify] https://crrev.com/af014ee5332bcf11e0b83549084f9b5402c4f696/services/data_decoder/public/cpp/safe_json_parser_impl.cc
[modify] https://crrev.com/af014ee5332bcf11e0b83549084f9b5402c4f696/services/data_decoder/public/cpp/safe_json_parser_impl.h

Project Member

Comment 3 by bugdroid1@chromium.org, Jun 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/23e67eddb987f565340b9bae5545f216bf92d302

commit 23e67eddb987f565340b9bae5545f216bf92d302
Author: Karan Bhatia <karandeepb@chromium.org>
Date: Tue Jun 26 20:43:01 2018

Declarative Net Request: Unify rule indexing code.

This CL unifies more of the JSON ruleset indexing code used by the
SandboxedUnpacker and UnpackedInstaller. In particular, the JSON parsing of the
ruleset is now handled by declarative_net_request::IndexAndPersistRules and
declarative_net_request::IndexAndPersistRulesUnsafe. This is useful since
subsequent usages of JSON ruleset indexing need to be added. (for example on
indexed ruleset corruption).

Also, no longer delete any existing file at the indexed ruleset path for packed
extensions. It's not necessary since these would be overwritten anyway while
ruleset indexing.

BUG= 852058 

Change-Id: Ic0718c03f7e056dfbb0e034f1dfafa3b52a0f03f
Reviewed-on: https://chromium-review.googlesource.com/1098526
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570520}
[modify] https://crrev.com/23e67eddb987f565340b9bae5545f216bf92d302/chrome/browser/extensions/api/declarative_net_request/rule_indexing_unittest.cc
[modify] https://crrev.com/23e67eddb987f565340b9bae5545f216bf92d302/chrome/browser/extensions/unpacked_installer.cc
[modify] https://crrev.com/23e67eddb987f565340b9bae5545f216bf92d302/extensions/browser/api/declarative_net_request/constants.cc
[modify] https://crrev.com/23e67eddb987f565340b9bae5545f216bf92d302/extensions/browser/api/declarative_net_request/constants.h
[modify] https://crrev.com/23e67eddb987f565340b9bae5545f216bf92d302/extensions/browser/api/declarative_net_request/parse_info.cc
[modify] https://crrev.com/23e67eddb987f565340b9bae5545f216bf92d302/extensions/browser/api/declarative_net_request/utils.cc
[modify] https://crrev.com/23e67eddb987f565340b9bae5545f216bf92d302/extensions/browser/api/declarative_net_request/utils.h
[modify] https://crrev.com/23e67eddb987f565340b9bae5545f216bf92d302/extensions/browser/install/sandboxed_unpacker_failure_reason.h
[modify] https://crrev.com/23e67eddb987f565340b9bae5545f216bf92d302/extensions/browser/sandboxed_unpacker.cc
[modify] https://crrev.com/23e67eddb987f565340b9bae5545f216bf92d302/extensions/browser/sandboxed_unpacker.h
[modify] https://crrev.com/23e67eddb987f565340b9bae5545f216bf92d302/extensions/common/manifest_constants.cc
[modify] https://crrev.com/23e67eddb987f565340b9bae5545f216bf92d302/extensions/common/manifest_constants.h

Project Member

Comment 4 by bugdroid1@chromium.org, Jul 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/24c48ccdd9ce853f96dc91dfda7014327c18a01c

commit 24c48ccdd9ce853f96dc91dfda7014327c18a01c
Author: Karan Bhatia <karandeepb@chromium.org>
Date: Mon Jul 09 22:50:45 2018

Declarative Net Request: Refactor RulesMonitorService (1/2).

This CL performs the following changes:
 - extension_with_rulesets_ holds ExtensionsIds instead of Extension pointers.
 - file_task_runner_ uses the central Extension file task runner now instead of
   creating its own.
 - RulesMonitorService declares explicit dependencies on ExtensionRegistry,
   ExtensionPrefs and ExtensionSystem.

This refactoring is in preparation for a subsequent CL which will handle indexed
ruleset corruption.

This CL does not introduce any behavior change.

BUG= 852058 

Change-Id: Ia3a6a234836ac23f1571bb7b062ebf7bf6fc98ba
Reviewed-on: https://chromium-review.googlesource.com/1128387
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573497}
[modify] https://crrev.com/24c48ccdd9ce853f96dc91dfda7014327c18a01c/extensions/browser/api/declarative_net_request/rules_monitor_service.cc
[modify] https://crrev.com/24c48ccdd9ce853f96dc91dfda7014327c18a01c/extensions/browser/api/declarative_net_request/rules_monitor_service.h

Project Member

Comment 5 by bugdroid1@chromium.org, Jul 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1f2e0a084d0ef20ba6b892f2731cf6f8c6c28cbc

commit 1f2e0a084d0ef20ba6b892f2731cf6f8c6c28cbc
Author: Karan Bhatia <karandeepb@chromium.org>
Date: Tue Jul 10 00:15:40 2018

Declarative Net Request: Refactor RulesMonitorService (2/2).

This CL refactors RulesMonitorService. In particular, inner classes
FileSequenceState and FileSequenceBridge are introduced to handle the file
sequence operations. Also, while loading a ruleset on the file sequence, instead
of hopping directly to the IO thread, we return the result to the UI thread,
which then decides what to do. And unloading a ruleset now proceeds directly to
the IO thread instead of hopping to the file sequence first. This refactoring is
in preparation for a subsequent CL which will handle indexed ruleset corruption.

This CL does not introduce any behavior change.

BUG= 852058 

Change-Id: Idad96dcacc4563c1936890daf3c7bd548b9d4319
Reviewed-on: https://chromium-review.googlesource.com/1119635
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573545}
[modify] https://crrev.com/1f2e0a084d0ef20ba6b892f2731cf6f8c6c28cbc/extensions/browser/api/declarative_net_request/rules_monitor_service.cc
[modify] https://crrev.com/1f2e0a084d0ef20ba6b892f2731cf6f8c6c28cbc/extensions/browser/api/declarative_net_request/rules_monitor_service.h

Project Member

Comment 6 by bugdroid1@chromium.org, Jul 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3b7bb4be8c040188b1ad9e36ee9262dd6fcdf8f8

commit 3b7bb4be8c040188b1ad9e36ee9262dd6fcdf8f8
Author: Karan Bhatia <karandeepb@chromium.org>
Date: Thu Jul 12 01:59:52 2018

Declarative Net Request: Handle flatbuffer indexed ruleset corruption.

The JSON rulesets provided by an extension for the Declarative Net Request API
are indexed to a flatbuffer (indexed) format during extension installation. This
indexed ruleset is excluded from content verification and it is possible that it
may get corrupted, making the extension ruleset useless.

This CL handles ruleset corruption by re-indexing the JSON ruleset if the
indexed ruleset fails to load.

BUG= 852058 

Change-Id: Ief43cd8f2bc030ad634a92e1fcf7656d03abd4c0
Reviewed-on: https://chromium-review.googlesource.com/1107479
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574458}
[modify] https://crrev.com/3b7bb4be8c040188b1ad9e36ee9262dd6fcdf8f8/chrome/browser/extensions/api/declarative_net_request/declarative_net_request_browsertest.cc
[modify] https://crrev.com/3b7bb4be8c040188b1ad9e36ee9262dd6fcdf8f8/extensions/browser/api/declarative_net_request/rules_monitor_service.cc
[modify] https://crrev.com/3b7bb4be8c040188b1ad9e36ee9262dd6fcdf8f8/extensions/browser/api/declarative_net_request/ruleset_manager.cc
[modify] https://crrev.com/3b7bb4be8c040188b1ad9e36ee9262dd6fcdf8f8/extensions/browser/api/declarative_net_request/ruleset_manager.h
[modify] https://crrev.com/3b7bb4be8c040188b1ad9e36ee9262dd6fcdf8f8/extensions/browser/api/declarative_net_request/utils.cc
[modify] https://crrev.com/3b7bb4be8c040188b1ad9e36ee9262dd6fcdf8f8/extensions/browser/api/declarative_net_request/utils.h
[modify] https://crrev.com/3b7bb4be8c040188b1ad9e36ee9262dd6fcdf8f8/extensions/browser/sandboxed_unpacker.cc
[modify] https://crrev.com/3b7bb4be8c040188b1ad9e36ee9262dd6fcdf8f8/tools/metrics/histograms/histograms.xml

Project Member

Comment 7 by bugdroid1@chromium.org, Jul 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/82130059c12b0c1ed4149a2ce441fbf3bee6634e

commit 82130059c12b0c1ed4149a2ce441fbf3bee6634e
Author: Karan Bhatia <karandeepb@chromium.org>
Date: Thu Jul 12 21:41:47 2018

Declarative Net Request: Show a warning to user if an extension's ruleset fails to load.

If the ruleset for an extension fails to load, the extension won't be able to
block/redirect network requests through the Declarative Net Request API. This
might happen due to extension corruption issues etc. If this happens, notify the
user through a warning on the wrench menu and the chrome://extensions page.

BUG= 852058 

Change-Id: I64157ef0acc2cb37a26cd77ec437eba5d08194b2
Reviewed-on: https://chromium-review.googlesource.com/1134574
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574736}
[modify] https://crrev.com/82130059c12b0c1ed4149a2ce441fbf3bee6634e/chrome/browser/extensions/api/declarative_net_request/declarative_net_request_browsertest.cc
[modify] https://crrev.com/82130059c12b0c1ed4149a2ce441fbf3bee6634e/extensions/browser/api/declarative_net_request/rules_monitor_service.cc
[modify] https://crrev.com/82130059c12b0c1ed4149a2ce441fbf3bee6634e/extensions/browser/api/declarative_net_request/rules_monitor_service.h
[modify] https://crrev.com/82130059c12b0c1ed4149a2ce441fbf3bee6634e/extensions/browser/extension_prefs.cc
[modify] https://crrev.com/82130059c12b0c1ed4149a2ce441fbf3bee6634e/extensions/browser/extension_prefs.h
[modify] https://crrev.com/82130059c12b0c1ed4149a2ce441fbf3bee6634e/extensions/browser/warning_set.cc
[modify] https://crrev.com/82130059c12b0c1ed4149a2ce441fbf3bee6634e/extensions/browser/warning_set.h
[modify] https://crrev.com/82130059c12b0c1ed4149a2ce441fbf3bee6634e/extensions/strings/extensions_strings.grd

Blocking: 696822
Status: Fixed (was: Assigned)
It was decided to show a warning if the ruleset failed to load, instead of disabling the extension, since disabling was considered an overkill and corruption of the JSON ruleset file and extension prefs should be managed by components other than DNR. 

This should be fixed now.

Sign in to add a comment