1369160 - Add a plugin blocklist that will suppress the infobar but not Flash itself

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, enhancement)

Description

[Benjamin Smedberg]

There are some topsites which occasionally legitimately are using Flash but also show infobars which are annoying to users. As a step between blocking Flash entirely and annoying users with infobars, we want the ability to only block the infobar.

The lego icon will still appear in the infobar and users can choose it, or click on visible Flash content.

If a site appears on the list, it should not trigger an infobar in either of the following situations:

* the toplevel site loaded in the browser is from that domain or a subdomain
* the hidden Flash which would trigger an infobar is loaded by the site (in a frame)

Felipe or Doug, I'll need help/guidance setting up the new shavar lists, but the rest of this I can write. I think we can safely download these tables by default, so we don't need to add complex code to switch the tables on and off via pref.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Kirk should be able to help more than myself since he was the one who set up the flash shavar lists for bug 1307604

Comment 2

[Robin Steuber (she/her) [:bytesized]]

I am happy to help, but i am not sure exactly what help you need. I believe you want me to do one or both of these things:

 - Have the shavar (staging and production?) servers serve a new list called something like "except-infobars-digest256"
 - Change SafeBrowsing.jsm to load the new list (if the pref |plugins.flashBlock.enabled| is true?)  and register it with the URL classifier.

Does that sound right? Do you want me to do both?

Comment 3

[Benjamin Smedberg]

I'd like both of those things, yes. Don't pref this on plugins.flashBlock.enabled, it can be on by default.

And after reading code, another question. Your code for classification uses urlclassifier.classifyLocalWithTables, but that is a [noscript] method and the code for infobars is JS.

urlclassifier.classifyLocal appears to be an identical scriptable equivalent, but it says that is internal and only for testing purposes: http://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/netwerk/base/nsIURIClassifier.idl#101

is .classifyLocal the correct call, and if so should I remove that comment about it being testing-only?

Comment 4

[:Felipe Gomes (needinfo for replies!)]

You should be using the newer asyncClassifyWithTables, which was recently added (FF 54) and is now the preferred method. Bug 1345058 is converting the flash code to use it instead of the sync version.

Comment 5

[Robin Steuber (she/her) [:bytesized]]

Felipe is correct that the async version is really what should be used. However, since the current flash blocking implementation still falls back on the synchronous version, I will answer the question about urlclassifier.classifyLocal in case you need it.

A while back, I changed ClassifyLocalWithTables to store its results in an |nsTArray<nsCString>| rather than a comma-separated string. However, a test script already used classifyLocalWithTables, and I am afraid that I (still) do not know how to return an array from C++ in a scriptable way. I tried to figure it out, but ultimately it did not seem to be worth the effort to fix it for a test script, so I just wrote a scriptable wrapper that would call ClassifyLocalWithTables and convert the result to a comma-separated string, which I could easily return in a scriptable manner [1].

However, I never intended it to be used outside of tests because the array to string conversion introduces totally unnecessary overhead. If you want to make comparisons with the list content, it is especially inefficient since you would end up needing to convert it back to an array anyways. However, if you have no problem with that inefficiency, it is perfectly usable.

It is worth noting, however, that you will most likely be using the classification function the same way that most consumers use it: All they really want is the boolean result of "did anything on the table match?" 

So ultimately I think that there are three valid solutions to this problem, should you need the synchronous local classification:
 - Accept the inefficiency and use urlclassifier.classifyLocal as-is.
 - Write a new version of classifyLocal (possibly replacing the old one) that eliminates the overhead by replacing the array to string conversion with an array to boolean conversion. I believe that this is approach will work for the existing consumer of urlclassifier.classifyLocal anyways.
 - Fix classifyLocal so that it actually returns an array.


Also, it is worth noting that asyncClassifyWithTables's output *should* also be converted to return an array, which will introduce the same problems with it that we are having with classifyLocal. I believe that currently it does the unnecessary array to string conversion that ClassifyLocalWithTables used to do.

[1] http://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#1821

Comment 6

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1369160 - Add a Shavar list to be used to suppress the infobar

