diff --git a/dom/identity/DOMIdentity.jsm b/dom/identity/DOMIdentity.jsm index c30d5d03b77..2c411ce8ad2 100644 --- a/dom/identity/DOMIdentity.jsm +++ b/dom/identity/DOMIdentity.jsm @@ -336,7 +336,15 @@ this.DOMIdentity = { _unwatch: function DOMIdentity_unwatch(message, targetMM) { log("DOMIDentity__unwatch: " + message.id); - this.getService(message).RP.unwatch(message.id, targetMM); + // If watch failed for some reason (e.g., exception thrown because RP did + // not have the right callbacks, we don't want unwatch to throw, because it + // will break the process of releasing the page's resources and leak + // memory. + try { + this.getService(message).RP.unwatch(message.id, targetMM); + } catch(ex) { + log("ERROR: can't unwatch " + message.id + ": " + ex); + } }, _request: function DOMIdentity__request(message) { diff --git a/dom/identity/nsDOMIdentity.js b/dom/identity/nsDOMIdentity.js index af75473f40f..73cf6c93844 100644 --- a/dom/identity/nsDOMIdentity.js +++ b/dom/identity/nsDOMIdentity.js @@ -101,7 +101,7 @@ nsDOMIdentity.prototype = { * Relying Party (RP) APIs */ - watch: function nsDOMIdentity_watch(aOptions) { + watch: function nsDOMIdentity_watch(aOptions = {}) { if (this._rpWatcher) { // For the initial release of Firefox Accounts, we support callers who // invoke watch() either for Firefox Accounts, or Persona, but not both. @@ -111,33 +111,7 @@ nsDOMIdentity.prototype = { throw new Error("navigator.id.watch was already called"); } - if (!aOptions || typeof(aOptions) !== "object") { - throw new Error("options argument to watch is required"); - } - - // The relying party (RP) provides callbacks on watch(). - // - // In the future, BrowserID will probably only require an onlogin() - // callback [1], lifting the requirement that BrowserID handle logged-in - // state management for RPs. See - // https://github.com/mozilla/id-specs/blob/greenfield/browserid/api-rp.md - // - // However, Firefox Accounts will almost certainly require RPs to provide - // onlogout(), onready(), and possibly an onerror() callback. - // XXX Bug 945278 - // - // To accomodate the more and less lenient uses of the API, we will simply - // be strict about checking for onlogin here. - if (typeof(aOptions["onlogin"]) != "function") { - throw new Error("onlogin() callback is required."); - } - - // Optional callbacks - for (let cb of ["onready", "onerror", "onlogout"]) { - if (aOptions[cb] && typeof(aOptions[cb]) != "function") { - throw new Error(cb + " must be a function"); - } - } + assertCorrectCallbacks(aOptions); let message = this.DOMIdentityMessage(aOptions); @@ -797,7 +771,38 @@ nsDOMIdentityInternal.prototype = { interfaces: [], classDescription: "Identity DOM Implementation" }) - }; +function assertCorrectCallbacks(aOptions) { + // The relying party (RP) provides callbacks on watch(). + // + // In the future, BrowserID will probably only require an onlogin() + // callback, lifting the requirement that BrowserID handle logged-in + // state management for RPs. See + // https://github.com/mozilla/id-specs/blob/greenfield/browserid/api-rp.md + // + // However, Firefox Accounts requires callers to provide onlogout(), + // onready(), and also supports an onerror() callback. + + let requiredCallbacks = ["onlogin"]; + let optionalCallbacks = ["onlogout", "onready", "onerror"]; + + if (aOptions.wantIssuer == "firefox-accounts") { + requiredCallbacks = ["onlogin", "onlogout", "onready"]; + optionalCallbacks = ["onerror"]; + } + + for (let cbName of requiredCallbacks) { + if (typeof(aOptions[cbName]) != "function") { + throw new Error(cbName + " callback is required."); + } + } + + for (let cbName of optionalCallbacks) { + if (aOptions[cbName] && typeof(aOptions[cbName]) != "function") { + throw new Error(cbName + " must be a function"); + } + } +} + this.NSGetFactory = XPCOMUtils.generateNSGetFactory([nsDOMIdentityInternal]); diff --git a/dom/identity/tests/mochitest/chrome.ini b/dom/identity/tests/mochitest/chrome.ini index 965f760cd4c..a19d9f30a63 100644 --- a/dom/identity/tests/mochitest/chrome.ini +++ b/dom/identity/tests/mochitest/chrome.ini @@ -1,9 +1,16 @@ [DEFAULT] support-files= + file_browserid_rp_ok.html + file_browserid_rp_noOnlogin.html file_declareAudience.html + file_fxa_rp_ok.html + file_fxa_rp_noOnready.html + file_fxa_rp_noOnlogin.html + file_fxa_rp_noOnlogout.html file_syntheticEvents.html [test_declareAudience.html] +[test_rpHasValidCallbacks.html] [test_syntheticEvents.html] diff --git a/dom/identity/tests/mochitest/file_browserid_rp_noOnlogin.html b/dom/identity/tests/mochitest/file_browserid_rp_noOnlogin.html new file mode 100644 index 00000000000..72e84bbd026 --- /dev/null +++ b/dom/identity/tests/mochitest/file_browserid_rp_noOnlogin.html @@ -0,0 +1,44 @@ + + + + + + + Test app for bug 945363 + + + + + + diff --git a/dom/identity/tests/mochitest/file_browserid_rp_ok.html b/dom/identity/tests/mochitest/file_browserid_rp_ok.html new file mode 100644 index 00000000000..ab34652e62f --- /dev/null +++ b/dom/identity/tests/mochitest/file_browserid_rp_ok.html @@ -0,0 +1,43 @@ + + + + + + + Test app for bug 945363 + + + + + + diff --git a/dom/identity/tests/mochitest/file_fxa_rp_noOnlogin.html b/dom/identity/tests/mochitest/file_fxa_rp_noOnlogin.html new file mode 100644 index 00000000000..ced1877530f --- /dev/null +++ b/dom/identity/tests/mochitest/file_fxa_rp_noOnlogin.html @@ -0,0 +1,46 @@ + + + + + + + Test app for bug 945363 + + + + + + diff --git a/dom/identity/tests/mochitest/file_fxa_rp_noOnlogout.html b/dom/identity/tests/mochitest/file_fxa_rp_noOnlogout.html new file mode 100644 index 00000000000..78fb76f5262 --- /dev/null +++ b/dom/identity/tests/mochitest/file_fxa_rp_noOnlogout.html @@ -0,0 +1,46 @@ + + + + + + + Test app for bug 945363 + + + + + + diff --git a/dom/identity/tests/mochitest/file_fxa_rp_noOnready.html b/dom/identity/tests/mochitest/file_fxa_rp_noOnready.html new file mode 100644 index 00000000000..d9987db6323 --- /dev/null +++ b/dom/identity/tests/mochitest/file_fxa_rp_noOnready.html @@ -0,0 +1,46 @@ + + + + + + + Test app for bug 945363 + + + + + + diff --git a/dom/identity/tests/mochitest/file_fxa_rp_ok.html b/dom/identity/tests/mochitest/file_fxa_rp_ok.html new file mode 100644 index 00000000000..0fd876cd91b --- /dev/null +++ b/dom/identity/tests/mochitest/file_fxa_rp_ok.html @@ -0,0 +1,46 @@ + + + + + + + Test app for bug 945363 + + + + + + diff --git a/dom/identity/tests/mochitest/test_rpHasValidCallbacks.html b/dom/identity/tests/mochitest/test_rpHasValidCallbacks.html new file mode 100644 index 00000000000..a12a20ec9c3 --- /dev/null +++ b/dom/identity/tests/mochitest/test_rpHasValidCallbacks.html @@ -0,0 +1,95 @@ + + + + + + + BrowserID and Firefox Accounts RPs provide requried callbacks - Bug 945363 + + + + + +Mozilla Bug 945363 +

+
+ +
+
+
+
+ + diff --git a/toolkit/identity/tests/unit/head_identity.js b/toolkit/identity/tests/unit/head_identity.js index 20917c794f1..583f5802d3f 100644 --- a/toolkit/identity/tests/unit/head_identity.js +++ b/toolkit/identity/tests/unit/head_identity.js @@ -213,8 +213,8 @@ function setup_provisioning(identity, afterSetupCallback, doneProvisioningCallba doneProvisioningCallback(err); }, sandbox: { - // Emulate the free() method on the iframe sandbox - free: function() {} + // Emulate the free() method on the iframe sandbox + free: function() {} } }; diff --git a/toolkit/identity/tests/unit/test_firefox_accounts.js b/toolkit/identity/tests/unit/test_firefox_accounts.js index 698f2af3b45..ecc4a1eb3e6 100644 --- a/toolkit/identity/tests/unit/test_firefox_accounts.js +++ b/toolkit/identity/tests/unit/test_firefox_accounts.js @@ -15,18 +15,23 @@ XPCOMUtils.defineLazyModuleGetter(this, "FirefoxAccounts", do_get_profile(); function MockFXAManager() { - this.signedIn = true; + this.signedInUser = true; } MockFXAManager.prototype = { getAssertion: function(audience) { - let result = this.signedIn ? TEST_ASSERTION : null; + let result = this.signedInUser ? TEST_ASSERTION : null; return Promise.resolve(result); }, signOut: function() { - this.signedIn = false; + this.signedInUser = false; return Promise.resolve(null); }, + + signIn: function(user) { + this.signedInUser = user; + return Promise.resolve(user); + }, } let originalManager = FirefoxAccounts.fxAccountsManager; @@ -36,6 +41,14 @@ do_register_cleanup(() => { FirefoxAccounts.fxAccountsManager = originalManager; }); +function withNobodySignedIn() { + return FirefoxAccounts.fxAccountsManager.signOut(); +} + +function withSomebodySignedIn() { + return FirefoxAccounts.fxAccountsManager.signIn('Pertelote'); +} + function test_overall() { do_check_neq(FirefoxAccounts, null); run_next_test(); @@ -44,10 +57,12 @@ function test_overall() { function test_mock() { do_test_pending(); - FirefoxAccounts.fxAccountsManager.getAssertion().then(assertion => { - do_check_eq(assertion, TEST_ASSERTION); - do_test_finished(); - run_next_test(); + withSomebodySignedIn().then(() => { + FirefoxAccounts.fxAccountsManager.getAssertion().then(assertion => { + do_check_eq(assertion, TEST_ASSERTION); + do_test_finished(); + run_next_test(); + }); }); } @@ -70,15 +85,15 @@ function test_watch_signed_in() { } }); - FirefoxAccounts.RP.watch(mockedRP); + withSomebodySignedIn().then(() => { + FirefoxAccounts.RP.watch(mockedRP); + }); } function test_watch_signed_out() { do_test_pending(); let received = []; - let signedInState = FirefoxAccounts.fxAccountsManager.signedIn; - FirefoxAccounts.fxAccountsManager.signedIn = false; let mockedRP = mock_fxa_rp(null, TEST_URL, function(method) { received.push(method); @@ -89,14 +104,14 @@ function test_watch_signed_out() { do_check_eq(received[0], "logout"); do_check_eq(received[1], "ready"); - // restore initial state - FirefoxAccounts.fxAccountsManager.signedIn = signedInState; do_test_finished(); run_next_test(); } }); - FirefoxAccounts.RP.watch(mockedRP); + withNobodySignedIn().then(() => { + FirefoxAccounts.RP.watch(mockedRP); + }); } function test_request() { @@ -104,10 +119,6 @@ function test_request() { let received = []; - // initially signed out - let signedInState = FirefoxAccounts.fxAccountsManager.signedIn; - FirefoxAccounts.fxAccountsManager.signedIn = false; - let mockedRP = mock_fxa_rp(null, TEST_URL, function(method, data) { received.push([method, data]); @@ -117,23 +128,24 @@ function test_request() { do_check_eq(received[1][0], "ready"); // Pretend request() showed ux and the user signed in - FirefoxAccounts.fxAccountsManager.signedIn = true; - FirefoxAccounts.RP.request(mockedRP.id); + withSomebodySignedIn().then(() => { + FirefoxAccounts.RP.request(mockedRP.id); + }); } if (received.length === 3) { do_check_eq(received[2][0], "login"); do_check_eq(received[2][1], TEST_ASSERTION); - // restore initial state - FirefoxAccounts.fxAccountsManager.signedIn = signedInState; do_test_finished(); run_next_test(); } }); - // First, call watch() - FirefoxAccounts.RP.watch(mockedRP); + // First, call watch() with nobody signed in + withNobodySignedIn().then(() => { + FirefoxAccounts.RP.watch(mockedRP); + }); } function test_logout() { @@ -160,7 +172,9 @@ function test_logout() { }); // First, call watch() - FirefoxAccounts.RP.watch(mockedRP); + withSomebodySignedIn().then(() => { + FirefoxAccounts.RP.watch(mockedRP); + }); } function test_error() { @@ -171,11 +185,9 @@ function test_error() { // Mock the fxAccountsManager so that getAssertion rejects its promise and // triggers our onerror handler. (This is the method that's used internally // by FirefoxAccounts.RP.request().) - let originalManager = FirefoxAccounts.fxAccountsManager; - FirefoxAccounts.RP.fxAccountsManager = { - getAssertion: function(audience) { - return Promise.reject(new Error("barf!")); - } + let originalGetAssertion = FirefoxAccounts.fxAccountsManager.getAssertion; + FirefoxAccounts.fxAccountsManager.getAssertion = function(audience) { + return Promise.reject(new Error("barf!")); }; let mockedRP = mock_fxa_rp(null, TEST_URL, function(method, message) { @@ -185,14 +197,16 @@ function test_error() { do_check_true(/barf/.test(message)); // Put things back the way they were - FirefoxAccounts.fxAccountsManager = originalManager; + FirefoxAccounts.fxAccountsManager.getAssertion = originalGetAssertion; do_test_finished(); run_next_test(); }); // First, call watch() - FirefoxAccounts.RP.watch(mockedRP); + withSomebodySignedIn().then(() => { + FirefoxAccounts.RP.watch(mockedRP); + }); } function test_child_process_shutdown() { @@ -230,7 +244,9 @@ function test_child_process_shutdown() { }); mockedRP._mm = "my message manager"; - FirefoxAccounts.RP.watch(mockedRP); + withSomebodySignedIn().then(() => { + FirefoxAccounts.RP.watch(mockedRP); + }); // fake a dom window context DOMIdentity.newContext(mockedRP, mockedRP._mm);