New issue
Advanced search Search tips

Issue 755717 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 27
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 696822



Sign in to add a comment

DNR: Implement versioning on the flatbuffer ruleset file.

Project Member Reported by karandeepb@chromium.org, Aug 15 2017

Issue description

Flatbuffer schema evolution may involve making breaking or non-breaking changes. Both should be sufficient for our needs.

If we allow making breaking changes to the flatbuffer schema, we'll have to implement the ability to re-index the extension rulesets. For the manifest rules, the JSON file will serve as the source-of-truth for re-indexing (since we won't be able to read the old flatbuffer file). But for dynamic rules, this won't be the case. Hence we'll have to persist the dynamic rules in some other format as well. Also re-indexing would be expensive.

If we only allow making non-breaking changes, we will still be able to read the old persisted flatbuffer rules file. However, we may need to introduce migration code, for some kind of changes.

- Adding a new field should work as is in most cases. (Default values for the field will be read from old files on new Chrome version and old Chrome version won't be able to read the new field in the new file).

- Deprecating a field should also work as is. (Old chrome version would get the default value for the field in the new file).

-Here's how changing a field's default value or type may work:
  - Introduce a new field with the new type.
  - Add migration code to fill the new field from the old field's value.
  - After some Chrome versions, deprecate the old field.

 
Update:

- We decided we did not want to implement support for dynamically added rules.
- https://chromium-review.googlesource.com/c/chromium/src/+/1107479 adds ability to re-index extension rulesets.

Hence we should now be able to do some sort of versioning on the flatbuffer ruleset format and re-index rulesets as necessary. There are multiple ways in which we can incorporate the versioning info. with the indexed ruleset file:

- As part of the indexed ruleset file name (I think we do something similar for extension packages). This would be easy to implement, but a drawback would be that it requires explicitly cleaning up old indexed ruleset files (But schema updates should be rare for the lifetime of a single extension version).

- As part of the indexed ruleset file header. We can probably add some version info. to the start of the indexed ruleset file.

On detecting version mismatch between the version currently used by Chrome and the indexed ruleset, we should re-index the rulesets.
Summary: DNR: Implement versioning on the flatbuffer ruleset file. (was: DNR: Decide on how to handle flatbuffer schema evolution.)
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 23

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

commit 586fbb9525faa66016e2c335422f721d5b45cf68
Author: Karan Bhatia <karandeepb@chromium.org>
Date: Thu Aug 23 05:08:29 2018

DNR: Implement versioning for indexed ruleset format.

This CL implements versioning for indexed ruleset format. The indexed ruleset is
now prefixed with a version header. On loading an extension, if Chrome detects a
version mismatch between the current ruleset format version used by Chrome and
the one used in the extension ruleset, the extension ruleset will be reindexed.

This is necessary to support making breaking changes to the flatbuffer schema
used for extension indexed rulesets.

BUG= 755717 

Change-Id: I7e3947a15e66b556c864d1031899bfbe9702dfc5
Reviewed-on: https://chromium-review.googlesource.com/1152872
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585402}
[modify] https://crrev.com/586fbb9525faa66016e2c335422f721d5b45cf68/chrome/browser/extensions/api/declarative_net_request/declarative_net_request_browsertest.cc
[modify] https://crrev.com/586fbb9525faa66016e2c335422f721d5b45cf68/chrome/browser/extensions/api/declarative_net_request/ruleset_matcher_unittest.cc
[modify] https://crrev.com/586fbb9525faa66016e2c335422f721d5b45cf68/extensions/browser/api/declarative_net_request/rules_monitor_service.cc
[modify] https://crrev.com/586fbb9525faa66016e2c335422f721d5b45cf68/extensions/browser/api/declarative_net_request/rules_monitor_service.h
[modify] https://crrev.com/586fbb9525faa66016e2c335422f721d5b45cf68/extensions/browser/api/declarative_net_request/ruleset_matcher.cc
[modify] https://crrev.com/586fbb9525faa66016e2c335422f721d5b45cf68/extensions/browser/api/declarative_net_request/ruleset_matcher.h
[modify] https://crrev.com/586fbb9525faa66016e2c335422f721d5b45cf68/extensions/browser/api/declarative_net_request/utils.cc
[modify] https://crrev.com/586fbb9525faa66016e2c335422f721d5b45cf68/extensions/browser/api/declarative_net_request/utils.h
[modify] https://crrev.com/586fbb9525faa66016e2c335422f721d5b45cf68/extensions/browser/extension_prefs.cc
[modify] https://crrev.com/586fbb9525faa66016e2c335422f721d5b45cf68/extensions/browser/extension_prefs.h
[modify] https://crrev.com/586fbb9525faa66016e2c335422f721d5b45cf68/tools/metrics/histograms/enums.xml

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 25

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

