Bug 1240169 - aboutNewTabService relies on AboutRedirector for default URL resolution r?mconley draft
authorOlivier Yiptong <olivier@olivieryiptong.com>
Wed, 27 Jan 2016 13:50:18 -0500
changeset 326257 9934709fb2d531679984d517bec8ac6c4f53e368
parent 324482 5f7c184ccd800b2ed512c23fb609007efd198eaf
child 326258 3a81a700af0879998c63fb439316971089eb00ea
push id10118
push userolivier@olivieryiptong.com
push dateWed, 27 Jan 2016 18:51:35 +0000
reviewersmconley
bugs1240169
milestone46.0a1
Bug 1240169 - aboutNewTabService relies on AboutRedirector for default URL resolution r?mconley
browser/base/content/test/newtab/browser.ini
browser/base/content/test/newtab/browser_newtab_external_resource.js
browser/base/content/test/newtab/external_newtab.html
browser/components/about/AboutRedirector.cpp
browser/components/extensions/test/browser/browser_ext_tabs_create.js
browser/components/newtab/NewTabComponents.manifest
browser/components/newtab/NewTabURL.jsm
browser/components/newtab/aboutNewTabService.js
browser/components/newtab/nsIAboutNewTabService.idl
browser/components/newtab/tests/browser/browser.ini
browser/components/newtab/tests/browser/browser_newtab_overrides.js
browser/components/newtab/tests/browser/browser_remotenewtab_pageloads.js
browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js
browser/components/newtab/tests/xpcshell/test_NewTabURL.js
--- a/browser/base/content/test/newtab/browser.ini
+++ b/browser/base/content/test/newtab/browser.ini
@@ -45,11 +45,8 @@ support-files =
 [browser_newtab_sponsored_icon_click.js]
 [browser_newtab_undo.js]
 [browser_newtab_unpin.js]
 [browser_newtab_update.js]
 [browser_newtab_bug1145428.js]
 [browser_newtab_bug1178586.js]
 [browser_newtab_bug1194895.js]
 [browser_newtab_1188015.js]
-[browser_newtab_external_resource.js]
-support-files =
-  external_newtab.html
deleted file mode 100644
--- a/browser/base/content/test/newtab/browser_newtab_external_resource.js
+++ /dev/null
@@ -1,75 +0,0 @@
-/*
- * Description of the Tests for
- *  - Bug 1204983 - Allow about: pages to load remote content
- *
- * We perform two tests:
- * (1) We load a new tab (about:newtab) using the default url and make sure that URL
- *     of the doucment matches about:newtab and the principal is the systemPrincipal.
- * (2) We load a new tab (about:newtab) and make sure that document.location as well
- *     as the nodePrincipal match the URL in the URL bar.
- */
-
-/* globals Cc, Ci, ok, is, content, TestRunner, addNewTabPageTab, gWindow, Services, info */
-/* exported runTests */
-
-"use strict";
-
-var browser = null;
-var aboutNewTabService = Cc["@mozilla.org/browser/aboutnewtab-service;1"]
-                           .getService(Ci.nsIAboutNewTabService);
-
-const ABOUT_NEWTAB_URI = "about:newtab";
-const PREF_URI = "http://example.com/browser/browser/base/content/test/newtab/external_newtab.html";
-const DEFAULT_URI = aboutNewTabService.newTabURL;
-
-function testPref() {
-  // set the pref for about:newtab to point to an exteranl resource
-  aboutNewTabService.newTabURL = PREF_URI;
-  ok(aboutNewTabService.overridden,
-     "sanity check: default URL for about:newtab should be overriden");
-  is(aboutNewTabService.newTabURL, PREF_URI,
-     "sanity check: default URL for about:newtab should return the new URL");
-
-  browser.contentWindow.location = ABOUT_NEWTAB_URI;
-
-  browser.addEventListener("load", function onLoad() {
-    browser.removeEventListener("load", onLoad, true);
-    is(content.document.location, PREF_URI, "document.location should match the external resource");
-    is(content.document.documentURI, PREF_URI, "document.documentURI should match the external resource");
-    is(content.document.nodePrincipal.URI.spec, PREF_URI, "nodePrincipal should match the external resource");
-
-    // reset to about:newtab and perform sanity check
-    aboutNewTabService.resetNewTabURL();
-    is(aboutNewTabService.newTabURL, DEFAULT_URI,
-       "sanity check: resetting the URL to about:newtab should return about:newtab");
-
-    // remove the tab and move on
-    gBrowser.removeCurrentTab();
-    TestRunner.next();
-  }, true);
-}
-
-function runTests() {
-  // test the default behavior
-  yield addNewTabPageTab();
-  browser = gWindow.gBrowser.selectedBrowser;
-
-  ok(!aboutNewTabService.overridden,
-     "sanity check: default URL for about:newtab should not be overriden");
-  browser.contentWindow.location = ABOUT_NEWTAB_URI;
-
-  browser.addEventListener("load", function onLoad() {
-    browser.removeEventListener("load", onLoad, true);
-    is(content.document.location, ABOUT_NEWTAB_URI, "document.location should match about:newtab");
-    is(content.document.documentURI, ABOUT_NEWTAB_URI, "document.documentURI should match about:newtab");
-    is(content.document.nodePrincipal,
-       Services.scriptSecurityManager.getSystemPrincipal(),
-       "nodePrincipal should match systemPrincipal");
-
-    // also test the pref
-    testPref();
-  }, true);
-
-  info("Waiting for about:newtab to load ...");
-  yield true;
-}
deleted file mode 100644
--- a/browser/base/content/test/newtab/external_newtab.html
+++ /dev/null
@@ -1,11 +0,0 @@
-<!DOCTYPE HTML>
-<html>
-<!-- https://bugzilla.mozilla.org/show_bug.cgi?id=1204983 -->
-<head>
-  <meta charset="utf-8">
-  <title>Testpage for bug 1204983</title>
-</head>
-<body>
-  Just a testpage for Bug 1204983<br/>
-</body>
-</html>
--- a/browser/components/about/AboutRedirector.cpp
+++ b/browser/components/about/AboutRedirector.cpp
@@ -157,17 +157,18 @@ AboutRedirector::NewChannel(nsIURI* aURI
   for (int i = 0; i < kRedirTotal; i++) {
     if (!strcmp(path.get(), kRedirMap[i].id)) {
       nsAutoCString url;
 
       if (path.EqualsLiteral("newtab")) {
         // let the aboutNewTabService decide where to redirect
         nsCOMPtr<nsIAboutNewTabService> aboutNewTabService =
           do_GetService("@mozilla.org/browser/aboutnewtab-service;1", &rv);
-        rv = aboutNewTabService->GetNewTabURL(url);
+        NS_ENSURE_SUCCESS(rv, rv);
+        rv = aboutNewTabService->GetDefaultURL(url);
         NS_ENSURE_SUCCESS(rv, rv);
       }
       // fall back to the specified url in the map
       if (url.IsEmpty()) {
         url.AssignASCII(kRedirMap[i].url);
       }
 
       nsCOMPtr<nsIChannel> tempChannel;
--- a/browser/components/extensions/test/browser/browser_ext_tabs_create.js
+++ b/browser/components/extensions/test/browser/browser_ext_tabs_create.js
@@ -46,31 +46,31 @@ add_task(function* () {
         let activeWindow;
 
         function runTests() {
           const DEFAULTS = {
             index: 2,
             windowId: activeWindow,
             active: true,
             pinned: false,
-            url: "chrome://browser/content/newtab/newTab.xhtml",
+            url: "about:newtab",
           };
 
           let tests = [
             {
               create: { url: "http://example.com/" },
               result: { url: "http://example.com/" },
             },
             {
               create: { url: "blank.html" },
               result: { url: browser.runtime.getURL("bg/blank.html") },
             },
             {
               create: {},
-              result: { url: "chrome://browser/content/newtab/newTab.xhtml" },
+              result: { url: "about:newtab" },
             },
             {
               create: { active: false },
               result: { active: false },
             },
             {
               create: { active: true },
               result: { active: true },
--- a/browser/components/newtab/NewTabComponents.manifest
+++ b/browser/components/newtab/NewTabComponents.manifest
@@ -1,2 +1,2 @@
-component {cef25b06-0ef6-4c50-a243-e69f943ef23d} aboutNewTabService.js
-contract @mozilla.org/browser/aboutnewtab-service;1 {cef25b06-0ef6-4c50-a243-e69f943ef23d}
+component {dfcd2adc-7867-4d3a-ba70-17501f208142} aboutNewTabService.js
+contract @mozilla.org/browser/aboutnewtab-service;1 {dfcd2adc-7867-4d3a-ba70-17501f208142}
--- a/browser/components/newtab/NewTabURL.jsm
+++ b/browser/components/newtab/NewTabURL.jsm
@@ -1,13 +1,13 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-/* globals XPCOMUtils, Deprecated, aboutNewTabService*/
+/* globals XPCOMUtils, aboutNewTabService*/
 /* exported NewTabURL */
 
 "use strict";
 
 const {utils: Cu} = Components;
 
 this.EXPORTED_SYMBOLS = ["NewTabURL"];
 
--- a/browser/components/newtab/aboutNewTabService.js
+++ b/browser/components/newtab/aboutNewTabService.js
@@ -21,16 +21,18 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 XPCOMUtils.defineLazyModuleGetter(this, "Locale",
                                   "resource://gre/modules/Locale.jsm");
 
 const LOCAL_NEWTAB_URL = "chrome://browser/content/newtab/newTab.xhtml";
 
 const REMOTE_NEWTAB_URL = "https://newtab.cdn.mozilla.net/" +
                               "v%VERSION%/%CHANNEL%/%LOCALE%/index.html";
 
+const ABOUT_URL = "about:newtab";
+
 // Pref that tells if remote newtab is enabled
 const PREF_REMOTE_ENABLED = "browser.newtabpage.remote";
 
 // The preference that tells whether to match the OS locale
 const PREF_MATCH_OS_LOCALE = "intl.locale.matchOS";
 
 // The preference that tells what locale the user selected
 const PREF_SELECTED_LOCALE = "general.useragent.locale";
@@ -41,100 +43,155 @@ const REMOTE_NEWTAB_VERSION = "0";
 
 function AboutNewTabService() {
   NewTabPrefsProvider.prefs.on(PREF_REMOTE_ENABLED, this._handleToggleEvent.bind(this));
 
   // trigger remote change if needed, according to pref
   this.toggleRemote(Services.prefs.getBoolPref(PREF_REMOTE_ENABLED));
 }
 
+/*
+ * A service that allows for the overriding, at runtime, of the newtab page's url.
+ * Additionally, the service manages pref state between a remote and local newtab page.
+ *
+ * There is tight coupling with browser/about/AboutRedirector.cpp.
+ *
+ * 1. Browser chrome access:
+ *
+ * When the user issues a command to open a new tab page, usually clicking a button
+ * in the browser chrome or using shortcut keys, the browser chrome code invokes the
+ * service to obtain the newtab URL. It then loads that URL in a new tab.
+ *
+ * When not overridden, the default URL emitted by the service is "about:newtab".
+ * When overridden, it returns the overriden URL.
+ *
+ * 2. Redirector Access:
+ *
+ * When the URL loaded is about:newtab, the default behavior, or when entered in the
+ * URL bar, the redirector is hit. The service is then called to return either of
+ * two URLs, a chrome or remote one, based on the browser.newtabpage.remote pref.
+ *
+ * NOTE: "about:newtab" will always result in a default newtab page, and never an overridden URL.
+ *
+ * Access patterns:
+ *
+ * The behavior is different when accessing the service via browser chrome or via redirector
+ * largely to maintain compatibility with expectations of add-on developers.
+ *
+ * Loading a chrome resource, or an about: URL in the redirector with either the
+ * LOAD_NORMAL or LOAD_REPLACE flags yield unexpected behaviors, so a roundtrip
+ * to the redirector from browser chrome is avoided.
+ */
 AboutNewTabService.prototype = {
 
-  _newTabURL: LOCAL_NEWTAB_URL,
+  _newTabURL: ABOUT_URL,
   _remoteEnabled: false,
+  _remoteURL: null,
   _overridden: false,
 
-  classID: Components.ID("{cef25b06-0ef6-4c50-a243-e69f943ef23d}"),
+  classID: Components.ID("{dfcd2adc-7867-4d3a-ba70-17501f208142}"),
   QueryInterface: XPCOMUtils.generateQI([Ci.nsIAboutNewTabService]),
   _xpcom_categories: [{
     service: true
   }],
 
   _handleToggleEvent(prefName, stateEnabled, forceState) { //jshint unused:false
-    this.toggleRemote(stateEnabled, forceState);
+    if (this.toggleRemote(stateEnabled, forceState)) {
+      Services.obs.notifyObservers(null, "newtab-url-changed", ABOUT_URL);
+    }
   },
 
   /**
-   * React to changes to the remote newtab pref. Only act
-   * if there is a change of state and if not overridden.
+   * React to changes to the remote newtab pref.
+   *
+   * If browser.newtabpage.remote is true, this will change the default URL to the
+   * remote newtab page URL. If browser.newtabpage.remote is false, the default URL
+   * will be a local chrome URL.
+   *
+   * This will only act if there is a change of state and if not overridden.
    *
    * @returns {Boolean} Returns if there has been a state change
    *
    * @param {Boolean}   stateEnabled    remote state to set to
    * @param {Boolean}   forceState      force state change
    */
   toggleRemote(stateEnabled, forceState) {
 
     if (!forceState && (this._overriden || stateEnabled === this._remoteEnabled)) {
       // exit there is no change of state
       return false;
     }
 
     if (stateEnabled) {
-      this._newTabURL = this.generateRemoteURL();
+      this._remoteURL = this.generateRemoteURL();
       NewTabPrefsProvider.prefs.on(
         PREF_SELECTED_LOCALE,
         this._updateRemoteMaybe.bind(this));
       NewTabPrefsProvider.prefs.on(
         PREF_MATCH_OS_LOCALE,
         this._updateRemoteMaybe.bind(this));
       this._remoteEnabled = true;
     } else {
-      this._newTabURL = LOCAL_NEWTAB_URL;
       NewTabPrefsProvider.prefs.off(PREF_SELECTED_LOCALE, this._updateRemoteMaybe);
       NewTabPrefsProvider.prefs.off(PREF_MATCH_OS_LOCALE, this._updateRemoteMaybe);
       this._remoteEnabled = false;
     }
+    this._newTabURL = ABOUT_URL;
     return true;
   },
 
   /*
    * Generate a default url based on locale and update channel
    */
   generateRemoteURL() {
     let releaseName = this.releaseFromUpdateChannel(UpdateUtils.UpdateChannel);
     let url = REMOTE_NEWTAB_URL
       .replace("%VERSION%", REMOTE_NEWTAB_VERSION)
       .replace("%LOCALE%", Locale.getLocale())
       .replace("%CHANNEL%", releaseName);
     return url;
   },
 
   /*
+   * Returns the default URL.
+   *
+   * This URL only depends on the browser.newtabpage.remote pref. Overriding
+   * the newtab page has no effect on the result of this function.
+   *
+   * @returns {String} the default newtab URL, remote or local depending on browser.newtabpage.remote
+   */
+  get defaultURL() {
+    if (this._remoteEnabled) {
+      return this._remoteURL;
+    }
+    return LOCAL_NEWTAB_URL;
+  },
+
+  /*
    * Updates the remote location when the page is not overriden.
    *
    * Useful when there is a dependent pref change
    */
   _updateRemoteMaybe() {
     if (!this._remoteEnabled || this._overridden) {
       return;
     }
 
     let url = this.generateRemoteURL();
-    if (url !== this._newTabURL) {
-      this._newTabURL = url;
+    if (url !== this._remoteURL) {
+      this._remoteURL = url;
       Services.obs.notifyObservers(null, "newtab-url-changed",
-        this._newTabURL);
+        this._remoteURL);
     }
   },
 
   /**
    * Returns the release name from an Update Channel name
    *
-   * @return {String} a release name based on the update channel. Defaults to nightly
+   * @returns {String} a release name based on the update channel. Defaults to nightly
    */
   releaseFromUpdateChannel(channelName) {
     return VALID_CHANNELS.has(channelName) ? channelName : "nightly";
   },
 
   get newTabURL() {
     return this._newTabURL;
   },
@@ -143,20 +200,23 @@ AboutNewTabService.prototype = {
     return REMOTE_NEWTAB_VERSION;
   },
 
   get remoteReleaseName() {
     return this.releaseFromUpdateChannel(UpdateUtils.UpdateChannel);
   },
 
   set newTabURL(aNewTabURL) {
-    if (aNewTabURL === "about:newtab") {
+    aNewTabURL = aNewTabURL.trim();
+    if (aNewTabURL === ABOUT_URL) {
       // avoid infinite redirects in case one sets the URL to about:newtab
       this.resetNewTabURL();
       return;
+    } else if (aNewTabURL === "") {
+      aNewTabURL = "about:blank";
     }
     let remoteURL = this.generateRemoteURL();
     let prefRemoteEnabled = Services.prefs.getBoolPref(PREF_REMOTE_ENABLED);
     let isResetLocal = !prefRemoteEnabled && aNewTabURL === LOCAL_NEWTAB_URL;
     let isResetRemote = prefRemoteEnabled && aNewTabURL === remoteURL;
 
     if (isResetLocal || isResetRemote) {
       if (this._overriden) {
@@ -177,14 +237,15 @@ AboutNewTabService.prototype = {
   },
 
   get remoteEnabled() {
     return this._remoteEnabled;
   },
 
   resetNewTabURL() {
     this._overridden = false;
+    this._newTabURL = ABOUT_URL;
     this.toggleRemote(Services.prefs.getBoolPref(PREF_REMOTE_ENABLED), true);
     Services.obs.notifyObservers(null, "newtab-url-changed", this._newTabURL);
   }
 };
 
 this.NSGetFactory = XPCOMUtils.generateNSGetFactory([AboutNewTabService]);
--- a/browser/components/newtab/nsIAboutNewTabService.idl
+++ b/browser/components/newtab/nsIAboutNewTabService.idl
@@ -4,26 +4,31 @@
 
 #include "nsISupports.idl"
 
 /**
  * Allows to override about:newtab to point to a different location
  * than the one specified within AboutRedirector.cpp
  */
 
-[scriptable, uuid(cef25b06-0ef6-4c50-a243-e69f943ef23d)]
+[scriptable, uuid(dfcd2adc-7867-4d3a-ba70-17501f208142)]
 interface nsIAboutNewTabService : nsISupports
 {
   /**
    * Returns the url of the resource for the newtab page if not overridden,
    * otherwise a string represenation of the new URL.
    */
   attribute ACString newTabURL;
 
   /**
+   * Returns the default URL (remote or local depending on pref)
+   */
+  attribute ACString defaultURL;
+
+  /**
    * Returns true if the default resource got overridden.
    */
   readonly attribute bool overridden;
 
   /**
    * Returns true if the default resource is remotely hosted and isn't
    * overridden
    */
--- a/browser/components/newtab/tests/browser/browser.ini
+++ b/browser/components/newtab/tests/browser/browser.ini
@@ -1,5 +1,6 @@
 [DEFAULT]
 support-files =
   dummy_page.html
 
 [browser_remotenewtab_pageloads.js]
+[browser_newtab_overrides.js]
new file mode 100644
--- /dev/null
+++ b/browser/components/newtab/tests/browser/browser_newtab_overrides.js
@@ -0,0 +1,139 @@
+/*globals
+  XPCOMUtils,
+  aboutNewTabService,
+  Services,
+  ContentTask,
+  TestUtils,
+  BrowserOpenTab,
+  registerCleanupFunction,
+  is,
+  content
+*/
+
+"use strict";
+
+let Cu = Components.utils;
+Cu.import("resource://gre/modules/Task.jsm");
+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
+Cu.import("resource://gre/modules/Services.jsm");
+Cu.import("resource://gre/modules/Preferences.jsm");
+
+XPCOMUtils.defineLazyServiceGetter(this, "aboutNewTabService",
+                                   "@mozilla.org/browser/aboutnewtab-service;1",
+                                   "nsIAboutNewTabService");
+
+registerCleanupFunction(function() {
+  Services.prefs.setBoolPref("browser.newtabpage.remote", false);
+  aboutNewTabService.resetNewTabURL();
+});
+
+/*
+ * Tests that the default newtab page is always returned when one types "about:newtab" in the URL bar,
+ * even when overridden.
+ */
+add_task(function* redirector_ignores_override() {
+  let overrides = [
+    "chrome://browser/content/downloads/contentAreaDownloadsView.xul",
+    "about:home",
+  ];
+
+  for (let overrideURL of overrides) {
+    let notificationPromise = nextChangeNotificationPromise(overrideURL, `newtab page now points to ${overrideURL}`);
+    aboutNewTabService.newTabURL = overrideURL;
+
+    yield notificationPromise;
+    Assert.ok(aboutNewTabService.overridden, "url has been overridden");
+
+    let tabOptions = {
+      gBrowser,
+      url: "about:newtab",
+    };
+
+    /*
+     * Simulate typing "about:newtab" in the url bar.
+     *
+     * Bug 1240169 - We expect the redirector to lead the user to "about:newtab", the default URL,
+     * due to invoking AboutRedirector. A user interacting with the chrome otherwise would lead
+     * to the overriding URLs.
+     */
+    yield BrowserTestUtils.withNewTab(tabOptions, function*(browser) {
+      yield ContentTask.spawn(browser, {}, function*() {
+        is(content.location.href, "about:newtab", "Got right URL");
+        is(content.document.location.href, "about:newtab", "Got right URL");
+        is(content.document.nodePrincipal,
+           Services.scriptSecurityManager.getSystemPrincipal(),
+           "nodePrincipal should match systemPrincipal");
+      });
+    });  // jshint ignore:line
+  }
+});
+
+/*
+ * Tests loading an overridden newtab page by simulating opening a newtab page from chrome
+ */
+add_task(function* override_loads_in_browser() {
+  let overrides = [
+    "chrome://browser/content/downloads/contentAreaDownloadsView.xul",
+    "about:home",
+    " about:home",
+  ];
+
+  for (let overrideURL of overrides) {
+    let notificationPromise = nextChangeNotificationPromise(overrideURL.trim(), `newtab page now points to ${overrideURL}`);
+    aboutNewTabService.newTabURL = overrideURL;
+
+    yield notificationPromise;
+    Assert.ok(aboutNewTabService.overridden, "url has been overridden");
+
+    // simulate a newtab open as a user would
+    BrowserOpenTab();  // jshint ignore:line
+
+    let browser = gBrowser.selectedBrowser;
+    yield BrowserTestUtils.browserLoaded(browser);
+
+    yield ContentTask.spawn(browser, {url: overrideURL}, function*(args) {
+      is(content.location.href, args.url.trim(), "Got right URL");
+      is(content.document.location.href, args.url.trim(), "Got right URL");
+    });  // jshint ignore:line
+    yield BrowserTestUtils.removeTab(gBrowser.selectedTab);
+  }
+});
+
+/*
+ * Tests edge cases when someone overrides the newtabpage with whitespace
+ */
+add_task(function* override_blank_loads_in_browser() {
+  let overrides = [
+    "",
+    " ",
+    "\n\t",
+    " about:blank",
+  ];
+
+  for (let overrideURL of overrides) {
+    let notificationPromise = nextChangeNotificationPromise("about:blank", "newtab page now points to about:blank");
+    aboutNewTabService.newTabURL = overrideURL;
+
+    yield notificationPromise;
+    Assert.ok(aboutNewTabService.overridden, "url has been overridden");
+
+    // simulate a newtab open as a user would
+    BrowserOpenTab();  // jshint ignore:line
+
+    let browser = gBrowser.selectedBrowser;
+    yield BrowserTestUtils.browserLoaded(browser);
+
+    yield ContentTask.spawn(browser, {}, function*() {
+      is(content.location.href, "about:blank", "Got right URL");
+      is(content.document.location.href, "about:blank", "Got right URL");
+    });  // jshint ignore:line
+    yield BrowserTestUtils.removeTab(gBrowser.selectedTab);
+  }
+});
+
+function nextChangeNotificationPromise(aNewURL, testMessage) {
+  return TestUtils.topicObserved("newtab-url-changed", function observer(aSubject, aData) {  // jshint unused:false
+      Assert.equal(aData, aNewURL, testMessage);
+      return true;
+  }.bind(this));
+}
--- a/browser/components/newtab/tests/browser/browser_remotenewtab_pageloads.js
+++ b/browser/components/newtab/tests/browser/browser_remotenewtab_pageloads.js
@@ -1,57 +1,49 @@
-/* globals XPCOMUtils, aboutNewTabService, Services */
+/* globals Cu, XPCOMUtils, TestUtils, aboutNewTabService, ContentTask, content, is */
 "use strict";
 
-let Cu = Components.utils;
 Cu.import("resource://gre/modules/Task.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
-Cu.import("resource://gre/modules/Services.jsm");
 
-XPCOMUtils.defineLazyModuleGetter(this, "RemotePageManager",
-                                  "resource://gre/modules/RemotePageManager.jsm");
 XPCOMUtils.defineLazyServiceGetter(this, "aboutNewTabService",
                                    "@mozilla.org/browser/aboutnewtab-service;1",
                                    "nsIAboutNewTabService");
 
 const TEST_URL = "https://example.com/browser/browser/components/newtab/tests/browser/dummy_page.html";
 
+/*
+ * Tests opening a newtab page with a remote URL. Simulates a newtab open from chrome
+ */
 add_task(function* open_newtab() {
   let notificationPromise = nextChangeNotificationPromise(TEST_URL, "newtab page now points to test url");
   aboutNewTabService.newTabURL = TEST_URL;
 
   yield notificationPromise;
   Assert.ok(aboutNewTabService.overridden, "url has been overridden");
 
-  let tabOptions = {
-    gBrowser,
-    url: "about:newtab",
-  };
-
-  yield BrowserTestUtils.withNewTab(tabOptions, function* (browser) {
-    Assert.equal(TEST_URL, browser.contentWindow.location, `New tab should open to ${TEST_URL}`);
-  });
-});
+  /*
+   * Simulate a newtab open as a user would.
+   *
+   * Bug 1240169 - We cannot set the URL to about:newtab because that would invoke the redirector.
+   * The redirector always yields the loading of a default newtab URL. We expect the user to use
+   * the browser UI to access overriding URLs, for istance by click on the "+" button in the tab
+   * bar, or by using the new tab shortcut key.
+   */
+  BrowserOpenTab();  // jshint ignore:line
 
-add_task(function* emptyURL() {
-  let notificationPromise = nextChangeNotificationPromise("", "newtab service now points to empty url");
-  aboutNewTabService.newTabURL = "";
-  yield notificationPromise;
+  let browser = gBrowser.selectedBrowser;
+  yield BrowserTestUtils.browserLoaded(browser);
 
-  let tabOptions = {
-    gBrowser,
-    url: "about:newtab",
-  };
-
-  yield BrowserTestUtils.withNewTab(tabOptions, function* (browser) {
-    Assert.equal("about:blank", browser.contentWindow.location, `New tab should open to ${"about:blank"}`);
+  yield ContentTask.spawn(browser, {url: TEST_URL}, function*(args) {
+    is(content.document.location.href, args.url, "document.location should match the external resource");
+    is(content.document.documentURI, args.url, "document.documentURI should match the external resource");
+    is(content.document.nodePrincipal.URI.spec, args.url, "nodePrincipal should match the external resource");
   });
+  yield BrowserTestUtils.removeTab(gBrowser.selectedTab);
 });
 
 function nextChangeNotificationPromise(aNewURL, testMessage) {
-  return new Promise(resolve => {
-    Services.obs.addObserver(function observer(aSubject, aTopic, aData) {  // jshint unused:false
-      Services.obs.removeObserver(observer, aTopic);
+  return TestUtils.topicObserved("newtab-url-changed", function observer(aSubject, aData) {  // jshint unused:false
       Assert.equal(aData, aNewURL, testMessage);
-      resolve();
-    }, "newtab-url-changed", false);
-  });
+      return true;
+  }.bind(this));
 }
--- a/browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js
+++ b/browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js
@@ -1,77 +1,114 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/
  */
 
-/* globals Services, XPCOMUtils, NewTabPrefsProvider, Preferences, aboutNewTabService */
+/* globals Services, XPCOMUtils, NewTabPrefsProvider, Preferences, aboutNewTabService, do_register_cleanup */
 
 "use strict";
 
 const {utils: Cu} = Components;
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://gre/modules/Preferences.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "NewTabPrefsProvider",
                                   "resource:///modules/NewTabPrefsProvider.jsm");
 
 XPCOMUtils.defineLazyServiceGetter(this, "aboutNewTabService",
                                    "@mozilla.org/browser/aboutnewtab-service;1",
                                    "nsIAboutNewTabService");
 
 const DEFAULT_HREF = aboutNewTabService.generateRemoteURL();
+const DEFAULT_CHROME_URL = "chrome://browser/content/newtab/newTab.xhtml";
+const DOWNLOADS_URL = "chrome://browser/content/downloads/contentAreaDownloadsView.xul";
+
+function cleanup() {
+  Services.prefs.setBoolPref("browser.newtabpage.remote", false);
+  aboutNewTabService.resetNewTabURL();
+  NewTabPrefsProvider.prefs.uninit();
+}
+
+do_register_cleanup(cleanup);
 
 /**
  * Test the overriding of the default URL
  */
-add_task(function* () {
+add_task(function* test_override_remote_disabled() {
   NewTabPrefsProvider.prefs.init();
   let notificationPromise;
   Services.prefs.setBoolPref("browser.newtabpage.remote", false);
-  let localChromeURL = "chrome://browser/content/newtab/newTab.xhtml";
 
   // tests default is the local newtab resource
-  Assert.equal(aboutNewTabService.newTabURL, localChromeURL,
-               `Default newtab URL should be ${localChromeURL}`);
+  Assert.equal(aboutNewTabService.defaultURL, DEFAULT_CHROME_URL,
+               `Default newtab URL should be ${DEFAULT_CHROME_URL}`);
 
-  // test the newtab service does not go in a circular redirect
-  aboutNewTabService.newTabURL = "about:newtab";
-  Assert.equal(aboutNewTabService.newTabURL, localChromeURL,
-               "Newtab URL avoids a circular redirect by setting to the default URL");
-
+  // override with some remote URL
   let url = "http://example.com/";
   notificationPromise = nextChangeNotificationPromise(url);
   aboutNewTabService.newTabURL = url;
   yield notificationPromise;
   Assert.ok(aboutNewTabService.overridden, "Newtab URL should be overridden");
   Assert.ok(!aboutNewTabService.remoteEnabled, "Newtab remote should not be enabled");
   Assert.equal(aboutNewTabService.newTabURL, url, "Newtab URL should be the custom URL");
 
-  notificationPromise = nextChangeNotificationPromise("chrome://browser/content/newtab/newTab.xhtml");
+  // test reset with remote disabled
+  notificationPromise = nextChangeNotificationPromise("about:newtab");
   aboutNewTabService.resetNewTabURL();
   yield notificationPromise;
   Assert.ok(!aboutNewTabService.overridden, "Newtab URL should not be overridden");
-  Assert.equal(aboutNewTabService.newTabURL, "chrome://browser/content/newtab/newTab.xhtml",
-               "Newtab URL should be the default");
+  Assert.equal(aboutNewTabService.newTabURL, "about:newtab", "Newtab URL should be the default");
 
+  // test override to a chrome URL
+  notificationPromise = nextChangeNotificationPromise(DOWNLOADS_URL);
+  aboutNewTabService.newTabURL = DOWNLOADS_URL;
+  yield notificationPromise;
+  Assert.ok(aboutNewTabService.overridden, "Newtab URL should be overridden");
+  Assert.equal(aboutNewTabService.newTabURL, DOWNLOADS_URL, "Newtab URL should be the custom URL");
+
+  cleanup();
+});
+
+add_task(function* test_override_remote_enabled() {
+  NewTabPrefsProvider.prefs.init();
+  let notificationPromise;
   // change newtab page to remote
+  notificationPromise = nextChangeNotificationPromise("about:newtab");
   Services.prefs.setBoolPref("browser.newtabpage.remote", true);
+  yield notificationPromise;
   let remoteHref = aboutNewTabService.generateRemoteURL();
-  Assert.equal(aboutNewTabService.newTabURL, remoteHref, "Newtab URL should be the default remote URL");
+  Assert.equal(aboutNewTabService.defaultURL, remoteHref, "Newtab URL should be the default remote URL");
   Assert.ok(!aboutNewTabService.overridden, "Newtab URL should not be overridden");
   Assert.ok(aboutNewTabService.remoteEnabled, "Newtab remote should be enabled");
-  NewTabPrefsProvider.prefs.uninit();
+
+  // change to local newtab page while remote is enabled
+  notificationPromise = nextChangeNotificationPromise(DEFAULT_CHROME_URL);
+  aboutNewTabService.newTabURL = DEFAULT_CHROME_URL;
+  yield notificationPromise;
+  Assert.equal(aboutNewTabService.newTabURL, DEFAULT_CHROME_URL,
+               "Newtab URL set to chrome url");
+  Assert.equal(aboutNewTabService.defaultURL, DEFAULT_CHROME_URL,
+               "Newtab URL defaultURL set to the default chrome URL");
+  Assert.ok(aboutNewTabService.overridden, "Newtab URL should be overridden");
+  Assert.ok(!aboutNewTabService.remoteEnabled, "Newtab remote should not be enabled");
+
+  cleanup();
 });
 
 /**
  * Tests reponse to updates to prefs
  */
 add_task(function* test_updates() {
+  /*
+   * Simulates a "cold-boot" situation, with some pref already set before testing a series
+   * of changes.
+   */
   Preferences.set("browser.newtabpage.remote", true);
+  aboutNewTabService.resetNewTabURL(); // need to set manually because pref notifs are off
   let notificationPromise;
   let expectedHref = "https://newtab.cdn.mozilla.net" +
                      `/v${aboutNewTabService.remoteVersion}` +
                      `/${aboutNewTabService.remoteReleaseName}` +
                      "/en-GB" +
                      "/index.html";
   Preferences.set("intl.locale.matchOS", true);
   Preferences.set("general.useragent.locale", "en-GB");
@@ -93,37 +130,38 @@ add_task(function* test_updates() {
   let testURL = "https://example.com/";
   notificationPromise = nextChangeNotificationPromise(
     testURL, "a notification occurs on override");
   aboutNewTabService.newTabURL = testURL;
   yield notificationPromise;
 
   // from overridden to default
   notificationPromise = nextChangeNotificationPromise(
-    DEFAULT_HREF, "a notification occurs on reset");
+    "about:newtab", "a notification occurs on reset");
   aboutNewTabService.resetNewTabURL();
   Assert.ok(aboutNewTabService.remoteEnabled, "Newtab remote should be enabled");
+  Assert.equal(aboutNewTabService.defaultURL, DEFAULT_HREF, "Default URL should be the remote page");
   yield notificationPromise;
 
   // override to default URL from default URL
   notificationPromise = nextChangeNotificationPromise(
     testURL, "a notification only occurs for a change in overridden urls");
   aboutNewTabService.newTabURL = aboutNewTabService.generateRemoteURL();
   Assert.ok(aboutNewTabService.remoteEnabled, "Newtab remote should be enabled");
   aboutNewTabService.newTabURL = testURL;
+  yield notificationPromise;
   Assert.ok(!aboutNewTabService.remoteEnabled, "Newtab remote should not be enabled");
-  yield notificationPromise;
 
   // reset twice, only one notification for default URL
   notificationPromise = nextChangeNotificationPromise(
-    DEFAULT_HREF, "reset occurs");
+    "about:newtab", "reset occurs");
   aboutNewTabService.resetNewTabURL();
   yield notificationPromise;
 
-  NewTabPrefsProvider.prefs.uninit();
+  cleanup();
 });
 
 /**
  * Verifies that releaseFromUpdateChannel
  * Returns the correct release names
  */
 add_task(function* test_release_names() {
   let valid_channels = ["esr", "release", "beta", "aurora", "nightly"];
--- a/browser/components/newtab/tests/xpcshell/test_NewTabURL.js
+++ b/browser/components/newtab/tests/xpcshell/test_NewTabURL.js
@@ -1,26 +1,26 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/
  */
 
-/* globals Services, NewTabURL, XPCOMUtils, aboutNewTabService */
+/* globals Services, NewTabURL, XPCOMUtils, aboutNewTabService, NewTabPrefsProvider */
 "use strict";
 
 const {utils: Cu} = Components;
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource:///modules/NewTabURL.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "NewTabPrefsProvider",
                                   "resource:///modules/NewTabPrefsProvider.jsm");
 XPCOMUtils.defineLazyServiceGetter(this, "aboutNewTabService",
                                    "@mozilla.org/browser/aboutnewtab-service;1",
                                    "nsIAboutNewTabService");
 
-add_task(function* () {
+add_task(function*() {
   let defaultURL = aboutNewTabService.newTabURL;
   Services.prefs.setBoolPref("browser.newtabpage.remote", false);
 
   Assert.equal(NewTabURL.get(), defaultURL, `Default newtab URL should be ${defaultURL}`);
   let url = "http://example.com/";
   let notificationPromise = promiseNewtabURLNotification(url);
   NewTabURL.override(url);
   yield notificationPromise;
@@ -31,18 +31,17 @@ add_task(function* () {
   NewTabURL.reset();
   yield notificationPromise;
   Assert.ok(!NewTabURL.overridden, "Newtab URL should not be overridden");
   Assert.equal(NewTabURL.get(), defaultURL, "Newtab URL should be the default");
 
   // change newtab page to remote
   NewTabPrefsProvider.prefs.init();
   Services.prefs.setBoolPref("browser.newtabpage.remote", true);
-  let remoteURL = aboutNewTabService.generateRemoteURL();
-  Assert.equal(NewTabURL.get(), remoteURL, `Newtab URL should be ${remoteURL}`);
+  Assert.equal(NewTabURL.get(), "about:newtab", `Newtab URL should be about:newtab`);
   Assert.ok(!NewTabURL.overridden, "Newtab URL should not be overridden");
   NewTabPrefsProvider.prefs.uninit();
 });
 
 function promiseNewtabURLNotification(aNewURL) {
   return new Promise(resolve => {
     Services.obs.addObserver(function observer(aSubject, aTopic, aData) { // jshint ignore:line
       Services.obs.removeObserver(observer, aTopic);