Similar to bug 1077078 we are still seeing a lot of debug assertions. This makes the debug logs for mochitest-jetpack too long in debug builds. 22:34:38 INFO - [3928] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/netwerk/protocol/res/nsResProtocolHandler.cpp, line 289 22:34:38 INFO - [3928] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/netwerk/protocol/res/nsResProtocolHandler.cpp, line 289 22:34:38 INFO - [3928] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\netwerk\base\nsIOService.cpp, line 704

Most likely bug 1036625 again

Comment on attachment 8578990 [details] pull request This is a less invasive change than bug 1036625. It has two pieces. The first piece is the only bit really required to solve the problem with debug tests. It just verifies resource URIs resolve to something before attempting to load them. The other parts of the patch make module resolution a little leaner, meaning we don't attempt to load as many bad URIs in the first place. Instead of attempting to find node modules all the way up to the root of the tree it instead stops at the rootURI for the loader. So for a loader at resource://gre/modules/commonjs we won't try to find modules at resource://node_modules, resource://gre/node_modules and resource://gre/modules/node_modules anymore. I've also included a couple of simple checks for absolute URIs that avoid doing node module resolution when it definitely wouldn't work anyway.

Comment on attachment 8578990 [details] pull request Nice! I only r- because we are at risk of regressing without some tests. I think for the issue of logging too much for debug builds we will have to download a debug build in our travis suite, run some tests in a child_process, read the stdout and analyze if the logs we wish to remove are actually removed from stdout/stderr.

Comment on attachment 8578990 [details] pull request Added a couple of tests here. The tests pass with and without the code changes since we aren't changing what nodeResolve returns merely creating a fast path for those cases to avoid doing needless file lookups.

(In reply to Dave Townsend [:mossop] from comment #5) > Comment on attachment 8578990 [details] > pull request > > Added a couple of tests here. The tests pass with and without the code > changes since we aren't changing what nodeResolve returns merely creating a > fast path for those cases to avoid doing needless file lookups. I think you missed the part about the travis test I was requesting?

(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #6) > (In reply to Dave Townsend [:mossop] from comment #5) > > Comment on attachment 8578990 [details] > > pull request > > > > Added a couple of tests here. The tests pass with and without the code > > changes since we aren't changing what nodeResolve returns merely creating a > > fast path for those cases to avoid doing needless file lookups. > > I think you missed the part about the travis test I was requesting? So I think there might be an easier way to catch this using the debugger API but either way I don't have time to look into testing this part before next week. Can we do that as a follow-up so we can get this landed?

(In reply to Dave Townsend [:mossop] from comment #7) > (In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #6) > > (In reply to Dave Townsend [:mossop] from comment #5) > > > Comment on attachment 8578990 [details] > > > pull request > > > > > > Added a couple of tests here. The tests pass with and without the code > > > changes since we aren't changing what nodeResolve returns merely creating a > > > fast path for those cases to avoid doing needless file lookups. > > > > I think you missed the part about the travis test I was requesting? > > So I think there might be an easier way to catch this using the debugger API > but either way I don't have time to look into testing this part before next > week. Can we do that as a follow-up so we can get this landed? I've started a test here https://github.com/mozilla/addon-sdk/pull/1911 which downloads debug builds and uses a special addon to test logging, I just need to put the final touches on it, not sure if I'll have time tonight but I shall try.

Comment on attachment 8578990 [details] pull request r+ over IRC, I filed bug 1146643 on the follow-up test.

Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/af7133332a24c4822548c7a8a4651cc37e3e3d50 Bug 1135219: Avoid opening channels for obviously bad URIs. Don't resolve node_module paths above the loader's rootURI. r=erikvold