Closed Bug 1240169 Opened 9 years ago Closed 9 years ago

Allow chrome URL overrides in aboutNewTabService

Categories

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

Product:
Firefox
Component:
New Tab Page
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 47
Iteration:
46.3 - Jan 25
Tracking Flags:
Tracking Status
firefox45 --- unaffected
firefox46 + fixed
firefox47 + fixed

People

(Reporter: oyiptong, Assigned: oyiptong)

References

Details

(Keywords: addon-compat, regression)

Attachments

(3 files)

MozReview Request: Bug 1240169 - aboutNewTabService relies on AboutRedirector for default URL resolution r?mconley
58 bytes, text/x-review-board-request
mconley
: review+
lizzard
: approval-mozilla-aurora+
Details
MozReview Request: Bug 1240169 - Remove redundant remote-newtab redirector interface contract r?mconley
58 bytes, text/x-review-board-request
mconley
: review+
lizzard
: approval-mozilla-aurora+
Details
MozReview Request: Bug 1240169 - Revert to returning a dynamic newtab URL for BROWSER_NEW_TAB_URL r?mconley
58 bytes, text/x-review-board-request
mconley
: review+
lizzard
: approval-mozilla-aurora+
Details 
Fixes a bug introduce in bug 1218996 where a chrome URL override does not load correctly.

This will allow addons to override the newtab page url with a chrome resource.
Assignee: nobody → oyiptong
Thanks for the bug confirmation, hope that this regression will be fixed soon.
Sorry for the typo in the original https://bugzilla.mozilla.org/show_bug.cgi?id=1218996#c66 report. The problem occured on 46.0a1 2016-01-13 nightly, not 44.0a1. I see that you have already found this at https://bugzilla.mozilla.org/show_bug.cgi?id=1240559, this note is for a final clarification.
So far, I've found that the redirector changes and the aboutNewTabService is not responsible for this regression.

The gist of it seems to be that the document loaded now thinks it is about:newtab instead of the chrome page, therefore relative links for resources won't work.

If you load your javascript resources using absolute paths, things should work fine.

I'm bisecting to figure out what patch caused the issue, should take a while, but my hunch is it's something related to docShell changes
I've tried to change all paths to javascript resources to absolute, but unfortunately this didn't helps.

As even there are no workaround, could I hope for the fix before 46.0 will be moved to aurora channel on coming Tuesday? I would be very grateful, if it be so.
Actually, I was wrong. 1218996 introduced the bug indeed. I'll have a fix shortly
And yes, I'll try to make it happen before the merge to aurora, or get it uplifted.
Ok. I have found the problem.

AboutRedirector used to load the URL directly from the redirMap. AboutRedirector reads from aboutNewTabService since bug 1204983, but only if overridden.

This codepath is triggered by typing "about:newtab" in the URL bar.

I've modified AboutRedirector to always read from aboutNewTabService and to disregard the url in the redirMap.

Another significant change I made in bug 1218996 was to make the browser chrome newtab open codepaths use the "about:newtab" redirector url, effectively unifying the destination newtab page:

https://dxr.mozilla.org/mozilla-central/rev/66e07ef46853709e3fa91e7c9ad9fe6abf0d5f06/browser/base/content/utilityOverlay.js#26

The problem is that now, the way AboutRedirector loads a resource URI is using a LOAD_NORMAL flag. This is problematic, because the docShell URL will consequently be about:newtab.

For the pages to work as they used to, the docShell URL needs to still be the resource URL. The other flag that the AboutRedirector sets non-resource URLs is LOAD_REPLACE.

That is also not a solution. LOAD_REPLACE correctly uses the resource URL instead of the original URI ("about:newtab"), however, it also unrolls the resource URL to its path on disk.

The solution is either to find an appropriate load flag, or to go back to two different code paths, each loading a potentially different URL.

I'm trying to find what solution #1 looks like.

Previously, this loaded the URL directly from NewTabURL.jsm.
err, disregard the last line. I don't know how it got here. It should end with: I'm trying to find what solution #1 looks like
It's good that you have found the cause and thanks for the detailed explanation. 

Actually, I can't understand the advantages of ignoring redirMap in AboutRedirector with the following attempt to correct url in docShell using flags. IMHO, this looks quite unnatural, even if you will be able to handle it properly.

On the other hand, any solution that leads to a correct result will undoubtedly suit me.
Before using the aboutNewTabService in AboutRedirector.cpp,

If one typed "about:newtab" in the url bar, it could yield a different URL than if the same person opened the newtab page using browser chrome codepaths (+ or cmd/ctrl+t).
It also makes for 2 codepaths to maintain. While it is more "ugly", it has been the way it's been for a while.

Either way, we have two solutions:

1. make "about:newtab" and browser chrome behaviors use the same codepath, therefore have the same behavior
2. bring back the old behavior, or something very close to it, with two codepaths. It will be the exact same for addons before bug 1218996.

