1283681 - Enable dom.ipc.cpows.forbid-cpows-in-compat-addons preference

Status: RESOLVED WONTFIX

(Firefox :: Extension Compatibility, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This needs to be based on telemetry.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

On Aurora, we have five add-ons:

{b9db16a4-6edc-47ec-a1f4-b86292ed211d}
privateTab@infocatcher
mousegesturessuite@lemon_juice.addons.mozilla.org
[e-mail]
[e-mail]

We'll need to set dom.ipc.cpows.forbid-cpows-in-compat-addons to true and dom.ipc.cpows.allow-cpows-in-compat-addons to this comma-separated list of add-on IDs. Then we'll need to reach out to these developers and ask them to stop using CPOWs. Once they have done so, we can remove them from the whitelist.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

This makes the change on our side.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Bill McCloskey (:billm) from comment #1)
> On Aurora, we have five add-ons:
> 
> {b9db16a4-6edc-47ec-a1f4-b86292ed211d}
> privateTab@infocatcher
> mousegesturessuite@lemon_juice.addons.mozilla.org
> [e-mail]
> [e-mail]
> 
> We'll need to set dom.ipc.cpows.forbid-cpows-in-compat-addons to true and
> dom.ipc.cpows.allow-cpows-in-compat-addons to this comma-separated list of
> add-on IDs. Then we'll need to reach out to these developers and ask them to
> stop using CPOWs. Once they have done so, we can remove them from the
> whitelist.

Shell, can you help make sure we reach out to these authors to get them off the whitelist?

Comment 4

[:shell escalante]

yes - but leaving the need info for myself.  Billm explained the issue to andy and me.  i will take up in email with add-ons team for the best plan forward and also just to share what is happening / what change needs to be communicated (with whitelist and with other authors or updated docs so others dont do it).

Comment 5

[lemon_juice]

I'm the author of Mouse Gestures Suite (mousegesturessuite@lemon_juice.addons.mozilla.org) and I'm aware of the CPOW usage. The problem is that refactoring of the code will be quite time consuming so I can't do it very quickly but I'll get to it some time. This add-on is based on a very old code base and some parts need to be moved to frame script - the most important one are already moved but not all. I remember how many hours I had to spend getting it to work with e10s at all...

On the plus side the add-on uses CPOW only when some of its own functions are used (mainly auto scrolling and grab & drag scrolling) so it shouldn't slow down the general use of Firefox.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Please just avoid marking it multiprocessCompatible in that case.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b10ecc57ae1a
Enable dom.ipc.cpows.forbid-cpows-in-compat-addons preference (r=blassey)

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b10ecc57ae1a

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8784111 [details] [diff] [review]
patch

I think we should backport this change to Aurora and beta. This patch makes it less like that add-ons will make mistakes when setting the multiprocessCompatibility flag. Add-on developers typically don't test with nightly, so having it on beta would be really nice. The risk is that we'll break some add-ons with e10s. However, telemetry data suggests that won't happen. Either way, we'll have an entire beta cycle to resolve the problem. Also, I believe we don't plan to enable e10s with add-ons until 51.

Approval Request Comment
[Feature/regressing bug #]: none
[User impact if declined]: worse add-on performance
[Describe test coverage new/current, TreeHerder]: on m-c
[Risks and why]: A small number of add-ons may break with e10s.
[String/UUID change made/needed]: None

Comment 10

[Fanolian]

Is there a meta bug that we can report extension breakages due to this setting?
Context Search X [1] is currently broken due to it. I have sent an email to the extension author regrading this issue.

[1]: https://addons.mozilla.org/firefox/addon/context-search-x/

Comment 11

[YUKI "Piro" Hiroshi]

I'm the author of Tree Style Tab addon and currently I cannot reproduce CPOW errors in my environment: Nightly 52.0a1 on Windows 7 and Ubuntu. (Of course e10s is activated and I removed TST's id from the whitelist "dom.ipc.cpows.allow-cpows-in-compat-addons".) Is there the source of the decision that TST is included in the whitelist due to existing CPOW usage? I hope to update TST to e10s compatible and TST disappear from the special whitelist.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Fanolian from comment #10)
> Is there a meta bug that we can report extension breakages due to this
> setting?
> Context Search X [1] is currently broken due to it. I have sent an email to
> the extension author regrading this issue.

Are you sure it's broken due to this change? That would mean that there's no one with telemetry enabled who uses the add-on.

(In reply to YUKI "Piro" Hiroshi from comment #11)
> I'm the author of Tree Style Tab addon and currently I cannot reproduce CPOW
> errors in my environment: Nightly 52.0a1 on Windows 7 and Ubuntu. (Of course
> e10s is activated and I removed TST's id from the whitelist
> "dom.ipc.cpows.allow-cpows-in-compat-addons".) Is there the source of the
> decision that TST is included in the whitelist due to existing CPOW usage? I
> hope to update TST to e10s compatible and TST disappear from the special
> whitelist.

The list was compiled using telemetry. Any add-on that uses CPOWs when multiprocessCompatible is set gets recorded in the telemetry ping. So unfortunately I don't know how to reproduce the problem. If you like, I could remove TST from the list and then you can see if users report problems.

If this becomes enough of a problem, we could try to gather more data about what went wrong. But that's a bit difficult.

Comment 13

[Fanolian]

Attachment: Context Search X errors with forbid-cpows-in-compat-addons set to true

(In reply to Bill McCloskey (:billm) from comment #12)
> (In reply to Fanolian from comment #10)
> > Is there a meta bug that we can report extension breakages due to this
> > setting?
> > Context Search X [1] is currently broken due to it. I have sent an email to
> > the extension author regrading this issue.
> 
> Are you sure it's broken due to this change? That would mean that there's no
> one with telemetry enabled who uses the add-on.

I am sure this change breaks Context Search X. This entry is the only setting I flip when I test it in a new profile. The warnings/errors given in Browser Console are different according to forbid-cpows-in-compat-addons value.

My testing:
1. Install CSX in a new profile in Nightly. Restart browser.
2. Right click on any highlighted text and observe the context menu.
3. Flip the value of dom.ipc.cpows.forbid-cpows-in-compat-addons. Restart browser.
4. Repeat step 2.

Result:
When forbid-cpows-in-compat-addons is true:
An empty entry in the context menu where CSX is supposed to be. An error appears in Browser Console.

When forbid-cpows-in-compat-addons is false:
"Search Google for …" is replaced by CSX's entries. Some warnings are shown in Browser Console but it still works.

Comment 14

[Fanolian]

Attachment: Context Search X warnings with forbid-cpows-in-compat-addons set to false

I have telemetry enabled in my profile with Context Search X installed. But I use only Nightly if it matters.

Comment 15

[Nils Maier [:nmaier]]

Enabling this is a nuisance for DownThemAll!
We need to check if we're over an html form (field) and display a context menu accordingly. We do this from a "popupshowing" event for the context area menu, i.e. we are more or less doing what nsContextMenu is doing anyway. Enabling this raises the "CPOW usage forbidden in this add-on" error.
Our code that is affected is essentially |if (gContextMenu.target && ('form' in gContextMenu.target))|


To work around this I see multiple avenues:

1) Cu.permitCPOWsInScope
This defeats the purpose of catching accidental CPOWs, tho

2) Even more lazily display the context menu items
Annoying UX when the context menu gets re-arranged after it is first displayed

3) Polling mouse-is-over-form all the time
Well, busy polling, nuff said

4) Provide another mechanism to explicitly access the CPOW without triggering the error