commit 7ed729ec90e988bede17483a1895e9225a971d3e
Author: Karan Bhatia <karandeepb@chromium.org>
Date: Sat Aug 25 01:41:42 2018

UrlPatternIndex: Introduce format version and add correctness checks.

This CL
  - Introduces url_pattern_index::kUrlPatternIndexFormatVersion to keep a track
    of the current format version of the UrlPatternIndex schema.
  - Adds checks to url_pattern_index clients (Declarative net request and
    subresource_filter) to ensure that their ruleset format versions are updated
    whenever kUrlPatternIndexFormatVersion is changed.
  - Adds presubmit checks to components/url_pattern_index to ensure that
    kUrlPatternIndexFormatVersion is kept updated.

BUG= 877214 ,  755717 

Change-Id: Ib82b775e3da2112d5f17a2040ec495a7ff8e18d9
Reviewed-on: https://chromium-review.googlesource.com/1187706
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586098}
[modify] https://crrev.com/7ed729ec90e988bede17483a1895e9225a971d3e/components/subresource_filter/core/common/indexed_ruleset.cc
[add] https://crrev.com/7ed729ec90e988bede17483a1895e9225a971d3e/components/url_pattern_index/PRESUBMIT.py
[modify] https://crrev.com/7ed729ec90e988bede17483a1895e9225a971d3e/components/url_pattern_index/flat/url_pattern_index.fbs
[modify] https://crrev.com/7ed729ec90e988bede17483a1895e9225a971d3e/components/url_pattern_index/url_pattern_index.h
[modify] https://crrev.com/7ed729ec90e988bede17483a1895e9225a971d3e/extensions/browser/api/declarative_net_request/utils.cc

Status: Fixed (was: Assigned)
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 30

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

commit e7d5894eae9a4d306e8be22bf649abf61a24faf6
Author: Karan Bhatia <karandeepb@chromium.org>
Date: Thu Aug 30 01:37:31 2018

DNR: Add unit test to ensure that indexed ruleset format version is kept updated.

This CL adds a unit test to ensure that indexed ruleset format version is kept
updated. It does so by detecting any changes to the flatbuffer schema.

BUG= 755717 

Change-Id: I38acd9c5042b4742f559d45167ebfebf6f8b0ff0
Reviewed-on: https://chromium-review.googlesource.com/1187884
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587389}
[modify] https://crrev.com/e7d5894eae9a4d306e8be22bf649abf61a24faf6/extensions/browser/BUILD.gn
[modify] https://crrev.com/e7d5894eae9a4d306e8be22bf649abf61a24faf6/extensions/browser/api/declarative_net_request/flat/extension_ruleset.fbs
[add] https://crrev.com/e7d5894eae9a4d306e8be22bf649abf61a24faf6/extensions/browser/api/declarative_net_request/indexed_ruleset_format_version_unittest.cc
[modify] https://crrev.com/e7d5894eae9a4d306e8be22bf649abf61a24faf6/extensions/browser/api/declarative_net_request/utils.cc
[modify] https://crrev.com/e7d5894eae9a4d306e8be22bf649abf61a24faf6/extensions/browser/api/declarative_net_request/utils.h

Sign in to add a comment