If #1 proves challenging, we still have #2 to fall back on
Of course the final decision is yours, but given that new aurora/dev is coming very soon, I think that optimal way is at first bring back the old behavior to fix the regression and only then try to implement universal solution, if it's really needed.
Keywords: addon-compat
Comment on attachment 8711282 [details]
MozReview Request: Bug 1240169 - aboutNewTabService relies on AboutRedirector for default URL resolution r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32065/diff/1-2/
Comment on attachment 8711283 [details]
MozReview Request: Bug 1240169 - Remove redundant remote-newtab redirector interface contract r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32067/diff/1-2/
Comment on attachment 8711284 [details]
MozReview Request: Bug 1240169 - Revert to returning a dynamic newtab URL for BROWSER_NEW_TAB_URL r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32069/diff/1-2/
Comment on attachment 8711282 [details]
MozReview Request: Bug 1240169 - aboutNewTabService relies on AboutRedirector for default URL resolution r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32065/diff/2-3/
Comment on attachment 8711283 [details]
MozReview Request: Bug 1240169 - Remove redundant remote-newtab redirector interface contract r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32067/diff/2-3/
Comment on attachment 8711284 [details]
MozReview Request: Bug 1240169 - Revert to returning a dynamic newtab URL for BROWSER_NEW_TAB_URL r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32069/diff/2-3/
This bug seems to be partially reverting changes from bug 1218996. Does it make sense for marcosc and/or mconley to review the changes? I'll probably need to spend some time catching up on the various background/context of the original changes and these, etc.
Flags: needinfo?(mconley)
Flags: needinfo?(mcaceres)
Indeed. Unfortunately, Marcos is not available in until a couple of weeks. Perhaps :mconley could take a look?
Flags: needinfo?(mcaceres)
Flags: needinfo?(Off.Just.Off)
Yes, I can confirm that on a build above (win32 tested) chrome:// url defined by aboutNewTabService.newTabURL works correctly.
Flags: needinfo?(Off.Just.Off)
Thank you!
https://reviewboard.mozilla.org/r/32065/#review28983

So I think I need to better understand what this patch intends to do.

As you explained to me in person, it appears that there are now two branches:

1) BrowserOpenTab(), which bypasses the redirector, and will result in a user being sent to the overridden URL
2) The redirector, which we get if the user types in about:newtab and presses enter. This will send the user to the default about:newtab (or about:remote-newtab).

That feels odd and unintuitive.

Can you please summarize why we don't want the AboutRedirector to behave in the same way as BrowserOpenTab? I realize that you already did this in person, but I'd really like to read it so that I can internalize the justification properly.

::: browser/base/content/test/newtab/browser.ini
(Diff revision 3)
> -[browser_newtab_external_resource.js]
> -support-files =
> -  external_newtab.html

Can you summarize why this test needs to be removed?

::: browser/components/newtab/NewTabComponents.manifest:1
(Diff revision 3)
> -component {cef25b06-0ef6-4c50-a243-e69f943ef23d} aboutNewTabService.js
> +component {9ec717d8-1561-4977-ae54-a225281a753f} aboutNewTabService.js

Not necessary - you don't need to bump the class uuid when you modify it.

::: browser/components/newtab/aboutNewTabService.js:84
(Diff revision 3)
> -  classID: Components.ID("{cef25b06-0ef6-4c50-a243-e69f943ef23d}"),
> +  classID: Components.ID("{9ec717d8-1561-4977-ae54-a225281a753f}"),

It's not necessary to bump the class ID when you change it.