5) Make add-on CPOW use only forbidden if it is done in an unsafe context.
Please note that I tried Cu.permitCPOWsInScope and it works and does not trigger an "unsafe" warning either, because the DownThemAll! use in "popupshowing" is safe.

6) Somehow make nsContextMenu populate and provide the information I need.

7) Something I didn't think of yet

Personally, I'd like option 4) and 5), but will use 1) in the meantime
Bill, can you give some input on this?

Comment 16

[Nils Maier [:nmaier]]

I implemented Cu.permitCPOWInScope now for DownThemAll! as a temporary measure until something better comes along, if anybody is interested:
https://github.com/downthemall/downthemall/commit/820a28acc3e2ac9f0b1de438afcf51591f1ceef3

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8784111 [details] [diff] [review]
patch

Even though this could have an add-compatibility impact, it seems like the right thing to do, Aurora51+, Beta50+

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This applies on aurora, but is hitting a conflict on beta. Could we get a rebased patch?

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9348c0fd4bde

Comment 20

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Hi Nils,
There is a way to do what you want. We have a frame script that triggers the context menu here:
http://searchfox.org/mozilla-central/rev/767e1e9b118269f957ca1817138472146278a29e/browser/base/content/content.js#86
Before it asks the parent to show the menu, it notifies an observer:
http://searchfox.org/mozilla-central/rev/767e1e9b118269f957ca1817138472146278a29e/browser/base/content/content.js#110

So you can listen on that observer in a frame script and add any info you want to addonInfo (using your addon ID as the property key in the object). Then addonInfo will be sent to the parent and you can get it out of gContextMenuContentData.

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

OK, I realized why I wasn't seeing Context Search X. There's an option in our telemetry UI to "hide data points that don't have enough submissions" and that's checked by default. When I uncheck it, I see a lot more add-ons :-(.

We'll have to add more stuff to the whitelist.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f974a93b5358
Update list of CPOW-whitelisted add-ons based on new data

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3db11822d83f7a97cb882ec8338578f50b51a994
https://hg.mozilla.org/releases/mozilla-beta/rev/fde985997af66449563b52413832d77d02d475c7

I landed an updated whitelist and uplifted it. Shell, it looks like we'll need to do more outreach.

Comment 24

[Nils Maier [:nmaier]]

(In reply to Bill McCloskey (:billm) from comment #23)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/
> 3db11822d83f7a97cb882ec8338578f50b51a994
> https://hg.mozilla.org/releases/mozilla-beta/rev/
> fde985997af66449563b52413832d77d02d475c7
> 
> I landed an updated whitelist and uplifted it. Shell, it looks like we'll
> need to do more outreach.

Umm, you added both DownThemAll! ids to the list
{DDC359D1-844A-42a7-9AA1-88A850A938A8} - Our Release ID.
[e-mail] - Our DownThemAll! Nightly ID (which even is supposed to break in cases like this, so that we and our testers can get a chance to notice and fix it)

Please don't do that.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f974a93b5358

Comment 26

[Nils Maier [:nmaier]]

(In reply to Nils Maier [:nmaier] from comment #24)

Comment 27

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Nils Maier [:nmaier] from comment #24)
> Umm, you added both DownThemAll! ids to the list
> {DDC359D1-844A-42a7-9AA1-88A850A938A8} - Our Release ID.
> [e-mail] - Our DownThemAll! Nightly ID (which even is supposed to
> break in cases like this, so that we and our testers can get a chance to
> notice and fix it)
> 
> Please don't do that.

Which ones do you want on the list? Do you have a fix yet for the context menu issue?

(In reply to Nils Maier [:nmaier] from comment #16)
> I implemented Cu.permitCPOWInScope now for DownThemAll! as a temporary
> measure until something better comes along, if anybody is interested:

To anyone else reading this, please do not use this function. It just means we'll have to go through another round of deprecations like this in the future. If your add-on is broken by the patch in this bug, then your options are to remove the multiprocessCompatible flag or fix the CPOW use.

Comment 28

[Nils Maier [:nmaier]]

(In reply to Bill McCloskey (:billm) from comment #27)
> Which ones do you want on the list?

None

> Do you have a fix yet for the context
> menu issue?

yes

> (In reply to Nils Maier [:nmaier] from comment #16)
> > I implemented Cu.permitCPOWInScope now for DownThemAll! as a temporary
> > measure until something better comes along, if anybody is interested:
> 
> To anyone else reading this, please do not use this function. It just means
> we'll have to go through another round of deprecations like this in the
> future.

Forbidding safe CPOW in general in add-ons is in my humble opinion a really bad decision and will get people to use Cu.permitCPOWInScope. A way to use CPOW explicitly instead of not at all or force-enabling it for the entire scope would be the way to go, combined with outreach and AMO specific policy.

Comment 29

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The purpose of the multiprocessCompatible flag is to say that an add-on doesn't use CPOWs. You're free to use CPOWs if you want as long as you don't set multiprocessCompatible.

Comment 30

[Nils Maier [:nmaier]]

If I don't set multiprocessCompatible then I get shimmed and completely CPOW white-listed even for unsafe CPOW, so no, the multiprocessCompatible isn't about CPOW use. By the "Uses some CPOW" metric, Firefox itself is far from compatible.

Comment 31

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Nils Maier [:nmaier] from comment #30)
> If I don't set multiprocessCompatible then I get shimmed and completely CPOW
> white-listed even for unsafe CPOW, so no, the multiprocessCompatible isn't
> about CPOW use.

Well, all right, I guess that's fair. I'll fix up the whitelist soon. I still would really discourage people from using permitCPOWsInScope, but I guess it does serve a purpose.

> By the "Uses some CPOW" metric, Firefox itself is far from compatible.

I think you might be overstating things. Firefox has very few uses of CPOWs overall. Context menus, maybe some developer tools, and view source are the ones I know of, and we're working on fixing those. If you think you need a CPOW for something, please do file a bug and we can try to resolve the issue.

Comment 32

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This isn't really relevant anymore.