645458 - Check for chrome authority should be comment-aware

Status: RESOLVED WONTFIX

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

Description

[Bobby Holley (:bholley)]

The add-on sdk seems to do a simple grep for use of chrome authority, in order to generate an error + abort like the following:

>To use chrome authority, as in:
>...code here...
>You must enable it with something like:
>  const {Ci} = require("chrome");

This is incorrect, since it doesn't account for code that is commented out.

This problem is exacerbated by the fact that the add-on builder hangs mysteriously when the aforementioned chrome authority error is raised. This means that pasting certain strings of commented-out code into a working add-on can cause it to start hanging inexplicably. It would be nice of the add-on builder was better about passing along errors too.

Comment 1

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

Brian: Irakli thought we fixed this.  Do you know the current status?

Daniel: is there a bug report tracking the second problem reported here, that Add-on Builder "hangs mysteriously when the aforementioned chrome authority error is raised"?  We should make sure to address that in the Add-on Builder code.

Comment 2

[Bobby Holley (:bholley)]

(In reply to comment #1)
> Brian: Irakli thought we fixed this.  Do you know the current status?

This may be true. I realize that I tested this under 1.0b4 rather than the latest from github.

Comment 3

[Brian Warner [:warner :bwarner]]

Please take a look again with current SDK trunk (which should be in an 1.0rc1
by some time next week). I rewrote the are-you-using-Components.* code a few
days ago and removed a number of the checks, so you may see different
behavior. (if you're using the online Builder, you may need to wait until it
gets updated to the new SDK code.. not sure what the schedule for that is)

The code is now looking for just two things:

 * if you're using Components.classes or friends, it will tell you to use
   require("chrome") instead
 * if you're using require("chrome"), it makes a note in the manifest, for
   use by the runtime and by AMO reviewers

The scanner knows about a few comment conventions (both single-line and
multi-line), so if the line starts with one of the following, it will be
ignored:

 //
 /*
  *
  '
  "
  dump(

I haven't seen any of the "hangs mysteriously" problems. If they're still
around, could you attach a code sample and a transcript/screenshot of the
output?

Comment 4

[Bobby Holley (:bholley)]

I just pulled the latest from the git repository (ad0498464ab774) and still see the same issue. The scanner is comment aware on a per-line basis, but this isn't enough.

It correctly ignores things of the form:

/* Components.classes.foo; */

But it throws an incorrect error for:

   /*
    Components.classes.foo;
   */

I stopped using the addon builder a while ago, but remember that the issue was hitting 'test' when a string such as the above was in the code. The spinner would go indefinitely, and I eventually had to download the command-line SDK to figure out what was going on.

Comment 5

[Brian Warner [:warner :bwarner]]

Hm, yeah, the logic necessary to catch multi-line comments without a
characteristic prefix is going to be too hard to put into a regexp-based
scanner. I suspect that a serious Computer Scientist would tell me that this
is because JS (and C/C++) is not a Regular Language, and cannot be parsed by
Regular Expressions (see
http://stackoverflow.com/questions/1732348/regex-match-open-tags-except-xhtml-self-contained-tags
for the sad tale of someone who tried). In particular, the scanner would need
to correctly handle not-actually-comments like:

 const s = "Calculator buttons like +-/* confuse parsers";
 const Cc = Components.classes;
 const t = "and punctuation like +-*/ eats code";

The cheap regexp-based scanner comes at the cost of false positives. I'm
inclined to tolerate this for now, meaning that experienced XUL developers
who are porting old-style addon code to chrome (and who have not yet replaced
the Components.classes lines with require("chrome")) will get errors unless
they follow an artificial (but common) commenting convention.

When we fix the SDK to remove 'Components' from the module environment
altogether, we can probably remove the error-raising regexp scanner. Our goal
here is to teach developers the correct-for-the-future way to build these
modules, which means require(chrome), and until Components.classes gets a
real runtime error, the regexp scanner and it's artificial link-time error is
our best teaching tool

(we'll continue to have the require()-scanner, but it won't raise any errors,
and the results will just go into the manifest for review purposes)

Comment 6

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

(automatic reprioritization of 1.0 bugs)

Comment 7

[Brian Warner [:warner :bwarner]]

Is this still causing problems for you? There's a limit to how clever we can make the scanner without pulling a full JS parser into the SDK, so we might have to close this as a WONTFIX, but I'd like to hear about how much trouble it's causing our users first. (note that if/when we port the SDK to JS and run it as an addon itself, this may get easier, since in that environment we'll have the full JS parser available to us).

Comment 8

[Bobby Holley (:bholley)]

(In reply to comment #7)
> Is this still causing problems for you?

I'm not actively working on my jetpack anymore, so I can't really answer that question. :(

Comment 9

[Brian Warner [:warner :bwarner]]

Ok, fair enough :). I'm going to WONTFIX this one. Eventually we'll have better (JS-based) parsing tools that can do this accurately, but the effort-to-reward ratio is too high for our current implementation. If anyone else reading this bug feels pain from this, feel free to reopen it and we can try to figure something out.