663541 - better transition/porting of modules that use "Components"

Status: VERIFIED FIXED

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

Description

[Brian Warner [:warner :bwarner]]

In speaking with Myk today, and watching Ben try to port some traditional
(XUL/XPCOM) Firefox addons to Jetpack, we concluded that the way we handle
the "Components" symbol could be changed, to make life easier for folks like
Ben doing porting work.

== The Current Situation ==

Traditional extensions assume that "Components", the main XPCOM access
object, is available ambiently. They usually start by defining some
convenience aliases like "Cc = Components.classes" (but sometimes assume that
the aliases are ambient too, which occasionally fails), and then invoke
Cc/Ci/etc to access low-level authority. Through "Components", this code can
take complete control of the browser, sort of like having root access in an
operating system. We call this sort of access "chrome" access.

According to our specifications, jetpack modules' only ambient symbols are
"require" and "exports" (well, also "define" and "module", and "console", but
that's it). All authority is obtained by invoking require(), in a way that
can be statically analyzed. The only way to get "chrome" access is with a
special require("chrome") call, through which the code can get Cc/Ci/etc (all
the common aliases) as well as a "components" object (note the lower-case)
that behaves just like the normal "Components" object. Low-level modules can
start their file with:

 const {Cc, Ci, Cu} = require("chrome");

and then do everything they would have done in a traditional XUL extension.

The current jetpack release has an outstanding problem (bug 636145): despite
our specification, "Components" is still available as an ambient symbol. This
is a clear authority leak: despite looking innocent (i.e. not declaring its
intentions to use chrome access with a require("chrome")), modules can just
use Components.classes (perhaps in some obsfuscated way) to do powerful
things. As soon as we figure out how, we'll remove this symbol (perhaps e10s
will help, by removing XPCOM from the process that jetpack code runs in).

To teach developers to use require("chrome"), and so we don't encourage bad
habits (i.e. using Components without require("chrome")), the manifest
scanner (run as part of "cfx xpi" or "cfx run") greps the code for
occurrences of "Components" and complains about them. The error message
instructs the developer to use require("chrome") instead.

The lowercase {components}=require("chrome") was included because there are
properties of Components that don't have common aliases. The exported symbol
is lowercase because, until bug 636145 is fixed, we can't override the
uppercase symbol. Furthermore, by making the existing (forbidden) symbol
distinct from the recommended replacement, the manifest scanner can correctly
complain about "Components" but tolerate "components".

== The Problem ==

Ben has a traditional extension which uses "Components.classes" extensively,
and he wants to port it to Jetpack. If his code merely used "Cc" extensively,
the job would be easy: insert one '{Cc}=require("chrome");' line at the
beginning. But because his code doesn't use the aliases, he's faced with the
annoying prospect of lowercasing each occurrence of "Components" in his code.

Because of bug 636145, his unmodified code will actually work for now, but
will stop working after the bug is fixed. At that point, he'll need a
require("chrome") call of some sort. Hopefully, removing "Components" from
the ambient namespace will also free up that name for assignment, so after
the 636145 is resolved, we'd like him to insert:

 {Components} = require("chrome");

at the beginning of his file, and then use "Components.classes" as he is now.

The trick is the transition. We don't want to teach developers bad habits now
by allowing use of ambient "Components" to go unpunished, but we don't want
to require excessive code rewriting when we know there's a simple one-line
fix coming soon.

The conclusion that Myk and I came to was that we should change the manifest
scanner now, to tolerate "Components" when there is a require("chrome") line
in the file. This will allow Ben to just add a require("chrome") line to his
file: the code will pass the scanner, and the runtime will work today. When
636145 is fixed, he'll get an error at runtime until he changes that line to
say {Components}=require("chrome"), after which his code will continue to
work.

The scanner doesn't look deeply at how the return value is used (that's too
hard), so it will be possible for a developer to get away with:

 {Cc,Ci} = require("chrome");
 Components.classes.blahblah();

which would pass the modified scanner without complaint, but will fail after
bug 636145 is fixed. Hopefully this won't teach too many bad habits.

== The Future ==

Once 636145 is fixed, and Components is removed, we must make it possible to
do {Components}=require("chrome"). Also, let's consider putting a dummy
Components object in the namespace which throws an exception upon any use (in
particular property access), reminding devs to replace it with the one you
get back from require("chrome"). This would give Ben a better error message
and a reminder to update his code as described above.

== The Tasks ==

So the tasks for this ticket are:

1: change the manifest scanner to tolerate use of "Components" if the file
   does require("chrome")

2: make the error message better: show the offending line and linenumber,
   also make a better effort to show an example of what they should use
   instead

