1235627 - test issues for system addons

Status: RESOLVED INACTIVE

(Firefox :: Pocket, defect)

Description

[Shane Caraveo (:mixedpuppy)]

In bug 1215694 a pref will allow the test system to disable the pocket addon in order to get by numerous test failures.  We should reenable the addon during tests after resolving those issues.  One area of issues is resetting CUI and checks against defaults.  Other issues are tests that check against context menus, etc.  Similar issues exist with loop in bug 1232207.

Comment 1

[:Gijs (he/him)]

Feels to me as if we should make an entrypoint in CUI that allows external (that is, not part of CustomizableUI itself) consumers to add a widget that, although "external", gets special treatment as part of a system add-on.

Jared/Mike (*2): does that make sense? Is there a simpler solution that I'm missing?

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Are we able to see who signed the system add-on? If we could do that we could check that the system add-on was signed by Mozilla and then allow it to act "built-in".

Comment 3

[Shane Caraveo (:mixedpuppy)]

(In reply to (Away 12/29-1/3) Jared Wein [:jaws] (please needinfo? me) from comment #2)
> Are we able to see who signed the system add-on? If we could do that we
> could check that the system add-on was signed by Mozilla and then allow it
> to act "built-in".

AFAIK system add-ons don't require signing.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

That seems alarming, but maybe I'm missing something? Dave, is there a reason why we wouldn't want to sign system add-ons?

Comment 5

[Dave Townsend [:mossop]]

(In reply to (Away 12/29-1/3) Jared Wein [:jaws] (please needinfo? me) from comment #4)
> That seems alarming, but maybe I'm missing something? Dave, is there a
> reason why we wouldn't want to sign system add-ons?

System add-ons that ship with the application don't need to be signed (above and beyond any signing we already do for the application installer/updates). Updated versions of system add-ons that are delivered out of band of the Firefox release do have to be signed.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

If we signed the ones that came with the installer we could depend on that signature to increase capabilities of these add-ons. This would be a safe a hopefully simple way to elevate privileges for add-ons that are trusted.

Gijs, how does this sound to you from a CUI standpoint?

Comment 7

[Dave Townsend [:mossop]]

(In reply to (Away 12/29-1/3) Jared Wein [:jaws] (please needinfo? me) from comment #6)
> If we signed the ones that came with the installer we could depend on that
> signature to increase capabilities of these add-ons. This would be a safe a
> hopefully simple way to elevate privileges for add-ons that are trusted.

We don't have a way to do this right now that doesn't involve landing signed blobs in the tree for every change and so making development of these extensions quite cumbersome.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Exposing such an API to built-in add-ons (only) makes sense to me.

Comment 9

[Shane Caraveo (:mixedpuppy)]

(In reply to (Away 12/29-1/3) Jared Wein [:jaws] (please needinfo? me) from comment #6)
> If we signed the ones that came with the installer we could depend on that
> signature to increase capabilities of these add-ons. This would be a safe a
> hopefully simple way to elevate privileges for add-ons that are trusted.
> 
> Gijs, how does this sound to you from a CUI standpoint?

Requiring signing may be over thinking the problem.  We already hide any UI in about:addons for system addons, so there is some flag that is used to recognize them.  We should just use the same flag in any API used for this.  The primary item is to add button id's to the various lists in CUI, and have reset leave those in place.  If an addon is able to bypass that somehow, it will also be able to bypass the code which hides system addons (IMO a bigger problem if possible).

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

It's fine by me to use the same mechanism about:addons uses to hide system add-ons.

Comment 11

[Dave Townsend [:mossop]]

(In reply to (Away 12/29-1/3) Jared Wein [:jaws] (please needinfo? me) from comment #10)
> It's fine by me to use the same mechanism about:addons uses to hide system
> add-ons.

Right now that is a hidden flag but we're probably adding something more specific in bug 1232222.

Comment 12

[:Gijs (he/him)]

(In reply to (Away 12/29-1/3) Jared Wein [:jaws] (please needinfo? me) from comment #6)
> If we signed the ones that came with the installer we could depend on that
> signature to increase capabilities of these add-ons. This would be a safe a
> hopefully simple way to elevate privileges for add-ons that are trusted.
> 
> Gijs, how does this sound to you from a CUI standpoint?

Yeah, relying on them being feature/system addons would work. The only issue I see is that all the add-on manager APIs are async, and the createWidget API is sync. Not sure how we can make that work...

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

[Tracking Requested - why for this release]:
Comment #0 mentions numerous test failures around customizing the UI of Firefox. The Pocket add-on is disabled during testing to help the tests pass, but we ship with the feature enabled so we actually aren't testing what we are shipping. Without this bug fixed, we don't have an accurate understanding of when things break or a way to prevent them from breaking further.

Shane, is there any update on this? It seems we know things are broken (comment #0 says that there are many test failures) and we aren't testing what we are actually shipping.

Comment 14

[Shane Caraveo (:mixedpuppy)]

Any test that requires some bit of information in the base firefox code for tests to complete successfully will fail.  The major culprits are CUI and context menu's.  

Context menu tests expect a specific order which is changed by the inclusion of an addon that modifies context menus.

CUI relies on a built-in list of default button id's for a few things, any test that resets CUI, which results in the pocket button being removed (bug 1245074).  I added an onWidgetReset handler to deal with that (not sure why it is no longer working), but the the CUI default set checks in the tests fail.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Why couldn't we just update CustomizableUI.jsm or CustomizableWidgets.jsm to know about the pocket add-on? We have direct access to that code.

> I added an onWidgetReset handler to deal with that (not sure why it is no longer working),

Anything that is lacking a test is asking to get broken. It is essentially broken-by-default.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking, discussed a bit with Shane. It sounds like similar issues will affect tests for other system addons so we may want to consider that and put more resources into keeping Firefox test coverage strong (and as jaws says, keep us testing what we ship so that we don't ship regressions we could have otherwise caught).   

Should this issue block releasing system addons ? What would an acceptable workaround be?

Comment 17

[:Gijs (he/him)]

(In reply to Dave Townsend [:mossop] from comment #11)
> (In reply to (Away 12/29-1/3) Jared Wein [:jaws] (please needinfo? me) from
> comment #10)
> > It's fine by me to use the same mechanism about:addons uses to hide system
> > add-ons.
> 
> Right now that is a hidden flag but we're probably adding something more
> specific in bug 1232222.

How would we know a JS caller is "from" an add-on, though? As in, how do we make the link from a call to whatever API we use to their add-on (and how do we make sure that can't be "faked") ?

I'm almost tempted to say we should make widgets used by an add-on in this way declarative in install.rdf or chrome.manifest or something. Is there an easier/better solution, Dave?

Comment 18

[Dave Townsend [:mossop]]

(In reply to :Gijs Kruitbosch from comment #17)
> (In reply to Dave Townsend [:mossop] from comment #11)
> > (In reply to (Away 12/29-1/3) Jared Wein [:jaws] (please needinfo? me) from
> > comment #10)
> > > It's fine by me to use the same mechanism about:addons uses to hide system
> > > add-ons.
> > 
> > Right now that is a hidden flag but we're probably adding something more
> > specific in bug 1232222.
> 
> How would we know a JS caller is "from" an add-on, though? As in, how do we
> make the link from a call to whatever API we use to their add-on (and how do
> we make sure that can't be "faked") ?
> 
> I'm almost tempted to say we should make widgets used by an add-on in this
> way declarative in install.rdf or chrome.manifest or something. Is there an
> easier/better solution, Dave?

Oh right. Yeah this is kinda a pain. We don't really have a good way for this. In bug 1231172 I proposed an idea for a way to give add-ons a private(ish) secret that could be used to identify the add-on to other APIs but it hasn't been implemented yet. We don't really have anything sane right now. We could probably implement the API piece of bug 1231172 quite quickly, though it's only going to work for restartless add-ons.

Comment 19

[Shane Caraveo (:mixedpuppy)]

What if there were some flag in the addon manager that is set during the call into bootstrap.js startup?  CUI could ask the addonmanager if it is inside a system addon startup.  In that case, CUI could use some additional flag on the widget object to know it should be a default.

Comment 20

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #19)
> What if there were some flag in the addon manager that is set during the
> call into bootstrap.js startup?  CUI could ask the addonmanager if it is
> inside a system addon startup.  In that case, CUI could use some additional
> flag on the widget object to know it should be a default.

That would only work if the calls were made synchronously, which likely won't be realistic for all cases for which we want to use this.

Comment 21

[Dave Townsend [:mossop]]

There are hacky options, like grabbing the stack trace and walking until you find a script from a URL that maps to an add-on. It's probably straightforward to spoof that though.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

OK so as this stands, we aren't testing this aspect of system addons, and aren't sure how to come up with a solution but we are still shipping system addons in 45. 

Sylvestre do you have any thoughts on this? Is this basically talking about the problems we're looking at in bug 1245277?

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Assuming you will wontfix this for 45. I probably should for 46 as well. But maybe we can figure something out for 47 onwards.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Not running tests always make me sad ... Thanks for the ni Liz!

Comment 25

[Shane Caraveo (:mixedpuppy)]

(per comment 15) We could alternately insert some pocket code back into core firefox to enable tests.  Note that there were NO pocket tests, but the benefit of having an addon enabled during tests would ensure no other breakage.  Given a longer term solution on fixing tests when system addons are involved, I'd be happy with this as a short term solution.  It may not be easy to do, I did try a couple basic things I thought would work but it's not that straightforward.

Comment 26

[:Gijs (he/him)]

(In reply to Dave Townsend [:mossop] from comment #18)
> In bug 1231172 I proposed an idea for a way to give add-ons a
> private(ish) secret that could be used to identify the add-on to other APIs
> but it hasn't been implemented yet. We don't really have anything sane right
> now. We could probably implement the API piece of bug 1231172 quite quickly,
> though it's only going to work for restartless add-ons.

This has been implemented in bug 1249689.

Is there a way to verify with that symbol that an add-on is a system add-on in a synchronous fashion? Last I checked all the AddonsManager APIs were async. :-(

Comment 27

[Dave Townsend [:mossop]]

(In reply to :Gijs Kruitbosch from comment #26)
> (In reply to Dave Townsend [:mossop] from comment #18)
> > In bug 1231172 I proposed an idea for a way to give add-ons a
> > private(ish) secret that could be used to identify the add-on to other APIs
> > but it hasn't been implemented yet. We don't really have anything sane right
> > now. We could probably implement the API piece of bug 1231172 quite quickly,
> > though it's only going to work for restartless add-ons.
> 
> This has been implemented in bug 1249689.
> 
> Is there a way to verify with that symbol that an add-on is a system add-on
> in a synchronous fashion? Last I checked all the AddonsManager APIs were
> async. :-(

Unfortunately not, we don't have that info without loading it from the database and we don't want to do it synchronously :(

Comment 28

[:Gijs (he/him)]

(In reply to Dave Townsend [:mossop] from comment #27)
> (In reply to :Gijs Kruitbosch from comment #26)
> > (In reply to Dave Townsend [:mossop] from comment #18)
> > > In bug 1231172 I proposed an idea for a way to give add-ons a
> > > private(ish) secret that could be used to identify the add-on to other APIs
> > > but it hasn't been implemented yet. We don't really have anything sane right
> > > now. We could probably implement the API piece of bug 1231172 quite quickly,
> > > though it's only going to work for restartless add-ons.
> > 
> > This has been implemented in bug 1249689.
> > 
> > Is there a way to verify with that symbol that an add-on is a system add-on
> > in a synchronous fashion? Last I checked all the AddonsManager APIs were
> > async. :-(
> 
> Unfortunately not, we don't have that info without loading it from the
> database and we don't want to do it synchronously :(

Wait, this is confusing. At the point where we send the Symbol() to the bootstrap method, we have both the add-on ID and the symbol to hand, right? Why can't we cache that in-process?

Comment 29

[Dave Townsend [:mossop]]

(In reply to :Gijs Kruitbosch from comment #28)
> (In reply to Dave Townsend [:mossop] from comment #27)
> > (In reply to :Gijs Kruitbosch from comment #26)
> > > (In reply to Dave Townsend [:mossop] from comment #18)
> > > > In bug 1231172 I proposed an idea for a way to give add-ons a
> > > > private(ish) secret that could be used to identify the add-on to other APIs
> > > > but it hasn't been implemented yet. We don't really have anything sane right
> > > > now. We could probably implement the API piece of bug 1231172 quite quickly,
> > > > though it's only going to work for restartless add-ons.
> > > 
> > > This has been implemented in bug 1249689.
> > > 
> > > Is there a way to verify with that symbol that an add-on is a system add-on
> > > in a synchronous fashion? Last I checked all the AddonsManager APIs were
> > > async. :-(
> > 
> > Unfortunately not, we don't have that info without loading it from the
> > database and we don't want to do it synchronously :(
> 
> Wait, this is confusing. At the point where we send the Symbol() to the
> bootstrap method, we have both the add-on ID and the symbol to hand, right?
> Why can't we cache that in-process?

It's complicated because there can be multiple add-ons installed with the same ID. But now you mention it I think there is a way we could do it synchronously without forcing the database to load, but as a rule I want to keep the APIs async wherever possible as it protects us against future changes. Why does this need to be synchronous?

Comment 30

[:Gijs (he/him)]

(In reply to Dave Townsend [:mossop] from comment #29)
> (In reply to :Gijs Kruitbosch from comment #28)
> > (In reply to Dave Townsend [:mossop] from comment #27)
> > > (In reply to :Gijs Kruitbosch from comment #26)
> > > > Is there a way to verify with that symbol that an add-on is a system add-on
> > > > in a synchronous fashion? Last I checked all the AddonsManager APIs were
> > > > async. :-(
> > > 
> > > Unfortunately not, we don't have that info without loading it from the
> > > database and we don't want to do it synchronously :(
> > 
> > Wait, this is confusing. At the point where we send the Symbol() to the
> > bootstrap method, we have both the add-on ID and the symbol to hand, right?
> > Why can't we cache that in-process?
> 
> It's complicated because there can be multiple add-ons installed with the
> same ID.

Mind blown. How? What happens in this case? :S

> But now you mention it I think there is a way we could do it
> synchronously without forcing the database to load, but as a rule I want to
> keep the APIs async wherever possible as it protects us against future
> changes. Why does this need to be synchronous?

Because some add-on will go and call createWidget() with a bunch of params, and that is currently synchronous. We'd break the API contract if we made that call asynchronous. Because a lot of add-ons will basically create the widget, and then move it somewhere once that method returns, making the creation async would break stuff.

In theory we could make it async only when passing the "I'm a system add-on" params, but that would be pretty hacky and not a very nice API because whether it was or wasn't sync would change / be unpredictable for novice addon authors...

I guess the other possibility from CUI's perspective would be to add its own async API that such system add-ons would need to call first, and it could then cache the symbol info on its own. But that feels like we'd be doing the add-on manager's job for it, and maybe it could get out of sync... doesn't sound nice. :-(

Comment 31

[Shane Caraveo (:mixedpuppy)]

Since this is only necessary for system addons, personally I'd be fine having to do something like:

CUI.systemWrap(addonid, key, (token) => {
 CUI.CreateWidget({ token: token });
});

Comment 32

[Dave Townsend [:mossop]]

If all you care about is the "is a system add-on" property then let's add a synchronous check for that. I don't have much time available but I could guide someone on doing that.

Comment 33

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

Hi Shane, is this something that is actionable? I don't feel the need to track it for a future release.

Comment 34

[Shane Caraveo (:mixedpuppy)]

I don't think it needs to be tracked for a version, but we should still address how to test addons w/CUI widgets, probably also need to consider WebExtensions at this point too.

Comment 35

[Jessilyn Davis]

This bug has been closed due to inactivity and/or the potential for this bug to no longer be an issue with the new Discovery Stream-powered New Tab experience, and improvements to Pocket’s integration with Firefox.

Please help us triage by reopening if this issue still persists and should be addressed.

Thanks!