566028 - remove -service suffix from observer and preferences modules

Status: RESOLVED WONTFIX

(Add-on SDK Graveyard :: General, defect)

Description

[Dietrich Ayala (:dietrich)]

Attachment: v1

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: v2

Comment 2

[Dietrich Ayala (:dietrich)]

Attachment: v2

bah, forgot to qrefresh

Comment 3

[Atul Varma [:atul]]

So there were a few reasons I originally appended '-service' to the end of these module names. Most significantly, they were application-wide singletons, rather than merely jetpack-program singletons. In this sense, changing a preference through the preference service could actually affect something outside of your jetpack's sandbox.

For instance, 'observer-service' was so named instead of 'observer' because I didn't want it to be mis-interpreted as a generic observer singleton for one's jetpack program: if, for instance, one mistakenly assumed that and used it to dispatch events of type "foo" to other modules in the same jetpack program, they would actually be propagated application-wide and received by other instances of the module in the app. The same goes for preferences, but ultimately "service" is probably too generic a term to imply such a thing.

I'd like to preserve *some* sort of naming to indicate to folks that these aren't just generic observer/preference patterns, but are very specific to the mozilla platform we're building on. I guess this gets to the broader issue of naming our modules in such a way that it's evident which ones are "internal" and meant to be used only by low-level jetpack apis or XUL extensions, and which are low-level jetpack apis.

For now, what do you think of renaming them to e.g., "xpcom/observer" and "xpcom/preferences"?

Comment 4

[Dietrich Ayala (:dietrich)]

I think that adding "-service" to the module name is not sufficient to solve the problem of mistaken assumptions. I think it also adds complexity since we'll need to have mirror modules that do the same thing ("er, am i working with the global observer or the local observer?!").

If we want to create a local vs global distinction, can we hang it off the module like:

require("observers").add()
require("observers").addGlobal()

(or alternatively: require("observers").addLocal(), but i prefer to have global be the specialized one)

IMO this is much more explicit than expecting add-on authors to understand that "-service" means application-global, and doesn't require mirror modules.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #4)
> If we want to create a local vs global distinction, can we hang it off the
> module like:
> 
> require("observers").add()
> require("observers").addGlobal()
> 
> (or alternatively: require("observers").addLocal(), but i prefer to have global
> be the specialized one)

That's not a bad idea, except that there's a significant difference between the capabilities granted by access to application-wide preferences/observers and those granted by access to addon-specific equivalents, i.e. you can do a lot more damage if you have access to the application's observers/preferences than you can if you only have access to your own.

And since our proposed mechanisms for policing addon capabilities are at the module level (i.e. via analysis of `require` usage) rather than at the level of individual exported symbols (which I suspect Brian would like to achieve at some point, although not in the near-term), it suggests that this mismatch in capabilities warrants putting the APIs into two separate modules.

Nevertheless, I agree with Dietrich that the "-service" suffix doesn't communicate what we want to communicate, while the "xpcom/" prefix references an implementation detail about how we access those services.

My preference would be to call these app-observers and app-preferences, which are explicitly suggestive of the application-wide applicability of these modules while also being simple, until we come up with that better scheme for differentiating between low-level, potentially application-wide APIs and high-level ones.

cc:ing Brian for confirmation that my thinking wrt. the security implications is correct.

Comment 6

[Drew Willcoxon :adw]

FWIW I agree with everything Myk said.  (I should get a stamp that says "I probably agree with Myk.")  I'll add that a "service" suffix isn't particular helpful, because all these modules are really services in the sense that they're singletons exposing some interface.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #6)
> I should get a stamp that says "I probably agree with Myk."

That might confuse people following my "I probably agree with Drew" stamp!

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 445449 [details] [diff] [review]
v2

Unsetting review flag until we decide what we're going to do.

Comment 9

[Brian Warner [:warner :bwarner]]

Atul: yeah, I agree. If we were to have a single require("preferences") which
exported two objects (.global and .local) with drastically different
authority levels, then a user or reviewer could not make a comfortable
decision about whether to allow it or not:

 ADDON: I want access to require("preferences") because I want to save
        some of my persistent state.
 REVIEWER: Eek! That lets you change all of the user's preferences!
 ADDON: Oh, it's ok, I promise to only ever use prefs.local and never
        touch prefs.global .
 REVIEWER: I don't know...
 ADDON: Anyways, you didn't give me an API to just store local prefs, so
        I'm forced to use this more powerful API.
 REVIEWER: oh, ok, fine, you can have require("preferences")
 [TIME PASSES..]
 [USER INSTALLS ADDON]
 [ADDON GETS CORRUPTED BY BAD INPUT]
 [CORRUPTED-ADDON uses prefs.global]
 CORRUPTED-ADDON: Muahaha! Now I will change all the user's font
                  preferences to 4-point MinchoGothicBold and make
                  them go blind!

Comment 10

[Dietrich Ayala (:dietrich)]

not a priority, so unassigning for now.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Now that these are low-level modules, their names don't matter enough to justify this.