1482306 - Race condition when multiple tables are using the same underlying lists

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[François Marier [:francois]]

I noticed that if I disable TP but leave annotations enabled (for example, by turning off content blocking in bug 1480450) I get the following:

SafeBrowsing: 16:37:46 GMT-0700 (PDT): phishingEnabled: true malwareEnabled: true downloadsEnabled: true passwordsEnabled: false trackingEnabled: false blockedEnabled: true trackingAnnotations true flashBlockEnabled true flashInfobarListEnabled: false
...
listmanager: 16:37:46 GMT-0700 (PDT): Enabling table updates for base-track-digest256
listmanager: 16:37:46 GMT-0700 (PDT): Enabling table updates for mozstd-trackwhite-digest256
listmanager: 16:37:46 GMT-0700 (PDT): Disabling table updates for base-track-digest256
listmanager: 16:37:46 GMT-0700 (PDT): Disabling table updates for mozstd-trackwhite-digest256
...
listmanager: 16:37:46 GMT-0700 (PDT): needsUpdate: {
...
  "https://shavar.services.mozilla.com/downloads?client=Firefox&appver=63.0a&pver=2.2": {
    "base-track-digest256": false,
    "mozstd-trackwhite-digest256": false,
...
  }
}

In other words, there's a race condition. If the disabling feature is first, the second one will re-enable the lists, but if the disabling feature goes second, then the lists will be flagged as disabled.

While tableList makes it seem like it still requests the tables:

listmanager: 16:38:05 GMT-0700 (PDT): forceUpdates with base-track-digest256,mozstd-trackwhite-digest256,content-track-digest256,mozplugin-block-digest256,mozplugin2-block-digest256,block-flash-digest256,except-flash-digest256,allow-flashallow-digest256,except-flashallow-digest256,block-flashsubdoc-digest256,except-flashsubdoc-digest256,except-flashinfobar-digest256,ads-track-digest256,social-track-digest256,analytics-track-digest256
listmanager: 16:38:05 GMT-0700 (PDT): checkForUpdates with https://shavar.services.mozilla.com/downloads?client=Firefox&appver=63.0a&pver=2.2
listmanager: 16:38:05 GMT-0700 (PDT): this.tablesData: {
...
  "base-track-digest256": {
    "updateUrl": "https://shavar.services.mozilla.com/downloads?client=Firefox&appver=63.0a&pver=2.2",
    "gethashUrl": "https://shavar.services.mozilla.com/gethash?client=Firefox&appver=63.0a&pver=2.2",
    "provider": "mozilla"
  },
  "mozstd-trackwhite-digest256": {
    "updateUrl": "https://shavar.services.mozilla.com/downloads?client=Firefox&appver=63.0a&pver=2.2",
    "gethashUrl": "https://shavar.services.mozilla.com/gethash?client=Firefox&appver=63.0a&pver=2.2",
    "provider": "mozilla"
  },
...
}
...
listmanager: 16:38:05 GMT-0700 (PDT): update request: {
  "tableList": "base-track-digest256,mozstd-trackwhite-digest256,mozplugin-block-digest256,allow-flashallow-digest256,except-flashallow-digest256,block-flash-digest256,except-flash-digest256,block-flashsubdoc-digest256,except-flashsubdoc-digest256,except-flashinfobar-digest256",
  "tableNames": {},
  "requestPayload": "except-flashallow-digest256;a:1490633678\nmozplugin-block-digest256;a:1471849627\nblock-flashsubdoc-digest256;a:1512160865\nexcept-flash-digest256;a:1494877265\nexcept-flashsubdoc-digest256;a:1517935265\nallow-flashallow-digest256;a:1490633678\nblock-flash-digest256;a:1496263270\n",
  "isPostRequest": true
}

The request payload doesn't include them:

listmanager: 16:38:05 GMT-0700 (PDT): makeUpdateRequestForEntry_: requestPayload except-flashallow-digest256;a:1490633678
mozplugin-block-digest256;a:1471849627
block-flashsubdoc-digest256;a:1512160865
except-flash-digest256;a:1494877265
except-flashsubdoc-digest256;a:1517935265
allow-flashallow-digest256;a:1490633678
block-flash-digest256;a:1496263270
 update: https://shavar.services.mozilla.com/downloads?client=Firefox&appver=63.0a&pver=2.2 tablelist: base-track-digest256,mozstd-trackwhite-digest256,mozplugin-block-digest256,allow-flashallow-digest256,except-flashallow-digest256,block-flash-digest256,except-flash-digest256,block-flashsubdoc-digest256,except-flashsubdoc-digest256,except-flashinfobar-digest256

listmanager: 16:38:05 GMT-0700 (PDT): update success for base-track-digest256,mozstd-trackwhite-digest256,mozplugin-block-digest256,allow-flashallow-digest256,except-flashallow-digest256,block-flash-digest256,except-flash-digest256,block-flashsubdoc-digest256,except-flashsubdoc-digest256,except-flashinfobar-digest256 from https://shavar.services.mozilla.com/downloads?client=Firefox&appver=63.0a&pver=2.2: 3600

and if I delete the cached files in obj-x86_64-pc-linux-gnu/tmp/profile-default/safebrowsing/ and re-trigger an update of the mozilla provider, the TP lists are not downloaded:

$ ls obj-x86_64-pc-linux-gnu/tmp/profile-default/safebrowsing/
allow-flashallow-digest256.pset      except-flashallow-digest256.pset      google4                            test-harmful-simple.sbstore  test-track-simple.sbstore
allow-flashallow-digest256.sbstore   except-flashallow-digest256.sbstore   mozplugin-block-digest256.pset     test-malware-simple.pset     test-trackwhite-simple.pset
block-flash-digest256.pset           except-flash-digest256.pset           mozplugin-block-digest256.sbstore  test-malware-simple.sbstore  test-trackwhite-simple.sbstore
block-flash-digest256.sbstore        except-flash-digest256.sbstore        test-block-simple.pset             test-phish-simple.pset       test-unwanted-simple.pset
block-flashsubdoc-digest256.pset     except-flashsubdoc-digest256.pset     test-block-simple.sbstore          test-phish-simple.sbstore    test-unwanted-simple.sbstore
block-flashsubdoc-digest256.sbstore  except-flashsubdoc-digest256.sbstore  test-harmful-simple.pset           test-track-simple.pset

Comment 1

[François Marier [:francois]]

Attachment: Bug 1482306 - Ensure that tables are enabled when shared between features. r=dimi!

The enable/disable logic of the list manager was wrong. If multiple features
shared a given table (e.g. tracking protection and tracking annotations)
and only one of them was enabled, then updates could either be disabled
or enabled depending on which feature was checked first.

By disabling everything at the beginning and selectively re-enabling tables,
we can ensure that no table gets both enabled and disabled by different
feature toggles.

Comment 2

[Dimi Lee [:dimi]]

Comment on attachment 8999741 [details]
Bug 1482306 - Ensure that tables are enabled when shared between features. r=dimi!

Dimi Lee[:dimi][:dlee] has approved the revision.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6560b92dbe64
Ensure that tables are enabled when shared between features. r=dimi!

Comment 4

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/6560b92dbe64