1268120 - Make plugin content blocking work on e10s

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Tobias Schneider [:tobytailor]]

Our current plugin content blocking uses a e10s incompatible solution. This should be fixed before we finally decide to ship this feature.

Comment 1

[François Marier [:francois]]

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

Comment 2

[Benjamin Smedberg]

Toby does this affect tracking protection in private browsing or just the new plugin blocking behavior?

Comment 3

[Jim Mathies [:jimm]]

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.

Comment 4

[Tobias Schneider [:tobytailor]]

Benjamin: Yes, this should also avoid initializing the Flash Player if content gets Blocked by tracking Protection.

Comment 5

[Benjamin Smedberg]

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.

Comment 6

[Tobias Schneider [:tobytailor]]

Attachment: Make plugin content blocking work on e10s

Make plugin content blocking work on e10s

Comment 7

[Tobias Schneider [:tobytailor]]

Attachment: Make plugin content blocking work on e10s (v2)

Fixed ShouldBlockContent().

Comment 8

[Tobias Schneider [:tobytailor]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=afb5acf4f2d0&selectedJob=22529192

Comment 10

[Chris Peterson [:cpeterson]]

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.

Comment 11

[:Felipe Gomes (needinfo for replies!)]

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.

Comment 12

[François Marier [: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.

Comment 13

[Tobias Schneider [:tobytailor]]

Attachment: Make plugin content blocking work on e10s (v3)

Addressed review comments.

Comment 14

[François Marier [: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

Comment 15

[Chris Peterson [:cpeterson]]

Try build looks pretty green:

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a3f45f80219
Make plugin content blocking work on e10s. r=francois

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3a3f45f80219

Comment 18

[Benjamin Smedberg]

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.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

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)

Comment 20

[Chris Peterson [:cpeterson]]

(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 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8768105 [details] [diff] [review]
Make plugin content blocking work on e10s (v3)

OK, thanks. So, not taking it for 48.

Comment 22

[Gerry Chang [:gchang]]

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.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/db4d7a074f53