Closed Bug 1335549 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- wontfix
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: bytesized, Assigned: bytesized)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
@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.
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|)?
Flags: needinfo?(felipc)
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
Flags: needinfo?(felipc)
(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.
@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
Flags: needinfo?(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.
Flags: needinfo?(francois)
Assignee: nobody → ksteuber
Attachment #8834585 - Flags: review?(francois)
Comment on attachment 8834585 [details]
Bug 1335549 - Enable table updates when flashblock pref is flipped

https://reviewboard.mozilla.org/r/110452/#review111884
Attachment #8834585 - Flags: review?(francois) → review+
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/09cbce8b9b21
Enable table updates when flashblock pref is flipped r=francois
https://hg.mozilla.org/mozilla-central/rev/09cbce8b9b21
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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
Attachment #8834585 - Flags: approval-mozilla-beta?
Attachment #8834585 - Flags: approval-mozilla-aurora?
Comment on attachment 8834585 [details]
Bug 1335549 - Enable table updates when flashblock pref is flipped

For shield study purpose. Aurora53+.
Attachment #8834585 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8834585 [details]
Bug 1335549 - Enable table updates when flashblock pref is flipped

this was deemed too risky for beta
Attachment #8834585 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.