Closed Bug 1369160 Opened 5 years ago Closed 5 years ago
Add a plugin blocklist that will suppress the infobar but not Flash itself
59 bytes, text/x-review-board-request
59 bytes, text/x-review-board-request
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.
Kirk should be able to help more than myself since he was the one who set up the flash shavar lists for bug 1307604
Flags: needinfo?(felipc) → needinfo?(ksteuber)
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?
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?
Flags: needinfo?(benjamin) → needinfo?(ksteuber)
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.
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 . 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.  http://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#1821
Attachment #8873218 - Flags: review?(ckolos)
The patch should be ready (pending review), but the Shavar lists are not yet deployed.
:bytesized - I am the wrong person to review this. I suggest fmarier or lcrouch.
Attachment #8873218 - Flags: review?(ckolos) → review?(francois)
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 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.
Attachment #8873218 - Flags: review?(francois) → review+
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.
Attachment #8874977 - Flags: review?(ksteuber) → review+
Comment on attachment 8874977 [details] Bug 1369160 part B - use the new list to suppress infobars https://reviewboard.mozilla.org/r/146334/#review150374
Attachment #8874977 - Flags: review?(felipc) → review+
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
Everything is ready on the Shavar servers. This can be merged if it is all ready!
Pushed by firstname.lastname@example.org: 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
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.