679554 - require() statements fail when preceded by a leading quotation mark

Status: RESOLVED FIXED

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

Description

[Daniel Buchner [:dbuc]]

When require() statements are preceded by a quotation mark, in particular like this:

 var obj = {
   "propname": require("tabs").activeTab.url
 }

the require module look-up fails with the following error:

Traceback (most recent call last):
  File "resource://jid0-j1mkjvghbj0xhdp4g5x3bsqaq4m-at-jetpack-api-utils-lib/securable-module.js", line 396, in asyncRequire
    return syncRequire(deps);
  File "resource://jid0-j1mkjvghbj0xhdp4g5x3bsqaq4m-at-jetpack-api-utils-lib/securable-module.js", line 245, in syncRequire
    throw new Error("Module at "+basePath+" not allowed to require"+"("+module+")");
Error: Module at resource://jid0-j1mkjvghbj0xhdp4g5x3bsqaq4m-at-jetpack-translator-test-lib/main.js not allowed to require(tabs)

Comment 1

[Brian Warner [:warner :bwarner]]

Root cause is that the manifest scanner includes " and ' as comment markers, so it misfires and thinks that the whole "propname" is a comment, and ignores the require() call. If there are no other less-commenty require("tabs") lines, the manifest won't contain a marker for "tabs", and the runtime call will fail.

I'm struggling to remember why we included quotes as comment markers. I think it might have been to allow library modules to print messages at the user with instructions on what other modules they need to require, without actually causing those modules to depend upon the modules they recommended.

We could try to improve the manifest scanner to look for matching quotes, but that's a losing battle (regexps vs JS is just as bad as regexps vs HTML, and the <center> cannot hold). I think the easiest thing is to remove quotes from the list of comment markers, which means the scanner will spot require() in quoted strings. I'm running tests now to see if this causes any immediate explosions. I can whip up a tool to look for quoted-requires in the SDK and see if we're still doing anything like that. Even if we aren't, we should still consider how this change might affect user code.

Comment 2

[Brian Warner [:warner :bwarner]]

Attachment: include require() in quote-prefixed lines

Here's the patch, and it doesn't appear to break existing JS tests. I'm not yet sure this is the right fix: I still need to look for instances of quoted require()s in the SDK library code, and we all need to think about how this could affect user code.

Comment 3

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

Brian: is this a regression from 1.0, or has it existed since before that release?

Comment 4

[Brian Warner [:warner :bwarner]]

It's been around forever, at least since 0.6, which used the same
comment-prefixes as we've got now.

Comment 5

[smionean]

I have just get 1.1rc1 and my add-on won't work because of this bug.  I want to point out, this bug wasn't in 1.0.

Erreur : An exception occurred.
Traceback (most recent call last):
  File "resource://api-utils-lib/securable-module.js", line 408, in asyncRequire
    return syncRequire(deps);
  File "resource://api-utils-lib/securable-module.js", line 247, in syncRequire
    throw new Error("Module at "+basePath+" not allowed to require"+"("+module+")");
Error: Module at resource://api-utils-lib/self-maker.js not allowed to require(./file)

Comment 6

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

Bringing back the triage flag because comment 4 and comment 5 are in conflict.

Comment 7

[Daniel Buchner [:dbuc]]

I really question the assertion that this bug has been in existence for any significant period of time. I have test add-ons on the Add-on Builder that I can guarantee worked as late as 0.8  and 0.9. This is clearly a regression, why are we not fixing it now?

It's also really strange error to debug because you are looking for something that *isn't* broken or malformed: using correct JSON syntax is the bug. That is going to make this pretty painful for anyone who experiences it. Furthermore, what if a developers tries to use a stringified JS keyword as their JSON key? { "class": {} } for instance, you'd be damned if you do and damned if you don't on that one...

Comment 8

[Brian Warner [:warner :bwarner]]

smionean: could you post the line of code that caused the problem? I'd like
to confirm that you're experiencing the same bug, or if maybe there's a
second problem lurking here.

I checked with Daniel on IRC, and it sounds like the code he's thinking of
used a different syntax. So I'm still of the opinion that this specific
problem has been around for a long time.

That said, it's pretty annoying, so I agree we should change it. The downside
will be that quoted strings which use require()-like syntax (maybe in an
error message) will be parsed by the scanner, and that will throw an
exception during 'cfx xpi' if the name it thinks you're requiring doesn't
correspond to a real module.

The best alternative I can think of is to teach our developers that require()
statements belong at the top-level, in stereotypical lines like:

 const tabs = require("tabs");

and then discourage them from using require() in the main body of their code.
This would have other advantages: the Perl/Python/Java/C communities use
similar conventions, because it makes the code more readable (all the
dependencies are in one place, at the beginning of the file). But given the
diverse audience we're aiming for, and the seriously unhelpful nature of the
error message you get when the scanner misses your require(), it's probably
an unwinnable herding-cats exercise to try and make this convention stick.

The second-best alternative I can think of, which is a bit longer-term, is to
embed a JS parser into the SDK, so it can find these statements more
accurately. Myk just pointed me at http://code.google.com/p/pynarcissus/ ,
which looks promising (it'd add about 50kB of python). I'll investigate that
option after fixing this ticket.

Comment 9

[Brian Warner [:warner :bwarner]]

Comment on attachment 553635 [details] [diff] [review]
include require() in quote-prefixed lines

Ok, I did a scan and didn't find any cases of quote-prefixed require() statements in the SDK codebase, so I think this is safe to apply. Looking for review now.

Comment 10

[Brian Warner [:warner :bwarner]]

Attachment: tool to locate unusual require() statements in SDK packages

for reference, here's the script to look for unusual (quote- or comment- prefixed) require() statements in a set of directories. Run like "python find_weird_requires.py packages/*"

Comment 11

[Brian Warner [:warner :bwarner]]

Incidentally, I've opened Bug 683788 to explore more accurate JS parsers (using pynarcissus).

Comment 12

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

Comment on attachment 553635 [details] [diff] [review]
include require() in quote-prefixed lines

(In reply to Brian Warner [:warner :bwarner] from comment #1)
> We could try to improve the manifest scanner to look for matching quotes,
> but that's a losing battle

I've had some success with this in the past.  It gets complicated when you try to exclude escaped quotes, but it's still doable.  So if feedback on this change suggests that a lot of folks are getting tripped up by the new behavior, and we aren't ready to land a parser, then we should give this a shot.

Comment 13

[Brian Warner [:warner :bwarner]]

Landed, in https://github.com/mozilla/addon-sdk/commit/f88122a612ce8f6339a0b79b0f491e9ff2f8ba81 .