1020292 - defineLazyModuleGetter and defineLazyServiceGetter should cause test failures if they fail

Status: RESOLVED WONTFIX

(Toolkit :: Async Tooling, defect)

Comment 1

[David Teller [:Yoric] - still alive but not very active]

As far as I can tell, an error in a call to defineLazy{Module, Service, ø}Getter should cause a test failure.

There are several ways to implement this. From the top of my head, in case of defineLazy{Module, Service, ø}Getter error, we can:

1. Crash.
2. Leak a Promise.reject() and let the existing uncaught rejections mechanisms handle the issue.
3. Notify the observer service on a topic "uncaught-async-error" and adapt the test harnesses to fail upon notification of these topics.

Cons:
1. We don't want this in release.
2. Adds a dependency towards Promise.jsm. May or may not be a problem (by now, we're using Promise.jsm all around the code).
3. We need to adapt each test harness independently.

Any idea?

Comment 2

[:Paolo Amadini]

(In reply to David Rajchenbach Teller [:Yoric] from comment #1)
> As far as I can tell, an error in a call to defineLazy{Module, Service,
> ø}Getter should cause a test failure.

Lazy getters fail lazily, but if the code that uses the getter is exercised and the getter fails to load the target, I expect the test to fail anyways.

So there shouldn't be an actual issue I think, or am I missing something?

Comment 3

[Marco Bonardo [:mak]]

I think paolo is right, this sounds a little bit paranoid error checking, if importing a module fails, some code will be very broken.  The only advantage seems to figure if some module is not exercised by any test, but still it sounds a rare case that nobody notices that in product (and at that point I expect someone will add a test for the filed bug)

Comment 4

[David Teller [:Yoric] - still alive but not very active]

It's more about finding the error early, rather than having to painfully trace the final error back to the missing import (without any stack information).

Comment 5

[Marco Bonardo [:mak]]

I can't imagine how may be painful to trace a "Something is undefined" error, when Something is exported by a module... Do you think that's common enough and hard to track to deserve specific error handling? I don't think so, but maybe I am ignoring some important fact that you already considered. Off-hand I don't see a large enough benefit to cover adding/complicating module import code.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

I vaguely recall the following bug in WebApps code:
1. During its first load, module "Foo" does some important side-effect (register some observers, maybe, I don't remember the details);
2. Module "Foo" is imported lazily, but with an error;
3. Import takes place in some observer and the error is swallowed (probably by xpconnect, I don't remember the details);
4. Failure appears several hundred lines below in the logs;
5. Debugging fun ensues.

Now, one can argue that the real issue is point 3. (aka bug 991595).

Comment 7

[Marco Bonardo [:mak]]

It sounds like a very exotic case as well as sounds like that module had poor testing coverage, that is a review-failure first of all.
I'm pretty sure there are cases like that around and this code would catch them, but I'm arguing that the code complication cost overweight that benefit if we do anything that is not trivial.

Moreover, in case of defineLazyModuleGetter we reportError eventual failures (http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/XPCOMUtils.jsm#229) it's hard to not notice that.
If you wish, we could also dump() there, so it's also visible in xpcshell-tests and in the console.
And as you said, the other errors should not be swallowed.

Btw, I didn't reply comment 0. The most trivial path would be to crash, but I think it wouldn't really solve the issue you are pointing out, developer spending time trying to figure what happened, indeed a crash may well cause him to spend hours trying to debug cpp code to find the crash origin (it would be different if from js we had something like MOZ_ASSERT that crashes but prints a reason and the js stack for that).
3. is a no-go due to the cost-benefit weight.

The only remaining option is 2, would the stack be clear enough in that case, may we provide a reject argument that would be printed out by the harness?

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Closing bug. Let's reopen it after the next catastrophe :)

> The only remaining option is 2, would the stack be clear enough in that case, may we provide a reject argument that would be printed out by the harness?

Yes to both.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

So, I have been hit by this again.

STR:
1. Some problem prevents a module from being [lazy] importable. Error message gets lost in mochitest noise.
2. Waste time finding out what's wrong.

Comment 10

[:Paolo Amadini]

(In reply to David Rajchenbach Teller [:Yoric] from comment #9)
> Error message gets lost in mochitest noise.

This is the right thing to spend time fixing.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Well, I have been bitten by this again.