683788 - rewrite require() scanner with pynarcissus

Status: RESOLVED WONTFIX

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

Description

[Brian Warner [:warner :bwarner]]

Bug 679554 pointed out the fragility of using regexps to scan javascript code for require() statements. Our security model doesn't depend upon this scanner being exactly correct, but deviations from what the real runtime spidermonkey sees will result in surprises and occasional frustration to developers who weren't trying to do anything sneaky.

http://code.google.com/p/pynarcissus/ is a pure-python javascript parser. It's a bit old: last updated in mid 2009. Assuming that JS syntax hasn't changed too much since then, it should be possible to build a scanner from this parser instead of using regexps. This require() scanner should be a lot more accurate: it should ignore comments and literal strings properly.

I think we're eventually going to move the SDK into JS and let it live in an add-on itself, at which point we'll be able to use spidermonkey's built-in parser (which of course will be highly accurate). But I think using pynarcissus would bridge the gap until we get there.

I'd call this low priority: when developers tell us that they're seeing frustrating errors (like in bug 679554), we should put more time into it.

Comment 1

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

(Pushing all open bugs to the --- milestone for the new triage system)

Comment 2

[Brian Warner [:warner :bwarner]]

note to self: when this is fixed, we can remove the "tolerate missing modules in tests" clause (python-lib/cuddlefish/manifest.py ManifestBuilder.process_module() L394), since the one test module that it stumbles upon (packages/api-utils/tests/modules/red.js, with a three-line "/*","  require('bad2');","*/" comment sequence) will be handled correctly. Normal python-side unit tests should be used to validate that commented sequences like this are excluded, and red.js should just be a fallback test.

Comment 3

[Brian Warner [:warner :bwarner]]

Also, there is loader code which takes advantage of the current scanner being kinda dumb. In addition spotting calls to the global "require" function, the scanner also needs to spot calls like 'var loader = require("@loader").Loader().new(); loader.require("foo");' which happens in a number of test cases. Also look through cuddlefish.js for places that load "helper" modules and make sure their forms of require() calls will be processed too.

Comment 4

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

We'll probably get cfx in JS before we get to this. Feel free to reopen if you disagree.