::: browser/components/newtab/tests/browser/browser_newtab_overrides.js:29
(Diff revision 3)
> +add_task(function* redirector_ignores_override() {

Please summarize what this test is testing in a comment block over add_task.

Same goes with the rest of the add_tasks in here.
Let me put in my two cents. Alongside with elimination of the regression that has emerged because of the implementation of bug 1218996, the different processing of BrowserOpenTab() and "about:newtab" has its own sense. This allows to access the native browser new tab page, even if it was redefined by aboutNewTabService.newTabURL. For example, the addon which replace native new tab can still have link to it. At the same time, a common person has ever seen "about:newtab" in the location bar, so it most likely will never misled by typing "about:newtab" there.
https://reviewboard.mozilla.org/r/32065/#review28983

The code, prior to bug 1218996, did have 2 branches:

Using aboutNewTabService: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js?rev=e7f7e3e691d1#23
And before aboutNewTabService, using NewTabURL.jsm: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js?rev=9eb07902468e#21

The behavior was that when the newtab URL was not overriden, the newtab service/jsm would return "about:newtab", which would then trigger the redirector.
When the newtab URL was overriden, the newtab service/jsm would return the overriding URL.

I wanted to  simplify the flow of instructions to the browser by having any requests funneled through the redirector, so that there would be only one entrypoint into loading a newtab page. Bug 1218996 attempted to unify those branches by always returning "about:newtab" and letting the overriding happen in AboutRedirector.

That works fine for our purposes: our local newtab page would be identified as a resource and loaded with a LOAD_NORMAL loadflag, with the document keeping the original "about:newtab" url.
A remote page would be loaded with LOAD_REPLACE, which replaces the original url with the url given by aboutNewTabService.

However, this had the unintentional consequence of breaking the expectation of addon developers who package replacements for the newtab page as resources.
With the behavior of AboutRedirector in bug 1218996, the pages would load with with a document url of "about:newtab", which breaks a lot of things.
They expect the document to load with their own chome:// url.

The way I saw it, there were two possible solutions to this:

1. Control the channel creation, loading with a LOAD_REPLACE loadflag, so that a request to "about:newtab" would yield the user "chrome://..."
2. Revert to the original situation of having 2 possible entrypoints to the newtab page

#1 proved challenging given how chrome URL resolution works. chrome urls, when resolved, yield file:// urls. There could potentially be some things we could try, but I asked bsmedberg and his opinion was that there was no easy way to fix that problem.
So, this patch does #2: revert to the original situation, with the caveat that the "default" URL is no longer decided by AboutRedirector's redirMap's, but by aboutNewTabService.

Another consequence of this patch is that typing "about:newtab" in the URL bar will yield the default newtab page, which is the behavior prior to 1218996 as well.

> Can you summarize why this test needs to be removed?

The things that are tested in this file are tested in browser/components/newtab/tests/browser/browser_remotenewtab_pageloads.js and /browser/components/newtab/tests/browser/browser_newtab_overrides.js, namely:

* loading a local newtab page and checking the document's location and principal
* loading a remote newtab page and checking the document's location and principal

Another part of the motivation to replace was to fix soon-to-be broken windows. The two files whose tests replace browser_newtab_external_resource.js, don't use soon to be deprecated CPOWs, and instead use ContentTask.jsm.
Okay, I believe I understand - thanks for clearing that up. I'll continue the review now.
Flags: needinfo?(mconley)
Attachment #8711282 - Attachment description: MozReview Request: Bug 1240169 - aboutNewTabService relies on AboutRedirector for default URL resolution r?Mardak → MozReview Request: Bug 1240169 - aboutNewTabService relies on AboutRedirector for default URL resolution r?mconley
Attachment #8711282 - Flags: review?(edilee) → review?(mconley)
Comment on attachment 8711282 [details]
MozReview Request: Bug 1240169 - aboutNewTabService relies on AboutRedirector for default URL resolution r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32065/diff/3-4/
Comment on attachment 8711283 [details]
MozReview Request: Bug 1240169 - Remove redundant remote-newtab redirector interface contract r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32067/diff/3-4/
Attachment #8711283 - Attachment description: MozReview Request: Bug 1240169 - Remove redundant remote-newtab redirector interface contract r?Mardak → MozReview Request: Bug 1240169 - Remove redundant remote-newtab redirector interface contract r?mconley
Attachment #8711283 - Flags: review?(edilee) → review?(mconley)
Attachment #8711284 - Attachment description: MozReview Request: Bug 1240169 - Revert to returning a dynamic newtab URL for BROWSER_NEW_TAB_URL r?Mardak → MozReview Request: Bug 1240169 - Revert to returning a dynamic newtab URL for BROWSER_NEW_TAB_URL r?mconley
Attachment #8711284 - Flags: review?(edilee) → review?(mconley)
Comment on attachment 8711284 [details]
MozReview Request: Bug 1240169 - Revert to returning a dynamic newtab URL for BROWSER_NEW_TAB_URL r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32069/diff/3-4/
Comment on attachment 8711282 [details]
MozReview Request: Bug 1240169 - aboutNewTabService relies on AboutRedirector for default URL resolution r?mconley

https://reviewboard.mozilla.org/r/32065/#review29023

::: browser/components/newtab/aboutNewTabService.js:55
(Diff revision 3)
> + * 1. Browser chrome access:

"Browser chrome access" doesn't really tell us much. Essentially, I think what you should be saying here, is that the browser front-end should be reading newTabURL when the user issues the command to open a new tab.

::: browser/components/newtab/aboutNewTabService.js:92
(Diff revision 3)
> +      Services.obs.notifyObservers(null, "newtab-url-changed", "about:newtab");

Again, best to use a constant here instead of having about:newtab sprinkled all over.

::: browser/components/newtab/aboutNewTabService.js:97
(Diff revision 3)
>     * React to changes to the remote newtab pref. Only act
>     * if there is a change of state and if not overridden.

Please update the documentation here to say that this will undo any overrides.

::: browser/components/newtab/aboutNewTabService.js:126
(Diff revision 3)
> +    this._newTabURL = "about:newtab";

Probably best to stash this as a const somewhere instead of using the literal here and on lines 79, 193, and 229.

::: browser/components/newtab/aboutNewTabService.js:143
(Diff revision 3)
> +   * Returns the default URL (remote or local depending on pref)

We should probably flesh this out saying that the defaultURL is not affected by add-ons overriding the URL.

::: browser/components/newtab/tests/browser/browser_newtab_overrides.js:14
(Diff revision 3)
> +let Cu = Components.utils;

This should already be available to you, so you don't need to redefine it.

::: browser/components/newtab/tests/browser/browser_newtab_overrides.js:47
(Diff revision 3)
> +    // simulate typing "about:newtab" in the url bar

Please put a note in here that we expect about:newtab to actually go to about:newtab, due to the two-branch system we've got here.

::: browser/components/newtab/tests/browser/browser_newtab_overrides.js:83
(Diff revision 3)
> +    gBrowser.removeTab(gBrowser.selectedTab);

```JavaScript
yield BrowserTestUtils.removeTab(gBrowser.selectedTab);
```

::: browser/components/newtab/tests/browser/browser_newtab_overrides.js:111
(Diff revision 3)
> +    gBrowser.removeTab(gBrowser.selectedTab);

```JavaScript
yield BrowserTestUtils.removeTab(gBrowser.selectedTab);
```

::: browser/components/newtab/tests/browser/browser_remotenewtab_pageloads.js:22
(Diff revision 3)
> -  let tabOptions = {
> +  // simulate a newtab open as a user would

I think this comment could be fleshed out a little - perhaps something like,

```JavaScript
// We can't just open about:newtab, since that won't
// give newtab overrides a chance. Instead, we call
// BrowserOpenTab, which will query for overrides
// before opening the new tab.
```

::: browser/components/newtab/tests/browser/browser_remotenewtab_pageloads.js:33
(Diff revision 3)
> +  gBrowser.removeTab(gBrowser.selectedTab);

```JavaScript
yield BrowserTestUtils.removeTab(gBrowser.selectedTab);
```

This does the job of making sure the closed tab has fully shut down.

::: browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js:45
(Diff revision 3)
>    aboutNewTabService.newTabURL = "about:newtab";
> -  Assert.equal(aboutNewTabService.newTabURL, localChromeURL,
> -               "Newtab URL avoids a circular redirect by setting to the default URL");
> +  Assert.equal(aboutNewTabService.newTabURL, "about:newtab",
> +               "Setting the newTabURL to about:newtab doesn't change anything");

I'm not sure this is testing anything useful at this point...

::: browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js:49
(Diff revision 3)
> +  // override with random remote URL

It's not technically random, so, perhaps just:

```JavaScript
// override with some remote URL
```

::: browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js:106
(Diff revision 3)
> +  aboutNewTabService.resetNewTabURL(); // need to set manually because pref notifs are off

Didn't the last test do this already in the cleanup function?
Attachment #8711282 - Flags: review?(mconley)
Attachment #8711283 - Flags: review?(mconley) → review+
Comment on attachment 8711283 [details]
MozReview Request: Bug 1240169 - Remove redundant remote-newtab redirector interface contract r?mconley

https://reviewboard.mozilla.org/r/32067/#review29047

::: browser/components/build/nsModule.cpp
(Diff revision 4)
> -    { NS_ABOUT_MODULE_CONTRACTID_PREFIX "remote-newtab", &kNS_BROWSER_ABOUT_REDIRECTOR_CID },

To be explicit, the idea is that as soon as you flip the remote newtab pref, about:newtab redirects to the remote page, and about:remote-newtab is no longer a thing?
Comment on attachment 8711284 [details]
MozReview Request: Bug 1240169 - Revert to returning a dynamic newtab URL for BROWSER_NEW_TAB_URL r?mconley

https://reviewboard.mozilla.org/r/32069/#review29049
Attachment #8711284 - Flags: review?(mconley) → review+
https://reviewboard.mozilla.org/r/32067/#review29047

> To be explicit, the idea is that as soon as you flip the remote newtab pref, about:newtab redirects to the remote page, and about:remote-newtab is no longer a thing?

That is correct. As we changed from loading the remote newtab in an iframe to loading a fully unprivileged page, the distinction isn't necessary anymore
https://reviewboard.mozilla.org/r/32065/#review29023

> I'm not sure this is testing anything useful at this point...

You are correct!

> Didn't the last test do this already in the cleanup function?

This is necessary. We rely on NewTabPrefsProvider to be initted to listen to pref changes and to cache the remote URL.
Since this simulates a cold-boot with prefs already on before we initiate changes, we use resetNewTabURL and other pref changes to test updates.
https://reviewboard.mozilla.org/r/32065/#review29023

> This is necessary. We rely on NewTabPrefsProvider to be initted to listen to pref changes and to cache the remote URL.
> Since this simulates a cold-boot with prefs already on before we initiate changes, we use resetNewTabURL and other pref changes to test updates.

I meant, we use resetNewTabURL and other prefs in this situation to set the internal state.
Attachment #8711282 - Flags: review?(mconley)
Comment on attachment 8711282 [details]
MozReview Request: Bug 1240169 - aboutNewTabService relies on AboutRedirector for default URL resolution r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32065/diff/4-5/
Comment on attachment 8711283 [details]
MozReview Request: Bug 1240169 - Remove redundant remote-newtab redirector interface contract r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32067/diff/4-5/
Comment on attachment 8711284 [details]
MozReview Request: Bug 1240169 - Revert to returning a dynamic newtab URL for BROWSER_NEW_TAB_URL r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32069/diff/4-5/
Ran out of time to review this today. I'll hopefully have this out tomorrow. Sorry for the delay!
Comment on attachment 8711282 [details]
MozReview Request: Bug 1240169 - aboutNewTabService relies on AboutRedirector for default URL resolution r?mconley

https://reviewboard.mozilla.org/r/32065/#review29337

Thanks oyiptong!

::: browser/components/newtab/nsIAboutNewTabService.idl:12
(Diff revision 5)
>  [scriptable, uuid(cef25b06-0ef6-4c50-a243-e69f943ef23d)]

I'm not sure if the rule still applies, but you _might_ need to bump the uuid here (see: https://groups.google.com/forum/#!topic/mozilla.dev.platform/n6qBpyxoI6I).

If the hg hook is still there, however, you'll need to bump this.

::: browser/components/newtab/tests/browser/browser_newtab_overrides.js:133
(Diff revision 5)
> +function nextChangeNotificationPromise(aNewURL, testMessage) {
> +  return new Promise(resolve => {
> +    Services.obs.addObserver(function observer(aSubject, aTopic, aData) {  // jshint unused:false
> +      Services.obs.removeObserver(observer, aTopic);
> +      Assert.equal(aData, aNewURL, testMessage);
> +      resolve();
> +    }, "newtab-url-changed", false);
> +  });

FWIW, something similar already exists in TestUtils.jsm:

https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/testing/modules/TestUtils.jsm#48

which can be imported via

```JavaScript
Cu.import("resource://testing-common/TestUtils.jsm");
```

::: browser/components/newtab/tests/xpcshell/test_NewTabURL.js:44
(Diff revision 5)
>  function promiseNewtabURLNotification(aNewURL) {
>    return new Promise(resolve => {
>      Services.obs.addObserver(function observer(aSubject, aTopic, aData) { // jshint ignore:line
>        Services.obs.removeObserver(observer, aTopic);
>        Assert.equal(aData, aNewURL, "Data for newtab-url-changed notification should be new URL.");
>        resolve();
>      }, "newtab-url-changed", false);
>    });

Same as above re: TestUtils
Attachment #8711282 - Flags: review?(mconley) → review+
https://reviewboard.mozilla.org/r/32065/#review29337

> I'm not sure if the rule still applies, but you _might_ need to bump the uuid here (see: https://groups.google.com/forum/#!topic/mozilla.dev.platform/n6qBpyxoI6I).
> 
> If the hg hook is still there, however, you'll need to bump this.

Looks like we still need to rev the UUID. The hook is still there: bug 1170718

> FWIW, something similar already exists in TestUtils.jsm:
> 
> https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/testing/modules/TestUtils.jsm#48
> 
> which can be imported via
> 
> ```JavaScript
> Cu.import("resource://testing-common/TestUtils.jsm");
> ```

Looks like there's not even a need to import it!
Comment on attachment 8711282 [details]
MozReview Request: Bug 1240169 - aboutNewTabService relies on AboutRedirector for default URL resolution r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32065/diff/5-6/
Comment on attachment 8711283 [details]
MozReview Request: Bug 1240169 - Remove redundant remote-newtab redirector interface contract r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32067/diff/5-6/
Comment on attachment 8711284 [details]
MozReview Request: Bug 1240169 - Revert to returning a dynamic newtab URL for BROWSER_NEW_TAB_URL r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32069/diff/5-6/
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ec780c2516c6791706d28ea82558993ae7dd6e6
Bug 1240169 - aboutNewTabService relies on AboutRedirector for default URL resolution r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/1de0ed56c3d4fcfab8afe4a4f748bb7c6b13f9f7
Bug 1240169 - Remove redundant remote-newtab redirector interface contract r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc9bef95c7e139056d59851caaa0ca748f416766
Bug 1240169 - Revert to returning a dynamic newtab URL for BROWSER_NEW_TAB_URL r=mconley
Please do not consider this bug resolved, we need to land to firefox46 too!
JustOff: how we let this stew for a week or so and then request an uplift to aurora?
s/how/how about/g
Oliver, do you have any particular considerations to retain aurora broken for a week or so? If none, I think it would be definitely right to uplift as soon as tree will be opened after 46 merge.
Tracking since this is a regression. 
You can go ahead and request uplift now.   Do you have any suggestions for manual testing on nightly ? Are their particular addon developers who might help test?
status-firefox45: --- → unaffected
tracking-firefox46: --- → +
tracking-firefox47: --- → +
Flags: needinfo?(oyiptong)
Thanks Liz!

The steps to testing are simple:

1. Install an addon that replace the newtab page with a packaged-replacement (chrome://...)
2. Open the newtab page using chrome facilities, using '+' or shortcut key

Expected:

The url bar should show chrome:// and should function correctly

An example addon:

https://addons.mozilla.org/en-US/firefox/addon/speed-start/
Flags: needinfo?(oyiptong)
Please note that in case of Speed Start url bar will never contain chrome:// as it masked by adding to gInitialPages, but extension itself should render its content correctly, not as blank page. I've tested this successfully on the try build and I of course will recheck with tomorrows nightly.
JustOff: quick update. I'll request an uplift to aurora once the treeherder build against the aurora repo completes successfully.
Thanks Olivier! I'd like to confirm that aurora 46.0a2 try build from https://archive.mozilla.org/pub/firefox/try-builds/olivier@olivieryiptong.com-bd5e46513bfdeb6b99e25f8f00cedbd7b11ae9ab/ works as expected.
Comment on attachment 8711282 [details]
MozReview Request: Bug 1240169 - aboutNewTabService relies on AboutRedirector for default URL resolution r?mconley

Approval Request Comment
[Feature/regressing bug #]: Fixing a regression that was introduced in https://bugzilla.mozilla.org/show_bug.cgi?id=1218996
[User impact if declined]: Addons which override the newtab page may be broken
[Describe test coverage new/current, TreeHerder]: There are tests to verify the correct behavior as well as manual steps to reproduce. Builds correctly on top of aurora https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd5e46513bfd
[Risks and why]: Low risk. This fixes a regression that was introduced in a previous bug
[String/UUID change made/needed]: A UUID change was made for aboutNewTabService.js
Attachment #8711282 - Flags: approval-mozilla-aurora?
Comment on attachment 8711283 [details]
MozReview Request: Bug 1240169 - Remove redundant remote-newtab redirector interface contract r?mconley

Approval Request Comment
[Feature/regressing bug #]: Fixing a regression that was introduced in https://bugzilla.mozilla.org/show_bug.cgi?id=1218996
[User impact if declined]: Addons which override the newtab page may be broken
[Describe test coverage new/current, TreeHerder]: There are tests to verify the correct behavior as well as manual steps to reproduce. Builds correctly on top of aurora https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd5e46513bfd
[Risks and why]: Low risk. This fixes a regression that was introduced in a previous bug
[String/UUID change made/needed]: A UUID change was made for aboutNewTabService.js
Attachment #8711283 - Flags: approval-mozilla-aurora?
Comment on attachment 8711284 [details]
MozReview Request: Bug 1240169 - Revert to returning a dynamic newtab URL for BROWSER_NEW_TAB_URL r?mconley

Approval Request Comment
[Feature/regressing bug #]: Fixing a regression that was introduced in https://bugzilla.mozilla.org/show_bug.cgi?id=1218996
[User impact if declined]: Addons which override the newtab page may be broken
[Describe test coverage new/current, TreeHerder]: There are tests to verify the correct behavior as well as manual steps to reproduce. Builds correctly on top of aurora https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd5e46513bfd
[Risks and why]: Low risk. This fixes a regression that was introduced in a previous bug
[String/UUID change made/needed]: A UUID change was made for aboutNewTabService.js
Attachment #8711284 - Flags: approval-mozilla-aurora?
Keywords: regression
We have a pretty frequent linux64 talos tart failure as a result of:

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc9bef95c7e139056d59851caaa0ca748f416766
Bug 1240169 - Revert to returning a dynamic newtab URL for BROWSER_NEW_TAB_URL r=mconley


this is seen in:
https://bugzilla.mozilla.org/show_bug.cgi?id=1230978

I tracked it down to a range on inbound:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=linux%20svg&tochange=5d8f454c269e&fromchange=beaea66985d4&selectedJob=20620385

and then on try:
https://treeherder.mozilla.org/#/jobs?repo=try&author=jmaher@mozilla.com&fromchange=680f9797cd4e

This needs to get addressed or backed out as the 's' job is about 40% failure.  :oyiptong, can you look into this?  I am concerned about uplifting to aurora as it would make that unstable as well.
Flags: needinfo?(oyiptong)
I'm on it
Flags: needinfo?(oyiptong)
Interesting, jmaher, I've been trying to reproduce the issue.

I've been able to successfully reproduce on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=301389d79e73

But not locally, running on an optimized build using:

> mach talos-test --suite svgr-e10s --cycle 10

Note, I'm running this using Xvfb. I suspect I will need to hit try a lot, reproduce the environment or get a loaner linux instance.

What are the details of the linux test machine?
here are the basic details of the machines we use:
https://wiki.mozilla.org/Buildbot/Talos/Misc#Hardware_Profile_of_machines_used_in_automation

they are real hardware, so we use X proper without Xfvb.  I am starting to look into running talos inside of docker which would be Xfvb and would be easier to reproduce most things locally.  But that is the future, not today.

Getting a loaner isn't hard, and hacking on try isn't too hard.  here is an example bug for a linux loaner (bug 1181250)
I've investigated the linux 64 tart failures.

My initial hypothesis was that prior to bug 1218996, tart timeouts would occur due to some race conditions in the tests.
Bug 1218996 would make those race conditions less likely by removing some code that would get executed on newtab open.

Bug 1240169 reverts some of the behavior in bug 1218996 to fix a regression for addon developers, re-adding the race conditions present prior to bug 1218996.

My talos runs confirm this hypothesis:

parent:
expected: BAD, actual: BAD
deaf7c6ca55b (try push 96c842bb31a9, https://treeherder.mozilla.org/#/jobs?repo=try&revision=96c842bb31a9)
Bug 1096804 - Enable existing taskbar tests. Remove one test that uses old sync api. r=me

child:
expected: GOOD, actual: GOOD
8e0a8cdc62ad (try push 44d09394b031, https://treeherder.mozilla.org/#/jobs?repo=try&revision=44d09394b031)
Bug 1218996 - Allow Remote New Tab to ride release trains r=marcosc

child, applying bug 1240169 on top of 8e0a8cdc62ad:
expected: BAD, actual: BAD
a241dece9d0c, a rebase of cc9bef95c7e1 (try push e6f3d434ab97, https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6f3d434ab97)
Bug 1240169 - Allow chrome URL overrides in aboutNewTabService

The last change to the behavior of a new tab open, IIRC, is in 1204983. I couldn't get that revision to build due to some taskcluster/mach changes.
I would expect there to be some race conditions as well, assuming it is indeed the last change in that code path. That would further confirm my hypothesis.

Meaning, this is indeed a regression, however, it returns the race condition that was present before.
Unsure if that tart regression should affect this bug. I propose another, separate bug to fix the tart race condition.
Flags: needinfo?(jmaher)
To further confirm my suspicions, I made a patch to tart to fix race conditions. Seems like a bug in tart:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f7828f975aa&selectedJob=16268156

avi: what do you think of such a patch to fix tart? How will that affect tart performance results?
Flags: needinfo?(avihpit)
this sounds probable and I think fixing the test is probably the right thing to do under the conditions that we don't make the test more noisy or unstable.

Do you need help figuring out the build issue to help test bug 1204983?
Flags: needinfo?(jmaher)
As far as I understand the situation is such:
1. Some changes made before bug 1240169 (or even bug 1240169), caused the tart timeouts
2. Bug 1218996 introduced the regression by skipping chrome:// url rendering in newTabURL and that of course leads to timeouts reduction
3. Bug 1240169 fixes the problem, but timouts becomes visible again
4. So, bug 1240169 solution should be accepted (and uplifted to aurora) while "tart timeouts" should be tracked separately

Would anyone be so kind as to confirm whether I am right? It really bothers me, because I keep getting complaints from users having problems with my extension in aurora while this bug is marked as resolved fixed.
(In reply to Olivier Yiptong [:oyiptong] from comment #74)
> To further confirm my suspicions, I made a patch to tart to fix race
> conditions. Seems like a bug in tart:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=4f7828f975aa&selectedJob=16268156
> 
> avi: what do you think of such a patch to fix tart? How will that affect
> tart performance results?

As far as I can tell, the patch only modifies the method to change the newtab URL. This is not being measured (only opening/closing tabs and the CSS animation are measured), and therefore it should have no effect on the measured performance numbers.

However, the new code has this new function, in few URL variants:

> +        function(){
> +          let url = "chrome://tart/content/blank.icon.html";
> +          self.makeNewTabURLChangePromise(url).then(next);
> +          aboutNewTabService.newTabURL = url;
> +        }


I've have not followed the evolution of the requirements to change the newtab URL* and I'm not able to say if it fixes a race/incorrect thingy, but the patch seems to me like jumping through possibly too many hoops, both in terms of duplication of this function (I counted 6, with 5 even having the same URL), and in terms of it being separated from makeNewTabURLChangePromise.

IMO, if possible, combine this function and makeNewTabURLChangePromise into a single function which takes a URL and a "next" function, then use this function whenever the newtab URL needs to be changed.


* When I wrote it originally it consisted of simply modifying a pref, and IIRC mconley modified it not too long ago to what it is now.
Flags: needinfo?(avihpit)
Thanks, Avi, the patch was just an exploration of the root cause of the tart timeouts. However, I was only able to mitigate the race conditions, making them happen less frequently:

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

While the intention is for it not to have an impact, it looks like it does: according to bug 1244522 and bug 1240408. Perhaps we need another bug to track this.
(In reply to JustOff from comment #76)
> As far as I understand the situation is such:
> 1. Some changes made before bug 1240169 (or even bug 1240169), caused the
> tart timeouts
> 2. Bug 1218996 introduced the regression by skipping chrome:// url rendering
> in newTabURL and that of course leads to timeouts reduction
> 3. Bug 1240169 fixes the problem, but timouts becomes visible again
> 4. So, bug 1240169 solution should be accepted (and uplifted to aurora)
> while "tart timeouts" should be tracked separately
> 

Yes, that's my conclusion as well. :lizzard, what else would we need for an uplift?
> Would anyone be so kind as to confirm whether I am right? It really bothers
> me, because I keep getting complaints from users having problems with my
> extension in aurora while this bug is marked as resolved fixed.
Flags: needinfo?(lhenry)
Olivier, let's uplift your fixes for the test(s?) once you have them ready, along with this work. 
That way we won't miss other potential TART regressions due to timeouts.  Does that sound good?
Flags: needinfo?(lhenry)
Flags: needinfo?(oyiptong)
My TART test patch doesn't completely fix the problem, as I've found out, it simply mitigates them.

I'll have to do some more work to find and fix all race conditions.
Flags: needinfo?(oyiptong)
(In reply to Joel Maher (:jmaher) from comment #75)
> this sounds probable and I think fixing the test is probably the right thing
> to do under the conditions that we don't make the test more noisy or
> unstable.
> 
> Do you need help figuring out the build issue to help test bug 1204983?

Sorry jmaher, I hadn't seen your comment. Here's where I've gotten with trying to build bug 1204983:

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

I applied a try.yml fix and a pip fix. I'm unsure what else I need.

gecko-decision seems to fail, and the build fails as well, from what looks like an invalid private key for SSH
Flags: needinfo?(jmaher)
ok, there is a magic patch to make an old build work from bug 1216907.  I have it up on try to verify:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7d2f22741d7&selectedJob=16343327

you can just rip the patch from my try push.
Flags: needinfo?(jmaher)
Thank you!
Depends on: 1246695
No longer depends on: 1246695
Depends on: 1246695
Moved the race condition fix to bug 1246695.

Initial results look good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=812529639a18
So, as bug 1246695 got resolved, finally there are no more blocks for uplifting this to aurora?
Let's uplift bug 1246695 as well. I'll file the uplift request
I would greatly appreciate if this saga will be completed this week
JustOff: I filed an uplift request for Aurora in bug 1246695.

When that patch gets uplifted, can we uplift this one?
Flags: needinfo?(lhenry)
Sure, once it lands. Looks like there are some issues with the tests.  

Justoff: that means look for this to be fixed in Developer Edition next week some time.
Flags: needinfo?(lhenry)
I am grateful to all involved, but really upset by the schedule. The regression was committed to trunk (46a1) Jan 13, reported at next day, acknowledged Jan 15 and fixed in trunk (47a1) Jan 28. That looks reasonable. But since then it's passed almost a month, the fix is still not in aurora and in two weeks the regression will go into beta (46b). This two months and two branches for simple bug fix drives me crazy.
:JustOff, I'm working on it. The uplift to aurora depends on an uplift to my patch to our performance test, Talos, which currently is landed but needs some work to work on Aurora.

For the record, it's one of those bugs that work fine on my machines, but fail on our test machines.
The errors aren't very descriptive at all.

I'm hoping to get it done this week, it's my top priority. Apologies for the time taken.
I'll eat my words from comment 91. I've been able to fix the bug in Aurora. It turned out to be pretty simple in the end.

Here's a build with bug 1240169 and bug 1246695 both applied on top of Aurora: https://treeherder.mozilla.org/#/jobs?repo=try&revision=54202240a496
Comment on attachment 8711282 [details]
MozReview Request: Bug 1240169 - aboutNewTabService relies on AboutRedirector for default URL resolution r?mconley

Fix for regression in 46, let's try not to ship with this issue. 
Must be uplifted with test changes from bug 1246695
Attachment #8711282 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8711283 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8711284 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I'd like to confirm that on aurora 46.0a2 build 20160226004032 the regression is finally gone. Thank you so much!
[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: