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

RESOLVED WONTFIX

Status

()

defect
RESOLVED WONTFIX
3 years ago
2 years ago

People

(Reporter: billm, Assigned: billm, NeedInfo)

Tracking

unspecified
Firefox 52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed, firefox51 fixed, firefox52 fixed)

Details

Attachments

(3 attachments)

This needs to be based on telemetry.
Flags: needinfo?(wmccloskey)
On Aurora, we have five add-ons:

{b9db16a4-6edc-47ec-a1f4-b86292ed211d}
privateTab@infocatcher
mousegesturessuite@lemon_juice.addons.mozilla.org
firegestures@xuldev.org
treestyletab@piro.sakura.ne.jp

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.
Posted patch patchSplinter Review
This makes the change on our side.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Flags: needinfo?(wmccloskey)
Attachment #8784111 - Flags: review?(blassey.bugs)
Attachment #8784111 - Flags: review?(blassey.bugs) → review+
(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
> firegestures@xuldev.org
> treestyletab@piro.sakura.ne.jp
> 
> 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?
Flags: needinfo?(sescalante)
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).
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.
Please just avoid marking it multiprocessCompatible in that case.
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b10ecc57ae1a
Enable dom.ipc.cpows.forbid-cpows-in-compat-addons preference (r=blassey)
https://hg.mozilla.org/mozilla-central/rev/b10ecc57ae1a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
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
Attachment #8784111 - Flags: approval-mozilla-beta?
Attachment #8784111 - Flags: approval-mozilla-aurora?
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/
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.
(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.
(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.
I have telemetry enabled in my profile with Context Search X installed. But I use only Nightly if it matters.
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?
Flags: needinfo?(wmccloskey)
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 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+
Attachment #8784111 - Flags: approval-mozilla-beta?
Attachment #8784111 - Flags: approval-mozilla-beta+
Attachment #8784111 - Flags: approval-mozilla-aurora?
Attachment #8784111 - Flags: approval-mozilla-aurora+
This applies on aurora, but is hitting a conflict on beta. Could we get a rebased patch?
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.
Flags: needinfo?(wmccloskey)
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.
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f974a93b5358
Update list of CPOW-whitelisted add-ons based on new data
(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.
dta@downthemall.net - 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.
Status: RESOLVED → REOPENED
Flags: needinfo?(wmccloskey)
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/f974a93b5358
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
(In reply to Nils Maier [:nmaier] from comment #24)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
> dta@downthemall.net - 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.
Flags: needinfo?(wmccloskey)
(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.
Flags: needinfo?(wmccloskey)
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.
Flags: needinfo?(wmccloskey)
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.
(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.
Flags: needinfo?(wmccloskey)
Depends on: 1315094
This isn't really relevant anymore.
Status: REOPENED → RESOLVED
Closed: 3 years ago2 years ago
Flags: needinfo?(wmccloskey)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.