1266889 - Plugin block list blocks SWF network requests, but does not prevent plugin instantiation (e10s-incompatible fix for Beta 47)

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Chris Peterson [:cpeterson]]

We don't want to load the plugin when we're going to block the SWF anyways. We need to check the block list before instantiating the plugin.

Comment 1

[Tobias Schneider [:tobytailor]]

Attachment: e10s-incompatible fix

Comment 2

[François Marier [:francois]]

Comment on attachment 8745024 [details] [diff] [review]
e10s-incompatible fix

Review of attachment 8745024 [details] [diff] [review]:
-----------------------------------------------------------------

I just thought of a simpler approach: just before doing the initialization, create a dummy channel with the same URL. If the channel creation fails, set allowLoad to false.

Would that work on e10s?

Another advantage of this (other than the fact it should be a lot less code) is that it will also prevent plugin init when blocking trackers in Private Browsing (TP list).

Comment 3

[Chris Peterson [:cpeterson]]

[Tracking Requested - why for this release]:

rel man: once this fix lands in mozilla-central, we'll want to quickly uplift it to Beta 47 for our plugin block experiment.

Comment 4

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 47. I am ready to take this uplift to Beta47 as soon as it's ready and I really hope it's soon otherwise we will have to reduce the length of telemetry experiment in bug 1248813.

Comment 5

[Tobias Schneider [:tobytailor]]

François: Are you be ok with landing the e10s incompatible fix so we can start the experiment, and working on getting it to work with e10s before we finally ship? Will look into your suggested solution but don't think this should block the experiment.

Comment 6

[François Marier [:francois]]

Comment on attachment 8745024 [details] [diff] [review]
e10s-incompatible fix

Review of attachment 8745024 [details] [diff] [review]:
-----------------------------------------------------------------

> François: Are you be ok with landing the e10s incompatible fix so we can start the experiment, and working on getting it to work with e10s before we finally ship? Will look into your suggested solution but don't think this should block the experiment.

How about we add a comment before that block along the lines of "This needs to be reverted once the plugin stability experiment is over (see bug #123456)." and then file a bug to track the removal of this code?

With that small change, I'm OK with landing/uplifting this patch. I just want to make sure we don't lose track of it.

Comment 7

[Tobias Schneider [:tobytailor]]

Attachment: e10s-incompatible fix (v2)

There you go: https://bugzilla.mozilla.org/show_bug.cgi?id=1268120.

Comment 8

[François Marier [:francois]]

Comment on attachment 8746092 [details] [diff] [review]
e10s-incompatible fix (v2)

