Closed Bug 1369160 Opened 6 years ago Closed 6 years ago

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


(Core Graveyard :: Plug-ins, enhancement)

Not set


(firefox55 fixed)

Tracking Status
firefox55 --- fixed


(Reporter: benjamin, Assigned: benjamin)




(2 files)

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.
Flags: needinfo?(felipc)
Flags: needinfo?(dothayer)
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?
Flags: needinfo?(ksteuber)
Flags: needinfo?(dothayer)
Flags: needinfo?(benjamin)
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:

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 [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.

Flags: needinfo?(ksteuber)
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)
Comment on attachment 8873218 [details]
Bug 1369160 - Add a Shavar list to be used to suppress the infobar

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

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
Attachment #8874977 - Flags: review?(felipc) → review+
All merged.

Everything is ready on the Shavar servers. This can be merged if it is all ready!
Pushed by
Add a Shavar list to be used to suppress the infobar, r=francois
part B - use the new list to suppress infobars, r=ksteuber r=felipe
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Verified as fixed. 
It is tested with Bug 1371819.
Test cases and runs are here:
needinfo me if you have feedbacks or questions.
Depends on: 1435098
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.