3: add notes to bug 635557 to include in developer documentation, explaining
   how to transition from raw Components.classes to {Cc}=require("chrome")

Comment 1

[Brian Warner [:warner :bwarner]]

Attachment: allow Components after seeing require("chrome"), improve error message

This patch implements the first two tasks described above. The new and
improved error message, for a module which uses "Components.interfaces"
without a require("chrome") declaration, looks like this:

  (jp-git)15:warner% cfx xpi
  
  In file /Users/warner/stuff/mozilla/jetpack/my-addons/minimal/lib/main.js:
  To use chrome authority, you need a line like this:
    const {Ci} = require("chrome");
  because things like 'Components.classes' and 'Components.interfaces'
  will not be available. Then replace e.g. 'Components.classes' with 'Cc'
  in the following lines:
  
    7: Components.interfaces["foo"];
  
  Traceback (most recent call last):
    File "/Users/warner/stuff/mozilla/jetpack/jp-git/bin/cfx", line 29, in <module>
      cuddlefish.run()
    File "/Users/warner/stuff/mozilla/jetpack/jp-git/python-lib/cuddlefish/__init__.py", line 656, in run
      loader_modules)
    File "/Users/warner/stuff/mozilla/jetpack/jp-git/python-lib/cuddlefish/manifest.py", line 566, in build_manifest
      mxt.build(scan_tests)
    File "/Users/warner/stuff/mozilla/jetpack/jp-git/python-lib/cuddlefish/manifest.py", line 190, in build
      self.top_uri = self.process_module(self.find_top(self.target_cfg))
    File "/Users/warner/stuff/mozilla/jetpack/jp-git/python-lib/cuddlefish/manifest.py", line 343, in process_module
      raise BadChromeMarkerError()
  cuddlefish.manifest.BadChromeMarkerError
  (jp-git)16:warner% 

How does that look? The suggested {Ci} assignment is computed according to
what was found in the file, but the rest of those two sentences are static.
Think it's worth building up a more accurate string (i.e., don't mention
Components.classes when the file only used Components.interfaces)?

Comment 2

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

I would at least mention both the Cc and Ci cases specifically, since those two usually go together. (And since you already mentioned "interfaces" in the previous sentence.)

Comment 3

[Brian Warner [:warner :bwarner]]

Attachment: allow Components after seeing require("chrome"), improve error message

KWierso: good point. Here's an updated patch. The error message now looks
like:

  In file /Users/warner/stuff/mozilla/jetpack/my-addons/minimal/lib/main.js:
  To use chrome authority, you need a line like this:
    const {Ci} = require("chrome");
  because things like 'Components.classes' and 'Components.interfaces'
  will not be available. Then replace e.g. 'Components.classes' with 'Cc'
  and 'Components.interfaces' with 'Ci' in the following lines:
  
    7: Components.interfaces["foo"];

Comment 4

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

Comment on attachment 538621 [details] [diff] [review]
allow Components after seeing require("chrome"), improve error message

The new message still makes it seem like you have to get rid of "Components" in your code, even though that is no longer the case.

And it clearly prefers C* over Components, although Components itself isn't the problem (the problem is that it is a predefined global that we can't currently get rid of), and replacing all occurrences of it is unnecessary work.

I think it would be better to say something like this instead:

--------------------------------------------------------------------------------
In file <path>:
You access `Components` to get chrome authority.  To do so, you need a line like this:

  const { Cc, Ci } = require("chrome");

Then you can use `Components` as well as any shortcuts to its properties that you import from the `chrome` module (`Cc`, `Ci`, `Cm`, `Cr`, and `Cu` for the `classes`, `interfaces`, `manager`, `results`, and `utils` properties, respectively).

(Note: once bug <number> is fixed, to access `Components` directly you'll need to retrieve it from the `chrome` module by adding it to the list of symbols you import from the module.  To avoid having to make this change in the future, replace all occurrences of `Components` in your code with the equivalent shortcuts now.)
--------------------------------------------------------------------------------


Then, once we remove `Components` from the global namespace, the message will change to something like:

--------------------------------------------------------------------------------
In file <path>:
You access `Components` to get chrome authority.  To do so, you need a line like this:

  const { Cc, Ci, Components } = require("chrome");

Then you can use `Components` as well as any shortcuts to its properties that you import from the `chrome` module (`Cc`, `Ci`, `Cm`, `Cr`, and `Cu` for the `classes`, `interfaces`, `manager`, `results`, and `utils` properties, respectively).
--------------------------------------------------------------------------------

Comment 5

[Brian Warner [:warner :bwarner]]

Yeah, I'm ok with that wording. My concern has been with what lessons we're
teaching new developers through this error message. In every other corner of
the SDK, we teach a consistent lesson: the only way to get symbols into your
namespace is with require(). The reluctant admission we're making with this
error message is that there's an exception: there's this magic you can type
to enable an ambient "Components" symbol (and you could do it anywhere in the
file, even at the very end, outside the scope of where you're using
Components).

So my goal was to weasel this error message into a shape that acknowledges
that this ambient Components is possible, but that we'd really rather you use
{Components}=require("chrome"). Except that you can't, yet, so we need a
compromise, like your proposed form.

So yeah, I'm good with this wording. I'll update the patch now. I might
change the first sentence a little bit, to make it feel more like we're
talking about your *code* using Components rather than You (the human).

Comment 6

[Brian Warner [:warner :bwarner]]

Attachment: updated patch

new version of the patch, which incorporates Myk's suggestion, plus some
slightly different wording. New message is:

(jp-git)25:warner@host-7-243% cfx xpi

The following lines from file /Users/warner/stuff/mozilla/jetpack/my-addons/minimal/lib/main.js:
   7: Components.interfaces["foo"];
   8: Components.classes(blah);
use 'Components' to access chrome authority. To do so, you need to add a
line somewhat like the following:

  const {Cc,Ci} = require("chrome");

Then you can use 'Components' as well as any shortcuts to its properties
that you import from the 'chrome' module ('Cc', 'Ci', 'Cm', 'Cr', and
'Cu' for the 'classes', 'interfaces', 'manager', 'results', and 'utils'
properties, respectively).

(Note: once bug 636145 is fixed, to access 'Components' directly you'll
need to retrieve it from the 'chrome' module by adding it to the list of
symbols you import from the module. To avoid having to make this change
in the future, replace all occurrences of 'Components' in your code with
the equivalent shortcuts now.)

Traceback (most recent call last):
  File "/Users/warner/stuff/mozilla/jetpack/jp-git/bin/cfx", line 29, in <module>
    cuddlefish.run()
  File "/Users/warner/stuff/mozilla/jetpack/jp-git/python-lib/cuddlefish/__init__.py", line 656, in run
    loader_modules)
  File "/Users/warner/stuff/mozilla/jetpack/jp-git/python-lib/cuddlefish/manifest.py", line 566, in build_manifest
    mxt.build(scan_tests)
  File "/Users/warner/stuff/mozilla/jetpack/jp-git/python-lib/cuddlefish/manifest.py", line 190, in build
    self.top_uri = self.process_module(self.find_top(self.target_cfg))
  File "/Users/warner/stuff/mozilla/jetpack/jp-git/python-lib/cuddlefish/manifest.py", line 343, in process_module
    raise BadChromeMarkerError()
cuddlefish.manifest.BadChromeMarkerError
(jp-git)26:warner@host-7-243%

Comment 7

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

Comment on attachment 539072 [details] [diff] [review]
updated patch

Looks good, works well, r=myk.

I've been going back and forth over whether to land this for 1.0, and I think we should wait and land it as soon as the tree reopens. There isn't much risk here, but there's a bit, and we're at the very tail end of a cycle in which supporting this use case was very much not a priority, whereas it will increasingly become a priority in the next set of cycles, so that feels like the right place to land, patch up as needed, and build upon as appropriate.

Comment 8

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

The post-1.0 tree has been open for a while, has this been reprioritized?

Comment 9

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

This should land in the current cycle.

Brian: can you land it?

Comment 10

[Brian Warner [:warner :bwarner]]

Oops.. forgot about this one. Thanks for the reminder. Landed in:

https://github.com/mozilla/addon-sdk/commit/3fe3dbd67094e9758bbfc9a0bd02d554f91958f1

One quirk I noticed testing just now: if your addon uses some non-shortcut form, like:

  Components.foobazzle()

then the message tells you to import the lower-case form:

  {components} = require("chrome");

and then proceeds to talk about how "you can use 'Components' as well as any shortcuts..".

Which, while functional and nominally correct, is kind of weird-looking. We can't recommend people import upper-case Components now, because they can't. We do want to reveal that they can *use* upper-case Components, because they can (and that's Ben's use case), although for new code we'd prefer they use lower-case components (because that will keep working without needing changes after we fix bug 636145).

I'm tempted to leave it, and allow developers who run into this case to just wonder if we're being incautious with our capitalization. Any strong feelings?

Comment 11

[Brian Warner [:warner :bwarner]]

feel free to reopen if you want to change the wording somehow

Comment 12

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

(In reply to comment #10)
> I'm tempted to leave it, and allow developers who run into this case to just
> wonder if we're being incautious with our capitalization. Any strong
> feelings?

Works for me (modulo intense embarrassment at the thought of being thought to be the source of a typo).