1335549 - Enable necessary table updates when plugins.flashBlock.enabled is flipped

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Robin Steuber (they/them) [:bytesized]]

Currently, the SafeBrowsing module checks which tables should be updated on start up. However, when the |plugins.flashBlock.enabled| pref is flipped, it should immediately trigger an update of the flash blocking lists.

Comment 1

[François Marier [:francois]]

Probably just needs to be added to http://searchfox.org/mozilla-central/rev/4e0c5c460318fb9ef7d92b129ac095ce04bc4795/toolkit/components/url-classifier/SafeBrowsing.jsm#46-54 and http://searchfox.org/mozilla-central/rev/4e0c5c460318fb9ef7d92b129ac095ce04bc4795/toolkit/components/url-classifier/SafeBrowsing.jsm#64-66.

It would be better to roll it into the main patch. I don't see any reasons for leaving this out.

Comment 2

[Robin Steuber (they/them) [:bytesized]]

@francois It is possible that it is as simple as adding a few lines, but I have two reasons that I would rather not integrate the change into the patch for Bug 1307604. First, that patch has just about finished a rather lengthy review process which I would prefer not to reopen. Second, I want to make sure that the change has the desired effect and I would rather keep track of this (new) issue in a new bug.

Comment 3

[Robin Steuber (they/them) [:bytesized]]

Is it sufficient to update on changes to |plugins.flashBlock.enabled|? Do we need to update on changes to the table names (ex: |urlclassifier.flashAllowTable|)?

Comment 4

[:Felipe Gomes (needinfo for replies!)]

My understanding is that we won't need to update the table names during the study.. is that right? It's only the contents of the tables that get updated.

If that's correct, then just watching for plugins.flashBlock.enabled is fine

Comment 5

[François Marier [:francois]]

(In reply to :Felipe Gomes (needinfo me!) from comment #4)
> My understanding is that we won't need to update the table names during the
> study.. is that right? It's only the contents of the tables that get updated.
>
> If that's correct, then just watching for plugins.flashBlock.enabled is fine

I would actually suggest that we watch the urclassifier.*Table prefs too so that tables are updated consistently across all types of URL classifier lists, instead of flashblocking being a special case.

Comment 6

[Robin Steuber (they/them) [:bytesized]]

@francois Ah! I had missed this line: [1]. So changes to |urlclassifier.*Table| already propagate through the SafeBrowsing module. Excellent!

It looks like we just need a single line addition: |Services.prefs.addObserver("plugins.flashBlock.enabled", this, false);|

After taking a look at the code, it looks like the SafeBrowsing module will do what we need with just that one line. The one remaining thing that we would like is to be able to know when necessary changes have propagated. Is there an observer that can be used or function call that can be added to allow for notifications when the tables receive updates?

My understanding is that the plugin deploying the Flash blocking experiment needs to know when updates corresponding to certain phases of the test are received. We were discussing the possibility of having it observe the Flash Blocking tables and check for a known (dummy) entry in the table to determine this. Is this doable? Does such an observable event already exist?

[1] http://searchfox.org/mozilla-central/rev/4e0c5c460318fb9ef7d92b129ac095ce04bc4795/toolkit/components/url-classifier/SafeBrowsing.jsm#66

Comment 7

[François Marier [:francois]]

(In reply to Kirk Steuber [:bytesized] from comment #6)
> It looks like we just need a single line addition:
> |Services.prefs.addObserver("plugins.flashBlock.enabled", this, false);|

Yes, that should be all you need.

> After taking a look at the code, it looks like the SafeBrowsing module will
> do what we need with just that one line. The one remaining thing that we
> would like is to be able to know when necessary changes have propagated. Is
> there an observer that can be used or function call that can be added to
> allow for notifications when the tables receive updates?

We don't have an event like that but maybe you could watch for changes to browser.safebrowsing.provider.mozilla.lastupdatetime since that pref is set after a successful update.

Comment 8

[Robin Steuber (they/them) [:bytesized]]

Attachment: Bug 1335549 - Enable table updates when flashblock pref is flipped

Review commit: https://reviewboard.mozilla.org/r/110452/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110452/

Comment 9

[François Marier [:francois]]

Comment on attachment 8834585 [details]
Bug 1335549 - Enable table updates when flashblock pref is flipped

https://reviewboard.mozilla.org/r/110452/#review111884

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09cbce8b9b21
Enable table updates when flashblock pref is flipped r=francois

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/09cbce8b9b21

Comment 12

[Robin Steuber (they/them) [:bytesized]]

Comment on attachment 8834585 [details]
Bug 1335549 - Enable table updates when flashblock pref is flipped

Approval Request Comment
[Feature/Bug causing the regression]: This feature is needed for the Shield study to be run on Release 52 (bug 1335232), which we'll use to study the effect of making flash click-to-play by default.
[User impact if declined]: Shield study will need to restart the browser in order to work correctly
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Not for the feature independently. We'll do QE on the study as a whole to make sure all pieces work as expected
[List of other uplifts needed for the feature/fix]: bug 1307604
[Is the change risky?]: No
[Why is the change risky/not risky?]: Adds single pref to the list of prefs SafeBrowsing tracks.
[String changes made/needed]: none

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8834585 [details]
Bug 1335549 - Enable table updates when flashblock pref is flipped

For shield study purpose. Aurora53+.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/47533612715f

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 8834585 [details]
Bug 1335549 - Enable table updates when flashblock pref is flipped

this was deemed too risky for beta