Closed Bug 649361 Opened 9 years ago Closed 9 years ago

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

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: myk, Assigned: irakli)

Details

Attachments

(1 file)

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.
Priority: -- → P2
Target Milestone: --- → 1.0
I'll give this a shot. Hoping to have time tonight to take a look at it.
Assignee: nobody → kwierso
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?
(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.
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).
Assignee: kwierso → rFobic
Attachment #529954 - Flags: review?(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.
Attachment #529954 - Flags: review?(dietrich) → review+
You need to log in before you can comment on or make changes to this bug.