1218996 - Replace usage of content frame with a direct load from AboutRedirector

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect, P1)

Description

[Olivier Yiptong [:oyiptong]]

Removing the content frame allows for direct access to the privileged context, diminishing the amount of code we have in RemoteAboutNewTab and of course getting rid of browser/base/content/remote-newtab

Comment 1

[Olivier Yiptong [:oyiptong]]

Bobby, you've previously mentioned your preference for loading the remote newtab page in a content process that doesn't have chrome privileges. Bug 1204983 allows us to load remote urls from aboutRedirector.

As I got into implementing this bug, MattN made the correct remark that loading a remote url instead of about:newtab would break a lot of code.

This is because we'd be loading the URL with the LOAD_REPLACE flag.

As implemented, this will also cause the URL to show up in the url bar, because the code that hides the url doesn't get to trigger (https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2418)

The path of least resistance would be to preserve the privileged page -> remote-iframe setup for now, with communication via API instead of RemotePageManager.

Should we load the page directly (sans-iframe), here are a couple of solutions:

* load the page with LOAD_REPLACE. Broken functionality to fix all over the place
* load the page with LOAD_NORMAL. However, the document's location then becomes about:newtab, which isn't truthful. However, the page seems to have a domain principal. This can further be enforced with an AboutRedirector flag URI_SAFE_FOR_UNTRUSTED_CONTENT. Other resources are specified via absolute, remote urls.


What do you think of the direct loading approaches above?

What's your opinion on loading the page in an iframe (assuming RPM goes away in favor of API's)?
What are the privilege escalation risks with the iframe approach?

Comment 2

[Bobby Holley (:bholley)]

(In reply to Olivier Yiptong [:oyiptong] from comment #1)
> Bobby, you've previously mentioned your preference for loading the remote
> newtab page in a content process that doesn't have chrome privileges. Bug
> 1204983 allows us to load remote urls from aboutRedirector.
> 
> As I got into implementing this bug, MattN made the correct remark that
> loading a remote url instead of about:newtab would break a lot of code.
> 
> This is because we'd be loading the URL with the LOAD_REPLACE flag.
> 
> As implemented, this will also cause the URL to show up in the url bar,
> because the code that hides the url doesn't get to trigger
> (https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> js#2418)

This was part of the plan from the start, right? Doing it the way we did it involves fixing the front-end code to detect the CDN page, rather than 'about:newtab'.

> The path of least resistance would be to preserve the privileged page ->
> remote-iframe setup for now, with communication via API instead of
> RemotePageManager.

Per previous discussion, that isn't acceptable.

> Should we load the page directly (sans-iframe), here are a couple of
> solutions:
> 
> * load the page with LOAD_REPLACE. Broken functionality to fix all over the
> place

This was always my understanding of the plan. How much broken functionality is there to fix? Don't we just need to replace the hard-coded 'about:newtab' string comparison with an isAboutNewTab() function that checks for the CDN?

> * load the page with LOAD_NORMAL. However, the document's location then
> becomes about:newtab, which isn't truthful. However, the page seems to have
> a domain principal. This can further be enforced with an AboutRedirector
> flag URI_SAFE_FOR_UNTRUSTED_CONTENT. Other resources are specified via
> absolute, remote urls.

I don't think we want this.

> What's your opinion on loading the page in an iframe (assuming RPM goes away
> in favor of API's)?
> What are the privilege escalation risks with the iframe approach?

It allows untrusted content to get a reference to a privileged window (via window.parent or window.top), which is an escalation vector that can be combined with an XSS to achieve arbitrary code execution.

Comment 3

[Olivier Yiptong [:oyiptong]]

(In reply to Bobby Holley (busy) from comment #2)
> This was part of the plan from the start, right? Doing it the way we did it
> involves fixing the front-end code to detect the CDN page, rather than
> 'about:newtab'.

The plan was more abstract than that. This bug is about implementing it.

> > Should we load the page directly (sans-iframe), here are a couple of
> > solutions:
> > 
> > * load the page with LOAD_REPLACE. Broken functionality to fix all over the
> > place
> 
> This was always my understanding of the plan. How much broken functionality
> is there to fix? Don't we just need to replace the hard-coded 'about:newtab'
> string comparison with an isAboutNewTab() function that checks for the CDN?

Yes, we'll have to implement that check and replace all parts of the front-end that checks for the 'about:newtab' url.

https://dxr.mozilla.org/mozilla-central/search?q=about%3Anewtab&redirect=true&case=false

> > What's your opinion on loading the page in an iframe (assuming RPM goes away
> > in favor of API's)?
> > What are the privilege escalation risks with the iframe approach?
> 
> It allows untrusted content to get a reference to a privileged window (via
> window.parent or window.top), which is an escalation vector that can be
> combined with an XSS to achieve arbitrary code execution.

Doesn't that mean that current remote pages are at risk too?
about:healthreport, about:accounts, uitour etc?

Comment 4

[Bobby Holley (:bholley)]

(In reply to Olivier Yiptong [:oyiptong] from comment #3)
> The plan was more abstract than that. This bug is about implementing it.

Yes, but I'm saying that this was my understanding of the design plan in the light of everything that has been done on the platform end thus far.

> Yes, we'll have to implement that check and replace all parts of the
> front-end that checks for the 'about:newtab' url.

Seems reasonable.

> > > What's your opinion on loading the page in an iframe (assuming RPM goes away
> > > in favor of API's)?
> > > What are the privilege escalation risks with the iframe approach?
> > 
> > It allows untrusted content to get a reference to a privileged window (via
> > window.parent or window.top), which is an escalation vector that can be
> > combined with an XSS to achieve arbitrary code execution.
> 
> Doesn't that mean that current remote pages are at risk too?
> about:healthreport, about:accounts, uitour etc?

Those pages are all chrome-privileged, right? My understanding was that we don't currently have any untrusted pages under this setup, which is why this case requires extra care.

Comment 5

[Olivier Yiptong [:oyiptong]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=17eede8746ce

Comment 6

[Olivier Yiptong [:oyiptong]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a79a5ad2d3ef

Comment 7

[Olivier Yiptong [:oyiptong]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0bca81876c3

Comment 8

[Olivier Yiptong [:oyiptong]]

Attachment: MozReview Request: Bug 1218996 - AboutRedirector uses aboutNewTabService exclusively for about:newtab r?mconley

Bug 1218996 - AboutRedirector uses aboutNewTabService exclusively for about:newtab

Comment 9

[Olivier Yiptong [:oyiptong]]

Attachment: MozReview Request: Bug 1218996 - Fuse aboutNewTabService and RemoteNewTabLocation and return resource URLS r?marcosc

Bug 1218996 - Fuse aboutNewTabService and RemoteNewTabLocation and return resource URLS

Comment 10

[Olivier Yiptong [:oyiptong]]

Attachment: MozReview Request: Bug 1218996 - Remove now redundant messaging code for remote newtab r?marcosc

Bug 1218996 - Remove now redundant messaging code for remote newtab

Comment 11

[Olivier Yiptong [:oyiptong]]

Attachment: MozReview Request: Bug 1218996 - Allow Remote New Tab to ride release trains r?marcosc

Bug 1218996 - Allow Remote New Tab to ride release trains

Comment 12

[Chris Karlof [:ckarlof]]

about:healthreport and about:accounts currently load remote documents into iframes and communicate with that content via postMessage and events.

Comment 13

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696000 [details]
MozReview Request: Bug 1218996 - AboutRedirector uses aboutNewTabService exclusively for about:newtab r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27245/diff/1-2/

Comment 14

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696001 [details]
MozReview Request: Bug 1218996 - Fuse aboutNewTabService and RemoteNewTabLocation and return resource URLS r?marcosc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27247/diff/1-2/

Comment 15

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696002 [details]
MozReview Request: Bug 1218996 - Remove now redundant messaging code for remote newtab r?marcosc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27249/diff/1-2/

Comment 16

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696003 [details]
MozReview Request: Bug 1218996 - Allow Remote New Tab to ride release trains r?marcosc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27251/diff/1-2/

Comment 17

[Marcos Caceres [:marcosc]]

Comment on attachment 8696001 [details]
MozReview Request: Bug 1218996 - Fuse aboutNewTabService and RemoteNewTabLocation and return resource URLS r?marcosc

https://reviewboard.mozilla.org/r/27247/#review24855

::: browser/base/content/browser.js:60
(Diff revision 2)
> +                                   "nsIAboutNewTabService");

Maybe we should keep the name of hte imported service the same as the bound variable (i.e., "nsIAboutNewTabService"). Using "gWhatever" should only be used for truly global things.

::: browser/base/content/browser.js:2369
(Diff revision 2)
> +    if (gInitialPages.indexOf(uri.spec) !== -1 || (gAboutNewTabService.remoteEnabled && uri.spec === gAboutNewTabService.newTabURL))

Nit: line length. Maybe split into a two variables.

::: browser/base/content/browser.js:2369
(Diff revision 2)
> +    if (gInitialPages.indexOf(uri.spec) !== -1 || (gAboutNewTabService.remoteEnabled && uri.spec === gAboutNewTabService.newTabURL))

Nit: line length. Maybe split into multiple variables.

Also, more clear than using index check:
!gInitialPages.includes(uri.spec)

::: browser/base/content/test/general/browser_bug763468_perwindowpb.js:7
(Diff revision 2)
> +/* globals is */

Nit: "is" can move up to the previous line.

::: browser/components/BrowserComponents.manifest
(Diff revision 2)
> -component {97eea4bb-db50-4ae0-9147-1e5ed55b4ed5} nsBrowserGlue.js

Removing browser glue?

::: browser/components/newtab/RemoteAboutNewTab.jsm:37
(Diff revision 2)
> +  "nsIAboutNewTabService");

I'm a bit confused. nsI\* are supposed to be interfaces implemented by things, but here you are using it as a component (i.e., like a JSM). What's the rationale?

::: browser/components/newtab/aboutNewTabService.js:41
(Diff revision 2)
> +const NEWTAB_CHANGE_NOTIF = "newtab-url-changed";

you can probably just use the string everywhere instead. The const has the same meaning and is the same length as the string it is aliasing.

::: browser/components/newtab/aboutNewTabService.js:44
(Diff revision 2)
> +  this._newTabURL = LOCAL_NEWTAB_URL;

This could be initialized on the prototype instead.

::: browser/components/newtab/aboutNewTabService.js:45
(Diff revision 2)
> +  NewTabPrefsProvider.prefs.on(PREF_REMOTE_ENABLED, this._toggleRemote.bind(this));

I think you should make a public interface for this handler. It doesn't make sense to have a private handler that returns a value for an event handler. 

It will also make the thing testable without needing to use the private interface.

::: browser/components/newtab/aboutNewTabService.js:46
(Diff revision 2)
> +  this._remoteEnabled = false;
> +  this._overridden = false;

These two could be initialized on the prototype.

::: browser/components/newtab/aboutNewTabService.js:70
(Diff revision 2)
> +  _toggleRemote(prefName, stateEnabled, forceState) { // jshint unused:false

As above, having the public inteface vs the private interface would mean not needing to use "null" everywhere when calling this.

::: browser/components/newtab/aboutNewTabService.js:99
(Diff revision 2)
> +    let releaseName = this.releaseFromUpdateChannel(UpdateUtils.UpdateChannel);

Q: maybe this could be cached?... if Update.UpdateChannel has changed, regenerate this? 

Feel free to ignore this... it's a bit of an optimization but likely not needed.

::: browser/components/newtab/aboutNewTabService.js:113
(Diff revision 2)
> +    if (this._remoteEnabled && !this._overridden) {

Nit: try to avoid nesting code. Just return straight away instead.

::: browser/components/newtab/aboutNewTabService.js:129
(Diff revision 2)
> +    let result = "nightly";

Maybe just: 

```
return VALID_CHANNELS.get(channelName) || "nightly";
```

You are welcome :)

::: browser/components/newtab/aboutNewTabService.js:151
(Diff revision 2)
> +    if ((!prefRemoteEnabled && aNewTabURL === LOCAL_NEWTAB_URL) ||

Nit: declare variables for checks?

::: browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js:66
(Diff revision 2)
> +                     `/v${aboutNewTabService.remoteVersion}` +

Nit: maybe stick to 2 space indent.

Comment 18

[Marcos Caceres [:marcosc]]

Comment on attachment 8696003 [details]
MozReview Request: Bug 1218996 - Allow Remote New Tab to ride release trains r?marcosc

https://reviewboard.mozilla.org/r/27251/#review24851

::: browser/base/content/browser.js:3501
(Diff revision 2)
> +             ((!newTabRemoted && NewTabUtils.allPages.enabled) ||

Can we make these declarations into variables? Like:

```JS
let isNotRemotedButEnabled = ...
```

It makes the code a bit easier to understand.

::: browser/components/nsBrowserGlue.js:181
(Diff revision 2)
> +XPCOMUtils.defineLazyModuleGetter(this, "AppConstants",

Nit: maybe move this back? cleaner history

Comment 19

[Marcos Caceres [:marcosc]]

Comment on attachment 8696000 [details]
MozReview Request: Bug 1218996 - AboutRedirector uses aboutNewTabService exclusively for about:newtab r?mconley

https://reviewboard.mozilla.org/r/27245/#review24859

::: browser/components/about/AboutRedirector.cpp:90
(Diff revision 2)
> +  { "newtab", "",

Maybe we can ask someone that knows C++ to review this? I have no idea what I'm doing :_(

Comment 20

[Olivier Yiptong [:oyiptong]]

https://reviewboard.mozilla.org/r/27247/#review24855

> Removing browser glue?

No, it removes the component from the manifest. nsBrowserGlue is the file the component is found in

Comment 21

[Olivier Yiptong [:oyiptong]]

https://reviewboard.mozilla.org/r/27247/#review24869

::: browser/base/content/browser.js:60
(Diff revision 2)
> +                                   "nsIAboutNewTabService");

For services, it seems that this convention does not hold:
https://dxr.mozilla.org/mozilla-central/search?q=defineLazyServiceGetter&redirect=false&case=false

Maybe your suggestion is a trend we should make current?

::: browser/base/content/test/general/browser_bug763468_perwindowpb.js:7
(Diff revision 2)
> +/* globals is */

My line length linter rule complains

::: browser/components/newtab/RemoteAboutNewTab.jsm:37
(Diff revision 2)
> +  "nsIAboutNewTabService");

Carrying over implementation from bug 1204983. Bobby felt that a JSM would be an unnecessary use of memory

::: browser/components/newtab/aboutNewTabService.js:99
(Diff revision 2)
> +    let releaseName = this.releaseFromUpdateChannel(UpdateUtils.UpdateChannel);

This generates the URL and its results are cached. See line 114

::: browser/components/newtab/aboutNewTabService.js:129
(Diff revision 2)
> +    let result = "nightly";

Set has no `get`

Comment 22

[Olivier Yiptong [:oyiptong]]

https://reviewboard.mozilla.org/r/27247/#review24855

> I'm a bit confused. nsI\* are supposed to be interfaces implemented by things, but here you are using it as a component (i.e., like a JSM). What's the rationale?

The interface is used to implement a js XPCOM component. The component is used both in C++ and in Javascript.

> Nit: maybe stick to 2 space indent.

Following indentation rules such as: https://developer.mozilla.org/en-US/docs/User%3AGavinSharp_JS_Style_Guidelines#Spaces.2C_wrapping.2C_and_indentation

Also shared by python.

Comment 23

[Olivier Yiptong [:oyiptong]]

https://reviewboard.mozilla.org/r/27247/#review24869

> Set has no `get`

Still, good comment. Replacing with:

```
return VALID_CHANNELS.has(channelName) ? channelName : "nightly";
```

Comment 24

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696000 [details]
MozReview Request: Bug 1218996 - AboutRedirector uses aboutNewTabService exclusively for about:newtab r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27245/diff/2-3/

Comment 25

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696001 [details]
MozReview Request: Bug 1218996 - Fuse aboutNewTabService and RemoteNewTabLocation and return resource URLS r?marcosc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27247/diff/2-3/

Comment 26

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696002 [details]
MozReview Request: Bug 1218996 - Remove now redundant messaging code for remote newtab r?marcosc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27249/diff/2-3/

Comment 27

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696003 [details]
MozReview Request: Bug 1218996 - Allow Remote New Tab to ride release trains r?marcosc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27251/diff/2-3/

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8696000 [details]
MozReview Request: Bug 1218996 - AboutRedirector uses aboutNewTabService exclusively for about:newtab r?mconley

https://reviewboard.mozilla.org/r/27245/#review24951

::: browser/components/about/AboutRedirector.cpp:168
(Diff revision 3)
> +  nsAutoCString spec;
> +  aURI->GetSpec(spec);

Unused

::: browser/components/about/AboutRedirector.cpp:190
(Diff revision 3)
>        // fall back to the specified url in the map
>        if (url.IsEmpty()) {
>          url.AssignASCII(kRedirMap[i].url);
>        }

If the service fails, this falls back to... the empty string? Should we have a saner fallback?

Comment 29

[Olivier Yiptong [:oyiptong]]

https://reviewboard.mozilla.org/r/27245/#review24951

> Unused

Good catch

> If the service fails, this falls back to... the empty string? Should we have a saner fallback?

Replacing with about:blank

Comment 30

[Olivier Yiptong [:oyiptong]]

https://reviewboard.mozilla.org/r/27245/#review24859

> Maybe we can ask someone that knows C++ to review this? I have no idea what I'm doing :_(

Going to assign another reviewer for this

Comment 31

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696000 [details]
MozReview Request: Bug 1218996 - AboutRedirector uses aboutNewTabService exclusively for about:newtab r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27245/diff/3-4/

Comment 32

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696001 [details]
MozReview Request: Bug 1218996 - Fuse aboutNewTabService and RemoteNewTabLocation and return resource URLS r?marcosc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27247/diff/3-4/

Comment 33

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696002 [details]
MozReview Request: Bug 1218996 - Remove now redundant messaging code for remote newtab r?marcosc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27249/diff/3-4/

Comment 34

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696003 [details]
MozReview Request: Bug 1218996 - Allow Remote New Tab to ride release trains r?marcosc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27251/diff/3-4/

Comment 35

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8696000 [details]
MozReview Request: Bug 1218996 - AboutRedirector uses aboutNewTabService exclusively for about:newtab r?mconley

https://reviewboard.mozilla.org/r/27245/#review25077

I _think_ it's fine to fallback to yet another about: URL. If you wouldn't mind testing it locally, could you make the AboutNewTab service thing return the empty string and make sure that the fallback works correctly?

Assuming so, r=me. Thanks!

Comment 36

[Marcos Caceres [:marcosc]]

https://reviewboard.mozilla.org/r/27247/#review25139

Hmm... let's see if I can work out how to r+ things int this review board thing...

::: browser/base/content/test/general/browser_bug763468_perwindowpb.js:7
(Diff revision 4)
> +/* globals is */

Nit: you can have /*global */ be on multiple lines, so to avoid having to declare it twice.

::: browser/base/content/test/general/browser_bug767836_perwindowpb.js:6
(Diff revision 4)
> +/* globals is */

NIT: As above, but this one is only little.

::: browser/base/content/test/newtab/browser_newtab_external_resource.js:21
(Diff revision 4)
> +const ABOUT_NEWTAB_URI = "about:newtab";

Nit: you could move these up to avoid it looking like a change.

::: browser/components/newtab/aboutNewTabService.js:8
(Diff revision 4)
> +/* globals Locale, UpdateUtils */

nit: can probably combine with the above.

::: browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js:28
(Diff revision 4)
> +  let notificationPromise;

Nit: having to set promises up like this could mean that we have a data-race condition. You don't need to fix this now, if things change before the promise resolves then we have a serious concurrency problem :( I don't think we have a problem, but just be warned that you should be able to do, for example:


```
aboutNewTabService.resetNewTabURL();
yield nextChangeNotificationPromise("chrome://browser/content/newtab/newTab.xhtml");
```

And it should still work as expected.

Comment 37

[Marcos Caceres [:marcosc]]

https://reviewboard.mozilla.org/r/27247/#review25143

Comment 38

[Marcos Caceres [:marcosc]]

https://reviewboard.mozilla.org/r/27249/#review25145

Comment 39

[Marcos Caceres [:marcosc]]

https://reviewboard.mozilla.org/r/27251/#review25147

R+ if you fix that one super nested if statement :)

::: browser/base/content/browser.js:3574
(Diff revision 4)
> +             ((!newTabRemoted && NewTabUtils.allPages.enabled) ||

Nit: don't hate me, but would like this one fixed. quadruple nested if statement is not nice for the next programmer.

Comment 40

[Olivier Yiptong [:oyiptong]]

https://reviewboard.mozilla.org/r/27247/#review25139

> Nit: you could move these up to avoid it looking like a change.

I rather prefer the change above than:

```javascript
"use strict";

const ABOUT_NEWTAB_URI = "about:newtab";
const PREF_URI = "http://example.com/browser/browser/base/content/test/newtab/external_newtab.html";

var browser = null;
var aboutNewTabService = Cc["@mozilla.org/browser/aboutnewtab-service;1"]
                           .getService(Ci.nsIAboutNewTabService);

const DEFAULT_URI = aboutNewTabService.newTabURL;
```

I like declaring the constants together for aesthetic purposes... arguably, aboutNewTabService could be a constant too, but in this case, I didn't want to make a change.

> Nit: having to set promises up like this could mean that we have a data-race condition. You don't need to fix this now, if things change before the promise resolves then we have a serious concurrency problem :( I don't think we have a problem, but just be warned that you should be able to do, for example:
> 
> 
> ```
> aboutNewTabService.resetNewTabURL();
> yield nextChangeNotificationPromise("chrome://browser/content/newtab/newTab.xhtml");
> ```
> 
> And it should still work as expected.

Apparently, you can't trust the machinery to work as stated in your comment.
You want to create the promise first, then yield it later, especially in an e10s situation, as the state may have changed by the time you create the promise.

Comment 41

[Olivier Yiptong [:oyiptong]]

https://reviewboard.mozilla.org/r/27247/#review25139

> Apparently, you can't trust the machinery to work as stated in your comment.
> You want to create the promise first, then yield it later, especially in an e10s situation, as the state may have changed by the time you create the promise.

Ah, spoke to mconley, because that reminded me of the Search test failures. My comment about e10s was false. In fact, if `resetNewTabURL` were async, we could've made the statements as you described, prepended with a `yield` statement.

In this case, if `resetNewTabURL()` is synchronous, this test will always fail:

```javascript
aboutNewTabService.resetNewTabURL();
yield nextChangeNotificationPromise("chrome://browser/content/newtab/newTab.xhtml");
```

This is because the observer notification would have been sent when resetNewTabURL executes and nextChangeNotificationPromise will not yield.

Comment 42

[Olivier Yiptong [:oyiptong]]

https://reviewboard.mozilla.org/r/27251/#review25147

> Nit: don't hate me, but would like this one fixed. quadruple nested if statement is not nice for the next programmer.

Do you mean to make the condition easier to understand? It is still a one-level deep if statement.
I'm making it easier to read (albeit potentially a couple of additional CPU instructions on the branch)

Comment 43

[Olivier Yiptong [:oyiptong]]

https://reviewboard.mozilla.org/r/27245/#review25077

Doing one better: adding a mochitest for the fallback scenario. Thanks!

Comment 44

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696000 [details]
MozReview Request: Bug 1218996 - AboutRedirector uses aboutNewTabService exclusively for about:newtab r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27245/diff/4-5/

Comment 45

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696001 [details]
MozReview Request: Bug 1218996 - Fuse aboutNewTabService and RemoteNewTabLocation and return resource URLS r?marcosc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27247/diff/4-5/

Comment 46

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696002 [details]
MozReview Request: Bug 1218996 - Remove now redundant messaging code for remote newtab r?marcosc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27249/diff/4-5/

Comment 47

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696003 [details]
MozReview Request: Bug 1218996 - Allow Remote New Tab to ride release trains r?marcosc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27251/diff/4-5/

Comment 48

[Marcos Caceres [:marcosc]]

Comment on attachment 8696003 [details]
MozReview Request: Bug 1218996 - Allow Remote New Tab to ride release trains r?marcosc

https://reviewboard.mozilla.org/r/27251/#review25609

Comment 49

[Marcos Caceres [:marcosc]]

Comment on attachment 8696002 [details]
MozReview Request: Bug 1218996 - Remove now redundant messaging code for remote newtab r?marcosc

https://reviewboard.mozilla.org/r/27249/#review25611

Comment 50

[Marcos Caceres [:marcosc]]

Comment on attachment 8696001 [details]
MozReview Request: Bug 1218996 - Fuse aboutNewTabService and RemoteNewTabLocation and return resource URLS r?marcosc

https://reviewboard.mozilla.org/r/27247/#review25613

Comment 51

[Marcos Caceres [:marcosc]]

Comment on attachment 8696001 [details]
MozReview Request: Bug 1218996 - Fuse aboutNewTabService and RemoteNewTabLocation and return resource URLS r?marcosc

https://reviewboard.mozilla.org/r/27247/#review25711

Comment 52

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7839222071ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/9410cbc0545e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2205cce34824
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba750628c4f3

Comment 53

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

I had to back these out in https://hg.mozilla.org/integration/mozilla-inbound/rev/fa2cfb656c59 for talos failures like https://treeherder.mozilla.org/logviewer.html#?job_id=18957796&repo=mozilla-inbound

Comment 54

[Olivier Yiptong [:oyiptong]]

Youch. Thanks. My hunch is that since we're reworking how the newtab page loads, we may need to look into some of the talos assumptions.

I had a chat with Vladan. I'll take a look in the new year

Comment 56

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696000 [details]
MozReview Request: Bug 1218996 - AboutRedirector uses aboutNewTabService exclusively for about:newtab r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27245/diff/5-6/

Comment 57

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696001 [details]
MozReview Request: Bug 1218996 - Fuse aboutNewTabService and RemoteNewTabLocation and return resource URLS r?marcosc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27247/diff/5-6/

Comment 58

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696002 [details]
MozReview Request: Bug 1218996 - Remove now redundant messaging code for remote newtab r?marcosc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27249/diff/5-6/

Comment 59

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696003 [details]
MozReview Request: Bug 1218996 - Allow Remote New Tab to ride release trains r?marcosc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27251/diff/5-6/

Comment 60

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696000 [details]
MozReview Request: Bug 1218996 - AboutRedirector uses aboutNewTabService exclusively for about:newtab r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27245/diff/6-7/

Comment 61

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696001 [details]
MozReview Request: Bug 1218996 - Fuse aboutNewTabService and RemoteNewTabLocation and return resource URLS r?marcosc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27247/diff/6-7/

Comment 62

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696002 [details]
MozReview Request: Bug 1218996 - Remove now redundant messaging code for remote newtab r?marcosc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27249/diff/6-7/

Comment 63

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8696003 [details]
MozReview Request: Bug 1218996 - Allow Remote New Tab to ride release trains r?marcosc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27251/diff/6-7/

Comment 64

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c03008a67c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/311418dbd2c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/684f00c0ae6e
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e0a8cdc62ad

Comment 65

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4c03008a67c9
https://hg.mozilla.org/mozilla-central/rev/311418dbd2c5
https://hg.mozilla.org/mozilla-central/rev/684f00c0ae6e
https://hg.mozilla.org/mozilla-central/rev/8e0a8cdc62ad

Comment 66

[JustOff]

Seems like latest changes from here broke NewTabURL override behaviour. Since 44.0a1 2016-01-13 custom newtab page defined by aboutNewTabService.newTabURL no more loaded correctly.

Speed Start extension (https://addons.mozilla.org/en-US/firefox/addon/speed-start/versions/2.0.5b6) sets aboutNewTabService.newTabURL to its internal page chrome://sstart/content/sstart.html, this worked well before.

Now only blank page displayed when opening new tab (but with correct favicon and good source in View Page Source) and Page Info says that its url is "about:newtab", instead of redefined, as it was before. At the same time chrome://sstart/content/sstart.html placed directrly into urlbar continues to work correctly.

Comment 67

[Olivier Yiptong [:oyiptong]]

Taking a look

Comment 68

[Olivier Yiptong [:oyiptong]]

Will fix in bug 1240169

Comment 69

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name 			Firefox
Version 		46.0b9
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected