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

VERIFIED FIXED in Firefox 47

Status

()

Core
Plug-ins
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: cpeterson, Assigned: tobytailor)

Tracking

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 unaffected, firefox47+ fixed, firefox48 wontfix, firefox49 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
Created attachment 8745024 [details] [diff] [review]
e10s-incompatible fix
(Assignee)

Updated

a year ago
Attachment #8745024 - Flags: review?(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).
Attachment #8745024 - Flags: review?(francois)
(Reporter)

Comment 3

a year ago
[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.
status-firefox49: --- → affected
tracking-firefox47: --- → ?
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.
tracking-firefox47: ? → +
(Assignee)

Comment 5

a year ago
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.
tracking-firefox47: + → ?
Flags: needinfo?(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.
Attachment #8745024 - Flags: review+
Flags: needinfo?(francois)
(Assignee)

Comment 7

a year ago
Created attachment 8746092 [details] [diff] [review]
e10s-incompatible fix (v2)

There you go: https://bugzilla.mozilla.org/show_bug.cgi?id=1268120.
Attachment #8746092 - Flags: review?(francois)
(Assignee)

Updated

a year ago
Attachment #8745024 - Attachment is obsolete: true
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
Attachment #8746092 - Flags: review?(francois) → review+
(Assignee)

Comment 9

a year ago
Created attachment 8746107 [details] [diff] [review]
e10s-incompatible fix (v3)

Nit.
Attachment #8746092 - Attachment is obsolete: true
Attachment #8746107 - Flags: review+
(Reporter)

Comment 10

a year ago
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
Attachment #8746107 - Flags: approval-mozilla-beta?
(Reporter)

Comment 11

a year ago
(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 on attachment 8746107 [details] [diff] [review]
e10s-incompatible fix (v3)

We need this for plugin block experiment, Beta47+
Attachment #8746107 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

a year ago
tracking-firefox47: ? → +
(Reporter)

Comment 13

a year ago
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
status-firefox47: affected → fixed
status-firefox48: affected → wontfix
status-firefox49: affected → wontfix
Summary: Plugin block list blocks SWF network requests, but does not prevent plugin instantiation → Plugin block list blocks SWF network requests, but does not prevent plugin instantiation (e10s-incompatible fix for Beta 47)
(Reporter)

Updated

a year ago
Blocks: 1268716
(Reporter)

Comment 14

a year ago
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
Flags: needinfo?(tschneider)
(Assignee)

Comment 15

a year ago
> 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);
(Assignee)

Comment 16

a year ago
Created attachment 8747401 [details] [diff] [review]
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.
Attachment #8746107 - Attachment is obsolete: true
Flags: needinfo?(tschneider)
(Reporter)

Comment 17

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

Comment 18

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

Comment 19

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c637d7478937
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Reporter)

Comment 20

a year ago
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.
Attachment #8747401 - Flags: approval-mozilla-beta?
Duplicate of this bug: 1268245
Comment on attachment 8747401 [details] [diff] [review]
e10s-incompatible fix (v4)

Needed for "plugin block experiment" in Beta47, let's uplift.
Attachment #8747401 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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!
status-firefox47: fixed → affected
Flags: needinfo?(wkocher)
(Reporter)

Comment 24

a year ago
https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=cd2eec139a29
(Reporter)

Updated

a year ago
status-firefox47: affected → fixed
status-firefox49: wontfix → fixed
Is this "wontfix" for 48 because it's just temporary? Or should this be uplifted?
Flags: needinfo?(tschneider)
(Reporter)

Comment 26

a year ago
(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
Flags: needinfo?(wkocher)
Verified 47.0b3; Plugin Block Experiment 
Windows 10x64
Windows 10x32
Mac 10.11
Status: RESOLVED → VERIFIED
(Reporter)

Updated

a year ago
Blocks: 1271384
(Assignee)

Updated

a year ago
Flags: needinfo?(tschneider)
(Reporter)

Updated

a year ago
Blocks: 1268120
You need to log in before you can comment on or make changes to this bug.