Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Make plugin content blocking work on e10s

RESOLVED FIXED in Firefox 49

Status

()

Core
Plug-ins
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: tobytailor, Assigned: tobytailor)

Tracking

(Depends on: 2 bugs)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s-, firefox47 wontfix, firefox48 wontfix, firefox49 fixed, firefox50 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

a year ago
Our current plugin content blocking uses a e10s incompatible solution. This should be fixed before we finally decide to ship this feature.
Another advantage of fixing this (even if we don't ship plugin blocking) is that we could avoid initializing plugins for resources that are going to get blocked by Tracking Protection anyways.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1266889#c2

Updated

a year ago
tracking-e10s: --- → ?
Toby does this affect tracking protection in private browsing or just the new plugin blocking behavior?
Flags: needinfo?(tschneider)
Comment 1 sounds like something that should be split out into a separate bug. Also, is there a tracking bug associated with plugin blocking this should be hooked up to?

Sounds like the e10s team doesn't need to block on this since we only block on bugs associated with release features. New features need to be e10s compat before they roll out.
(Assignee)

Comment 4

a year ago
Benjamin: Yes, this should also avoid initializing the Flash Player if content gets Blocked by tracking Protection.
Flags: needinfo?(tschneider)
Jim, this is the e10s half of bug 1266889.

Since this is new, it's not considered an e10s blocker, and the feature team is responsible for making new work e10s compatible. But we still need this.

Updated

a year ago
tracking-e10s: ? → -
(Assignee)

Comment 6

a year ago
Created attachment 8761876 [details] [diff] [review]
Make plugin content blocking work on e10s

Make plugin content blocking work on e10s
(Assignee)

Updated

a year ago
Attachment #8761876 - Attachment description: bug1268120.diff → Make plugin content blocking work on e10s
(Assignee)

Comment 7

a year ago
Created attachment 8763200 [details] [diff] [review]
Make plugin content blocking work on e10s (v2)

Fixed ShouldBlockContent().
Attachment #8761876 - Attachment is obsolete: true
(Assignee)

Comment 8

a year ago
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=afb5acf4f2d0&selectedJob=22529192
Blocks: 1277876, 1237198
status-firefox47: --- → wontfix
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox50: --- → affected
Depends on: 1266889
Duplicate of this bug: 1268716
No longer blocks: 1277876
(Assignee)

Updated

a year ago
Attachment #8763200 - Flags: review?(benjamin)
(Assignee)

Updated

a year ago
Attachment #8763200 - Flags: review?(benjamin) → review?(francois)
François, do you have an ETA for reviewing Tobias' e10s fix for the plugin blocklist? If everything looks good, we'd like to uplift the e10s fix and enable the plugin blocklist by default in Beta 48.
Assignee: nobody → tschneider
Needinfo'ing François to make sure he sees Chris's question. My interest in this is that I will start working on bug 1282484 soon and I'll probably have this code as its foundation and I want to start from an e10s-ready base.
Flags: needinfo?(francois)
Comment on attachment 8763200 [details] [diff] [review]
Make plugin content blocking work on e10s (v2)

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

::: dom/base/nsObjectLoadingContent.cpp
@@ +1130,5 @@
>  
> +  nsresult status;
> +  bool success = IsSuccessfulRequest(aRequest, &status);
> +
> +  if (status == NS_ERROR_BLOCKED_URI) {

I assume that this works and that we already know by then that the AsyncOpen call will be canceled?

If so, would it make sense to add an "else if (status == NS_ERROR_TRACKING_URI) { return NS_ERROR_FAILURE }" clause to address comment 1 too?

@@ +2346,4 @@
>      }
>    }
>  
> +  // Items resolved as Image/Document will not be checked for content blocking,

By "will not be checked" I assume you mean it won't prevent plugin instantiation right?

Because I would expect it to still be blocked at the network layer by the underlying Safe Browsing code.
Attachment #8763200 - Flags: review?(francois)
Flags: needinfo?(francois)
(Assignee)

Comment 13

a year ago
Created attachment 8768105 [details] [diff] [review]
Make plugin content blocking work on e10s (v3)

Addressed review comments.
Attachment #8763200 - Attachment is obsolete: true
Attachment #8768105 - Flags: review?(francois)
Comment on attachment 8768105 [details] [diff] [review]
Make plugin content blocking work on e10s (v3)

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

Looks fine to me, provided that we address two things in a follow-up bug(s):

1. Adding a network error page (for Desktop and Fennec) when users browse directly to a blocked URI.
2. Add a message to the devtool console about blocked resources (if the console logging you have doesn't already go to that console).

I don't want to delay your experiment, but I want to make sure we provide ample feedback when blocking things otherwise Firefox will just look broken to users/developers.

For the error page, you can find an example here:

  https://hg.mozilla.org/mozilla-central/rev/d7c0dbcf2a51
  https://hg.mozilla.org/mozilla-central/rev/40245af65036
Attachment #8768105 - Flags: review?(francois) → review+
Depends on: 1286391
Depends on: 1286392
Try build looks pretty green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=841f6b155844&selectedJob=23758488

Comment 16

a year ago
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a3f45f80219
Make plugin content blocking work on e10s. r=francois
https://hg.mozilla.org/mozilla-central/rev/3a3f45f80219
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment on attachment 8768105 [details] [diff] [review]
Make plugin content blocking work on e10s (v3)

Approval Request Comment
[Feature/regressing bug #]: Non-essential plugin blocking: reduces plugin crash rate by 5-10%
[User impact if declined]: more plugin crashes/hangs
[Describe test coverage new/current, TreeHerder]: tests already landed, were broken with e10s and this patch fixes
[Risks and why]: Pretty low risk: a little refactoring, and plugins are never 100% risk-free, but 
[String/UUID change made/needed]: None

This should land along with bug 1275591 which actually turns the feature on.
Attachment #8768105 - Flags: approval-mozilla-beta?
Attachment #8768105 - Flags: approval-mozilla-aurora?
Can we ship 48 with this bug?
I am a bit surprised to see such requests that late in the cycle...

(and bug 1275591 has been backout)
(In reply to Sylvestre Ledru [:sylvestre] from comment #19)
> Can we ship 48 with this bug?
> I am a bit surprised to see such requests that late in the cycle...
> 
> (and bug 1275591 has been backout)

Given the lateness in the 48 beta cycle and the backout of bug 1275591 from Nightly 50, I don't think we'll be able to get this into Beta 48. :(

btw, this feature does not block e10s release. This is a new feature and not an e10s regression.
Comment on attachment 8768105 [details] [diff] [review]
Make plugin content blocking work on e10s (v3)

OK, thanks. So, not taking it for 48.
Attachment #8768105 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8768105 [details] [diff] [review]
Make plugin content blocking work on e10s (v3)

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

This patch fixes plugin content blocking issue on e10s. Take it in 49 aurora.
Attachment #8768105 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox48: affected → wontfix

Comment 23

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/db4d7a074f53
status-firefox49: affected → fixed

Updated

9 months ago
Depends on: 1311368
You need to log in before you can comment on or make changes to this bug.