Review of attachment 8746092 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsObjectLoadingContent.cpp
@@ +2303,5 @@
>      // Content policy implementations can mutate the DOM, check for re-entry
>      if (!mIsLoading) {
>        LOG(("OBJLC [%p]: We re-entered in content policy, leaving original load",
>             this));
> +      

nit: if you have a chance to edit this patch before landing, you may as well remove this whitespace change

Comment 9

[Tobias Schneider [:tobytailor]]

Attachment: e10s-incompatible fix (v3)

Nit.

Comment 10

[Chris Peterson [:cpeterson]]

Comment on attachment 8746107 [details] [diff] [review]
e10s-incompatible fix (v3)

Approval Request Comment

[Feature/regressing bug #]: regression from bug 1237198

[User impact if declined]: We won't be able to run our plugin block experiment.

[Describe test coverage new/current, TreeHerder]:

[Risks and why]: Medium risk because this code has not landed on Nightly. However, the risk is reduced because this feature is disabled by default and will only be enabled for about 6% of Beta 47 users in our plugin block experiment (bug 1248813). We won't need this patch on Nightly or Aurora because they have e10s enabled, which will require a different fix.

[String/UUID change made/needed]: None

Comment 11

[Chris Peterson [:cpeterson]]

(In reply to Chris Peterson [:cpeterson] from comment #10)
> [String/UUID change made/needed]: None

well, technically there is a new string but it is only a debug warning in the browser console that is unlikely to be seen, even by web developers using Firefox devtools: "Blocking SWF since it was found on an internal Firefox blocklist."

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8746107 [details] [diff] [review]
e10s-incompatible fix (v3)

We need this for plugin block experiment, Beta47+

Comment 13

[Chris Peterson [:cpeterson]]

Thanks, Ritu. I'll open a new bug for the e10s-compatible fix for Aurora 48+.

https://hg.mozilla.org/releases/mozilla-beta/rev/11eb6b661fe4

Comment 14

[Chris Peterson [:cpeterson]]

Tobias, I had to back out your fix from mozilla-beta because it broke some tests:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=11eb6b661fe4b6d889af99aeec890a9f1706944a&selectedJob=1028622

19:04:35 INFO - 134 INFO TEST-UNEXPECTED-FAIL | dom/plugins/test/mochitest/test_bug777098.html | uncaught exception - TypeError: plugin.getObjectValue is not a function at http://mochi.test:8888/tests/dom/plugins/test/mochitest/test_bug777098.html:26
1905 19:04:35 INFO - 141 INFO TEST-UNEXPECTED-FAIL | dom/plugins/test/mochitest/test_bug827160.html | Testing object load without type, failed expected load - got 4, expected 2
1909 19:04:35 INFO - 142 INFO TEST-UNEXPECTED-FAIL | dom/plugins/test/mochitest/test_bug827160.html | Testing object load with type, failed expected load - got 4, expected 2

Comment 15

[Tobias Schneider [:tobytailor]]

> I just thought of a simpler approach: just before doing the initialization,
> create a dummy channel with the same URL. If the channel creation fails, set
> allowLoad to false.

I tried this, but the following code successfully creates a new channel without any errors:

  nsCOMPtr<nsIPrincipal> nullPrincipal = nsNullPrincipal::Create();
  nsCOMPtr<nsIChannel> tempChannel;
  rv = NS_NewChannel(getter_AddRefs(tempChannel),
                     mURI,
                     nullPrincipal,
                     nsILoadInfo::SEC_NORMAL,
                     nsIContentPolicy::TYPE_OTHER,
                     nullptr,
                     nullptr,
                     nsIRequest::LOAD_NORMAL | nsIChannel::LOAD_CLASSIFY_URI);

Comment 16

[Tobias Schneider [:tobytailor]]

Attachment: e10s-incompatible fix (v4)

Since safebrowsing is disabled for some test and might not be initialized at the time we use the channel classifier, I needed to add a check to make sure the blocking feature is enabled.

Comment 17

[Chris Peterson [:cpeterson]]

tryserver run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fa68b118648a7d7e4609573d51313cb87acb404

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c637d7478937

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c637d7478937

Comment 20

[Chris Peterson [:cpeterson]]

Comment on attachment 8747401 [details] [diff] [review]
e10s-incompatible fix (v4)

Approval Request Comment
[Feature/regressing bug #]: regression from bug 1237198

[User impact if declined]: We won't be able to run our plugin block experiment.

[Describe test coverage new/current, TreeHerder]: Tobias fixed the test failures (comment 16) that required us to back out of Beta after our previous uplift (comment 14).

[Risks and why]: This code has landed on mozilla-central and I have confirmed that the code is working:

1. Set "browser.safebrowsing.blockedURIs.enabled" pref to true.
2. Set "urlclassifier.blockedTable" pref to "test-block-simple,mozplugin-block-digest256,mozplugin2-block-digest256".
3. Reset "browser.safebrowsing.provider.mozilla.nextupdatetime" pref to 1.
4. Disable e10s.
5. Restart Firefox.
6. Open browser console (SHIFT+CMD+J).
7. Load http://www.nytimes.com/
8. Filter browser console messages for "found on an internal Firefox blocklist".

[String/UUID change made/needed]: We added some temporary debug console messages, but they won't be visible to end users.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8747401 [details] [diff] [review]
e10s-incompatible fix (v4)

Needed for "plugin block experiment" in Beta47, let's uplift.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Wes, the status-fx47 flag was reverted from fixed to affected so it shows up on your queries. NI you just to be safe. Thanks!

Comment 24

[Chris Peterson [:cpeterson]]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=cd2eec139a29

Comment 25

[Benjamin Smedberg]

Is this "wontfix" for 48 because it's just temporary? Or should this be uplifted?

Comment 26

[Chris Peterson [:cpeterson]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #25)
> Is this "wontfix" for 48 because it's just temporary? Or should this be
> uplifted?

This fix is temporary and not e10s compatible and, as e10s is enabled by default in Aurora, there would not be much point to uplifting. Tobias is working on an e10s-compatible fix in bug 1268716.

That said, there would be no harm in uplifting this to Aurora 48 if you want consistently with Nightly and Beta. The fix landed in Beta 47 this morning:

https://hg.mozilla.org/releases/mozilla-beta/rev/cd2eec139a29713bfc9a254a47c5ecda50d639a3

Comment 27

[Michelle Funches - QA]

Verified 47.0b3; Plugin Block Experiment 
Windows 10x64
Windows 10x32
Mac 10.11