Closed Bug 1218996 Opened 9 years ago Closed 9 years ago

Replace usage of content frame with a direct load from AboutRedirector

Categories

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

Product:
Firefox
Component:
New Tab Page
Version:
40 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 46
Iteration:
44.3 - Nov 2
Tracking Flags:
Tracking Status
firefox46 --- fixed

People

(Reporter: oyiptong, Assigned: oyiptong)

References

Details

Attachments

(4 files)

MozReview Request: Bug 1218996 - AboutRedirector uses aboutNewTabService exclusively for about:newtab r?mconley
40 bytes, text/x-review-board-request
mconley
: review+
Details
MozReview Request: Bug 1218996 - Fuse aboutNewTabService and RemoteNewTabLocation and return resource URLS r?marcosc
40 bytes, text/x-review-board-request
marcosc
: review+
Details
MozReview Request: Bug 1218996 - Remove now redundant messaging code for remote newtab r?marcosc
40 bytes, text/x-review-board-request
marcosc
: review+
Details
MozReview Request: Bug 1218996 - Allow Remote New Tab to ride release trains r?marcosc
40 bytes, text/x-review-board-request
marcosc
: review+
Details 
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
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?
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
(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?
(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.
Bug 1218996 - AboutRedirector uses aboutNewTabService exclusively for about:newtab
Bug 1218996 - Fuse aboutNewTabService and RemoteNewTabLocation and return resource URLS
Bug 1218996 - Remove now redundant messaging code for remote newtab
about:healthreport and about:accounts currently load remote documents into iframes and communicate with that content via postMessage and events.
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/
Attachment #8696000 - Attachment description: MozReview Request: Bug 1218996 - AboutRedirector uses aboutNewTabService exclusively for about:newtab → MozReview Request: Bug 1218996 - AboutRedirector uses aboutNewTabService exclusively for about:newtab r?marcosc
Attachment #8696000 - Flags: review?(mcaceres)
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/
Attachment #8696001 - Attachment description: MozReview Request: Bug 1218996 - Fuse aboutNewTabService and RemoteNewTabLocation and return resource URLS → MozReview Request: Bug 1218996 - Fuse aboutNewTabService and RemoteNewTabLocation and return resource URLS r?marcosc
Attachment #8696001 - Flags: review?(mcaceres)
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/
Attachment #8696002 - Attachment description: MozReview Request: Bug 1218996 - Remove now redundant messaging code for remote newtab → MozReview Request: Bug 1218996 - Remove now redundant messaging code for remote newtab r?marcosc
Attachment #8696002 - Flags: review?(mcaceres)
Attachment #8696003 - Attachment description: MozReview Request: Bug 1218996 - Allow Remote New Tab to ride release trains → MozReview Request: Bug 1218996 - Allow Remote New Tab to ride release trains r?marcosc
Attachment #8696003 - Flags: review?(mcaceres)
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 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.
Attachment #8696001 - Flags: review?(mcaceres)
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
Attachment #8696003 - Flags: review?(mcaceres)
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 :_(
Attachment #8696000 - Flags: review?(mcaceres)
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
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`
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.
https://reviewboard.mozilla.org/r/27247/#review24869

> Set has no `get`

Still, good comment. Replacing with:

```
return VALID_CHANNELS.has(channelName) ? channelName : "nightly";
```
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/
Attachment #8696000 - Attachment description: MozReview Request: Bug 1218996 - AboutRedirector uses aboutNewTabService exclusively for about:newtab r?marcosc → MozReview Request: Bug 1218996 - AboutRedirector uses aboutNewTabService exclusively for about:newtab r?mconley
Attachment #8696000 - Flags: review?(mconley)
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/
Attachment #8696001 - Flags: review?(mcaceres)
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 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/
Attachment #8696003 - Flags: review?(mcaceres)
Attachment #8696000 - Flags: review?(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?
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
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 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/
Attachment #8696000 - Flags: review?(mconley)
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 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 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 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!
Attachment #8696000 - Flags: review?(mconley) → review+
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.
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.
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.
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.
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)
https://reviewboard.mozilla.org/r/27245/#review25077

Doing one better: adding a mochitest for the fallback scenario. Thanks!
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 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 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 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 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
Attachment #8696003 - Flags: review?(mcaceres) → review+
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
Attachment #8696002 - Flags: review?(mcaceres) → review+
Attachment #8696001 - Flags: review?(mcaceres)
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 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
Attachment #8696001 - Flags: review+
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
Flags: needinfo?(oyiptong)
Assignee: nobody → oyiptong
Priority: -- → P1
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 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 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 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 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 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 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 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/
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.
Taking a look
Blocks: 1240169
Will fix in bug 1240169
Blocks: 1242010
[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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: