Bug 1240169 - aboutNewTabService relies on AboutRedirector for default URL resolution r=mconley
authorOlivier Yiptong <olivier@olivieryiptong.com>
Wed, 27 Jan 2016 13:50:18 -0500
changeset 282048 1ec780c2516c6791706d28ea82558993ae7dd6e6
parent 282047 e3212c974c62fd43bb01332702b8885830d43ed6
child 282049 1de0ed56c3d4fcfab8afe4a4f748bb7c6b13f9f7
push id29950
push usercbook@mozilla.com
push dateThu, 28 Jan 2016 11:14:03 +0000
treeherdermozilla-central@2b73b0a4d52b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmconley
bugs1240169
milestone47.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
@@ -42,11 +42,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,74 +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 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* loadNewPageAndVerify(browser, uri) {
-  let browserLoadedPromise = BrowserTestUtils.waitForEvent(browser, "load", true);
-  browser.loadURI("about:newtab");
-  yield browserLoadedPromise;
-
-  yield ContentTask.spawn(gBrowser.selectedBrowser, { uri: uri }, function* (args) {
-    let uri = args.uri;
-
-    is(String(content.document.location), uri, "document.location should match " + uri);
-    is(content.document.documentURI, uri, "document.documentURI should match " + uri);
-
-    if (uri == "about:newtab") {
-      is(content.document.nodePrincipal,
-         Services.scriptSecurityManager.getSystemPrincipal(),
-         "nodePrincipal should match systemPrincipal");
-    }
-    else {
-      is(content.document.nodePrincipal.URI.spec, uri,
-         "nodePrincipal should match " + uri);
-    }
-  }, true);
-}
-
-add_task(function* () {
-  // test the default behavior
-  yield* addNewTabPageTab();
-
-  let browser = gBrowser.selectedBrowser;
-
-  ok(!aboutNewTabService.overridden,
-     "sanity check: default URL for about:newtab should not be overriden");
-
-  yield* loadNewPageAndVerify(browser, ABOUT_NEWTAB_URI);
-
-  // 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");
-
-  yield* loadNewPageAndVerify(browser, PREF_URI);
-
-  // 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();
-});
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);