Closed
Bug 679554
Opened 13 years ago
Closed 13 years ago
require() statements fail when preceded by a leading quotation mark
Categories
(Add-on SDK Graveyard :: General, defect, P3)
Add-on SDK Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
1.3
People
(Reporter: dbuchner, Assigned: warner)
Details
Attachments
(2 files)
1.14 KB,
patch
|
myk
:
review+
|
Details | Diff | Splinter Review |
2.38 KB,
text/plain
|
Details |
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)
Reporter | ||
Updated•13 years ago
|
Assignee: nobody → warner-bugzilla
Assignee | ||
Comment 1•13 years ago
|
||
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.
Assignee | ||
Comment 2•13 years ago
|
||
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•13 years ago
|
||
Brian: is this a regression from 1.0, or has it existed since before that release?
Updated•13 years ago
|
Whiteboard: [triage:followup]
Assignee | ||
Comment 4•13 years ago
|
||
It's been around forever, at least since 0.6, which used the same comment-prefixes as we've got now.
Updated•13 years ago
|
Priority: -- → P3
Whiteboard: [triage:followup]
Target Milestone: --- → 1.3
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)
Bringing back the triage flag because comment 4 and comment 5 are in conflict.
Whiteboard: [triage:followup]
Reporter | ||
Comment 7•13 years ago
|
||
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...
Assignee | ||
Comment 8•13 years ago
|
||
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.
Assignee | ||
Comment 9•13 years ago
|
||
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.
Attachment #553635 -
Flags: review?(myk)
Assignee | ||
Comment 10•13 years ago
|
||
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/*"
Assignee | ||
Comment 11•13 years ago
|
||
Incidentally, I've opened Bug 683788 to explore more accurate JS parsers (using pynarcissus).
Comment 12•13 years ago
|
||
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.
Attachment #553635 -
Flags: review?(myk) → review+
Assignee | ||
Comment 13•13 years ago
|
||
Landed, in https://github.com/mozilla/addon-sdk/commit/f88122a612ce8f6339a0b79b0f491e9ff2f8ba81 .
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Whiteboard: [triage:followup]
You need to log in
before you can comment on or make changes to this bug.
Description
•