If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Too many debug assertions in test runs

RESOLVED FIXED

Status

Add-on SDK
General
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

46 bytes, text/x-github-pull-request
mossop
: review+
erikvold
: feedback+
Details | Review | Splinter Review
(Assignee)

Description

3 years ago
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
Priority: -- → P1
Blocks: 1117922
(Assignee)

Updated

3 years ago
Blocks: 1140001
(Assignee)

Comment 1

3 years ago
Most likely bug 1036625 again
Depends on: 1036625
(Assignee)

Comment 2

3 years ago
Created attachment 8578990 [details] [review]
pull request
Assignee: nobody → dtownsend
(Assignee)

Comment 3

3 years ago
Comment on attachment 8578990 [details] [review]
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.
Attachment #8578990 - Flags: review?(evold)
Comment on attachment 8578990 [details] [review]
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.
Attachment #8578990 - Flags: review?(evold)
Attachment #8578990 - Flags: review-
Attachment #8578990 - Flags: feedback+
(Assignee)

Comment 5

3 years ago
Comment on attachment 8578990 [details] [review]
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.
Attachment #8578990 - Flags: review- → review?(evold)
(In reply to Dave Townsend [:mossop] from comment #5)
> Comment on attachment 8578990 [details] [review]
> 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?
Flags: needinfo?(dtownsend)
(Assignee)

Comment 7

3 years ago
(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] [review]
> > 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?
Flags: needinfo?(dtownsend) → needinfo?(evold)
(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] [review]
> > > 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.
Flags: needinfo?(evold)
(Assignee)

Comment 9

3 years ago
Comment on attachment 8578990 [details] [review]
pull request

r+ over IRC, I filed bug 1146643 on the follow-up test.
Attachment #8578990 - Flags: review?(evold) → review+

Comment 10

3 years ago
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
(Assignee)

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.