649361 - require("passwords").store() throws exception if optional onComplete not provided

Status: RESOLVED FIXED

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

Description

[Myk Melez [:myk] [@mykmelez]]

In bug 646865, comment 3, Will says:

Also: the doc describes the onComplete option to store() as optional, but if I
omit it, I get an error like:

error: An exception occurred.
Traceback (most recent call last):
  File "resource://jid0-04cbe23veepc3of5vclk4ytwjw8-pw1-lib/main.js", line 1,
in 
    require("passwords").store({
  File
"resource://jid0-04cbe23veepc3of5vclk4ytwjw8-addon-kit-lib/passwords.js", line
73, in 
    onComplete(wrapped(options));
TypeError: onComplete is not a function

(the corresponding code is the first example under the store() function:

require("passwords").store({
  realm: "User Registration",
  username: "joe",
  password: "SeCrEt123",
});

)


The docs are right that onComplete is optional, so that shouldn't happen.

Comment 1

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

I'll give this a shot. Hoping to have time tonight to take a look at it.

Comment 2

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

Passwords module seems really broken. The only time I can get the onError callback to do anything is if I don't specify onComplete. (And then it only catches the "onComplete is not a function" exception.)

Manually throwing an exception from an onComplete callback that I add doesn't get caught by onError. 

Trying to store() the same credentials twice throws an exception, but it also isn't caught by the onError callback.

I think I can fix this specific bug (onComplete seems to be required) by changing the following line of code in passwords.js:
    'onComplete' in options ? defer(options.onComplete) : null,
to be this:
    'onComplete' in options ? defer(options.onComplete) : defer(function() { }),


But I think that would make onComplete be optional for search(), which is the case where it IS required.

I'm still not quite sure I understand this module's code with all of the deferrals and async-ness.

Myk, Gozala: am I not getting something about this? Would it be best to split createWrapperMethod() and/or getCallbacks() into separate functions for the case where onComplete is required (search) vs the ones where it isn't (store/remove)? Is onError actually useful?

Comment 3

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

(In reply to comment #2)
> The only time I can get the onError
> callback to do anything is if I don't specify onComplete. (And then it only
> catches the "onComplete is not a function" exception.)
Not quite true, I guess. If I add a 'throw "some error"'; thing into passwords.js's createWrapperMethod's try block, my onError callback is fired off with "some error".

I guess whatever black magic is making everything async/deferred is eating whatever exceptions get created in onComplete or in the nsILoginManager component before onError gets a chance to see it.

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Hi Wes,

You are right, I forgot to take into account exceptions that may be thrown by `onComplete`.
`defer(foo)` is same as `function() { setTimeout(foo, 0) }` so errors thrown by `foo` just get lost (actually logged into error console as unhandled exceptions).

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/156

Pointer to Github pull-request

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 529954 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/156

this looks ok, r=me. one minor nit: it'd be good to use the methods in the 'errors' module for this, and if they're not sufficient, to update them.

Comment 7

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

https://github.com/mozilla/addon-sdk/commit/7443adc97f4971554ebb5267fe745b56c082f4b4