Closed Bug 1268120 Opened 4 years ago Closed 3 years ago

Make plugin content blocking work on e10s

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
e10s - ---
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: tschneider, Assigned: tschneider)

References

(Depends on 2 open bugs)

Details

Attachments

(1 file, 2 obsolete files)

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
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.
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.
Make plugin content blocking work on e10s
Attachment #8761876 - Attachment description: bug1268120.diff → Make plugin content blocking work on e10s
Fixed ShouldBlockContent().
Attachment #8761876 - Attachment is obsolete: true
Duplicate of this bug: 1268716
No longer blocks: 1277876
Attachment #8763200 - Flags: review?(benjamin)
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)
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
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
Closed: 3 years ago
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+
Depends on: 1311368
You need to log in before you can comment on or make changes to this bug.