The Flash infobar is showing up a bit too often. There are a few changes planned to address this. One of them is a list that will contain sites that should never display infobars. This patch allows the list to be downloaded from Shavar and updated via the URL classifier.

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

Comment 7

[Robin Steuber (she/her) [:bytesized]]

The patch should be ready (pending review), but the Shavar lists are not yet deployed.

Comment 8

[Chris Kolosiwsky [:ckolos] (ckolos has left the building)]

:bytesized - I am the wrong person to review this. I suggest fmarier or lcrouch.

Comment 9

[[github robot]]

Commits pushed to master at https://github.com/mozilla-services/shavar-list-creation

https://github.com/mozilla-services/shavar-list-creation/commit/e49fb710f4255e6ced9cda9496d0ad0eae1ebb9d
bug 1369160: add infobar-exceptions

See also mozilla-services/shavar-list-creation-config#21

https://github.com/mozilla-services/shavar-list-creation/commit/4611d0a8e94d67a0e19d757a647ee9482379f396
Merge pull request #46 from mozilla-services/infobar-exceptions-1369160

bug 1369160: add infobar-exceptions

Comment 10

[François Marier [:francois]]

Comment on attachment 8873218 [details]
Bug 1369160 - Add a Shavar list to be used to suppress the infobar

https://reviewboard.mozilla.org/r/144668/#review149920

That looks fine, but we should group it with the rest of the flash lists. So I'd use "flashinfobar" instead of "infobars" (also singular to match the other lists).

r+ with the above change.

Comment 11

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8873218 [details]
Bug 1369160 - Add a Shavar list to be used to suppress the infobar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144668/diff/1-2/

Comment 12

[Benjamin Smedberg]

Attachment: Bug 1369160 part B - use the new list to suppress infobars

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

Comment 13

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8874977 [details]
Bug 1369160 part B - use the new list to suppress infobars

https://reviewboard.mozilla.org/r/146334/#review150362

I'm not very familiar with this area of the codebase, so I suspect felipe's review may be a bit more informed than mine. That being said, this looks good to me.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8874977 [details]
Bug 1369160 part B - use the new list to suppress infobars

https://reviewboard.mozilla.org/r/146334/#review150374

Comment 15

[Chris Kolosiwsky [:ckolos] (ckolos has left the building)]

Filed:
https://github.com/mozilla-services/shavar-list-creation/pull/48
https://github.com/mozilla-services/shavar-list-creation-config/pull/23
and
https://github.com/mozilla-services/shavar-server-list-config/pull/10

For list name change

Comment 16

[Chris Kolosiwsky [:ckolos] (ckolos has left the building)]

All merged.

curl https://shavar.stage.mozaws.net/list
allow-flashallow-digest256
base-track-digest256
baseeff-track-digest256
basew3c-track-digest256
block-flash-digest256
block-flashsubdoc-digest256
content-track-digest256
contenteff-track-digest256
contentw3c-track-digest256
except-flash-digest256
except-flashallow-digest256
except-flashinfobar-digest256
except-flashsubdoc-digest256
mozfull-track-digest256
mozfullstaging-track-digest256
mozplugin-block-digest256
mozpub-track-digest256
mozstd-track-digest256
mozstd-trackwhite-digest256
mozstdstaging-track-digest256
mozstdstaging-trackwhite-digest256
moztestpub-track-digest256
moztestpub-trackwhite-digest256

Comment 17

[Robin Steuber (she/her) [:bytesized]]

Everything is ready on the Shavar servers. This can be merged if it is all ready!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2322c37ad902
Add a Shavar list to be used to suppress the infobar, r=francois
https://hg.mozilla.org/integration/mozilla-inbound/rev/26c76000d709
part B - use the new list to suppress infobars, r=ksteuber r=felipe

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2322c37ad902
https://hg.mozilla.org/mozilla-central/rev/26c76000d709

Comment 20

[Abe - QA (:Abe_LV)]

Verified as fixed. 
It is tested with Bug 1371819.
Test cases and runs are here: https://public.etherpad-mozilla.org/p/1371819
needinfo me if you have feedbacks or questions.