Closed Bug 1266889 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Plug-ins, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox46 --- unaffected
firefox47 + fixed
firefox48 --- wontfix
firefox49 --- fixed

People

(Reporter: cpeterson, Assigned: tschneider)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Attached patch e10s-incompatible fix (obsolete) — Splinter Review
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)
[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.
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.
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.
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)
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+
Attached patch e10s-incompatible fix (v3) (obsolete) — Splinter Review
Nit.
Attachment #8746092 - Attachment is obsolete: true
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?
(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+
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
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)
Blocks: 1268716
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)
> 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);
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)
https://hg.mozilla.org/mozilla-central/rev/c637d7478937
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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!
Flags: needinfo?(wkocher)
Is this "wontfix" for 48 because it's just temporary? Or should this be uplifted?
Flags: needinfo?(tschneider)
(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
Verified 47.0b3; Plugin Block Experiment 
Windows 10x64
Windows 10x32
Mac 10.11
Status: RESOLVED → VERIFIED
Blocks: 1271384
Flags: needinfo?(tschneider)
Blocks: 1268120
You need to log in before you can comment on or make changes to this bug.