1240559 - Regression in beta: NewTabURL.jsm is missing. NewTab extensions don't work in Firefox 44 Beta, works in Firefox 43 and in Firefox 46+

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[Sören Hentzschel]

Attachment: screenshot

[Tracking Requested - why for this release]:

STR:

1. install New Tab Override (https://addons.mozilla.org/en-US/firefox/addon/new-tab-override/)
2. go to the add-on settings and change the new tab page

Expected:

It works.

Actual:

It works in Firefox 43.0.4 Stable, Firefox 45 Developer Edition and Firefox 46 Nightly but not in Firefox 44 Beta 9.

The browser console says:

Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]

It should be tracked for Firefox 44 because it's a broken user customization and because it works in Firefox 43 AND Firefox 45 so it's clear that there is something wrong in Firefox 44.

Comment 1

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Christoph, is this in a related area of the bug you fixed this week (deprecation of warning)? This is not as a consequence of your fix but more a request to help me look for an owner to investigate how severe is this issue as it seems to be a Fx44 only bug. Appreciate your help!

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ritu Kothari (:ritu) from comment #1)
> Christoph, is this in a related area of the bug you fixed this week
> (deprecation of warning)? This is not as a consequence of your fix but more
> a request to help me look for an owner to investigate how severe is this
> issue as it seems to be a Fx44 only bug. Appreciate your help!

Can't today, but can have a look tomorrow morning. We should definitely consult Olivier or at least let him know about this bug. ni? him to make sure he has seen the bug. Olivier, let's chat tomorrow.

Comment 3

[Olivier Yiptong [:oyiptong]]

This is a duplicate of bug 1240169. I'm working on a fix.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olivier Yiptong [:oyiptong] from comment #3)
> This is a duplicate of bug 1240169. I'm working on a fix.
> 
> *** This bug has been marked as a duplicate of bug 1240169 ***

Thanks Olivier! Clearing my needinfo in that case!

Comment 5

[Sören Hentzschel]

How can a regression in Firefox 46 be a duplicate of a regression in Firefox 44? And if it's correct then bug 1240169 should be nominated for tracking for Firefox 44. It works in Firefox 45 and Firefox 46 without additional fixes…

Comment 6

[Olivier Yiptong [:oyiptong]]

I may have commented on bug 1240169 in haste. The comment (https://bugzilla.mozilla.org/show_bug.cgi?id=1218996#c66) that prompted me to file this bug also said that the problem started occurring in Firefox 44.

I'm investigating and may re-open this bug if need be

Comment 7

[Olivier Yiptong [:oyiptong]]

It is indeed a different problem. Reopening, and taking a look

Comment 8

[Olivier Yiptong [:oyiptong]]

It is a bug introduced in bug 1224950. My fault, really. The goal was to prevent some code from going beyond Aurora, and NewTabURL.jsm is not included

Comment 9

[Olivier Yiptong [:oyiptong]]

specifically, a condition prevents NewTabURL.jsm from being included at build time: http://mxr.mozilla.org/mozilla-beta/source/browser/components/newtab/moz.build#7

Comment 10

[Olivier Yiptong [:oyiptong]]

It is too late to uplift a fix to beta.

The easiest thing to do is to call the nsIAboutNewTabService implementation.

This is done in your addon as such:

> const {Cu} = require('chrome');
> Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> XPCOMUtils.defineLazyServiceGetter(this, "aboutNewTabService",
>                                    "@mozilla.org/browser/aboutnewtab-service;1",
>                                    "nsIAboutNewTabService");

To override, you can call:

> aboutNewTabService.newTabURL = newTabUrl;

I've made modifications to your index.js file that demonstrates how that would work: https://gist.github.com/oyiptong/380dc060c2d0f8d1ed5b

46 doesn't have this problem. 45 will once it goes to beta, but I can uplift a patch.
44 will be broken, as a release candidate for beta is going to be built today.

Hope that helps

Comment 11

[Sören Hentzschel]

Thanks, I reverted the previous version of my add-on which uses aboutNewTabService and uploaded it as new version to AMO. New Tab Override 2.3.1 will work in Firefox 44.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Not a ship stopper, too late, wontfix for Fx44.

Comment 13

[Jorge Villalobos [:jorgev] (he/him)]

Is there anything to do for this bug, given comment #12?

Comment 14

[Olivier Yiptong [:oyiptong]]

No, there isn't anything left to do. I'll close this as wontfix

Comment 16

[Ben Bucksch (:BenB)]

This is a major goof for ext compat. You goofed, you'll have to fix it. You can't leave the dirt on our carpet.
You can't just remove APIs without warning and then say "sorry".

This is a must fix. And even if FF4 were to be released tomorrow, you should still be glad to have found it before it hits end users.

> No, there isn't anything left to do.

NewTabURL.jsm was accidentally removed. There's a trivial fix: Re-add the file.

Comment 17

[Olivier Yiptong [:oyiptong]]

It has been re-added, in 46

Comment 18

[Olivier Yiptong [:oyiptong]]

44 is too late.

Given that it's too late for 44, you would already need the fallback to the service correct?

Something like:

> try {
>   Cu.import("resource:///modules/NewTabURL.jsm");
> } catch (e) {
>   Cc["@mozilla.org/browser/aboutnewtab-service;1"].getService(Ci.nsIAboutNewTabService);
> }

Now, does it help if it's uplifted for 45 if you have that conditional load?

Comment 19

[Ben Bucksch (:BenB)]

For the record, alpha was fine, and then you uplifted the breaking change to beta. We didn't expect that. (Plus, all that with Christmas in between.) This is why this wasn't even on our radar.

Now, please take the consequences and revert the uplifted change. Or alternatively fix it, that's trivial, as mentioned: Just re-add the NewTabURL.jsm file you removed.

Otherwise we'll have a few million users with a broken extension on our feet. Not just that the newtab override doesn't work, but the whole page is very visibly broken - I'll attach a screenshot. We can't make a fix release until Jan 26 (one working day from now).

FF 4 is not out yet, so it's not too late. Once a few million users see this broken screen, THEN it will be too late.

Comment 20

[Ben Bucksch (:BenB)]

Attachment: Broken newtab

This is now the Firefox newtab page will look like for a few million (!) Firefox users, the users of our extensions.

Comment 21

[Ben Bucksch (:BenB)]

I don't care about FF 46 or FF 45. We can fix our ext in time. I cannot fix our extension until Tuesday, when this will hit end users. I care about FF 44.

You goofed. Ball's in your park.

Comment 22

[Olivier Yiptong [:oyiptong]]

I'm sorry about the situation. What is the addon please?

Unfortunately, we cannot revert the uplift, because there's a security issue that prompted it in the first place.

It landed in bug 1224950 on Nov 20 and uplifted on Dec 01 to Aurora.

I'll see what I can do, but a patch would need to be to be made and it's too risky at this point. RC builds have already been made.

In the mean time, how would a conditional load work as described in comment 18? How soon can you get the addon fixed?

Comment 23

[Ben Bucksch (:BenB)]

> there's a security issue that prompted it in the first place.

Please elaborate in detail.

> we cannot revert the uplift,

Then add a NewTabURL.jsm that implements whatever you expect an addon to do. That would keep all exts working. That's what an API is for, to isolate others from underlying implementation changes.

> a patch would need to be to be made

We better make a patch right now. It's 1 AM here, for the record, and I had other plans for the weekend.

> it's too risky at this point

Riskier than having in-the-face brokenness - of the startpage, for crying out load - for a few million users? I highly doubt that.

Comment 24

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

I take responsibility as the reviewer of the original patch for not noticing how this would impact beta+ releases. I'm out tonight, but have cycles this weekend to try to work a fix.

Unsure I'll get relman buy in, but we'll see.

Comment 25

[Ben Bucksch (:BenB)]

Thank you!
I'll be available Sunday, in case I can help, with coding (FF patch) or testing or otherwise.

Comment 26

[Florian Quèze [:florian]]

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

Comment 27

[Florian Quèze [:florian]]

Attachment: Possible patch

I think this is the straight forward patch. I haven't tested it locally (yet).

Comment 28

[Justin Dolske [:Dolske]]

This is ultimately release-drivers' call, to balance the benefit involved fixing the issue with the risk of taking a last-second fix that might introduce some other problem. There are limited choices this late right before a release.

Just to summarize, AIUI:

NewTabURL.jsm is basically the API we added when we removed the ability to set the new tab page URL via a pref due to hijacking, which was a fairly widely discussed issue. Sounds like this regressed for Firefox 44 -- and only Firefox 44 -- as the result of other work (comments 1-9). We lost also test coverage, and since it's an API to replace Firefox functionality users won't notice unless they're using one of those addons.

The original addon this was reported against was quickly fixed to work around the issue. Today we have reports of 2 other addons (BenB's, and mkaply's from 1242049) that are also broken. Ben says he has millions of users, and looking at the AMO MXR I see a number of other addons that at lease reference this file (for those, actual impact and number of users I don't know).


This is all new info since reldrivers wontfixed back in comment 12, so it's fair to reopen discussion for what to do for the Firefox 44 release.

That said, the bar is high. This issue wasn't noticed for basically an entire Beta cycle (and part of when it was on Aurora). There has already been an effort underway to tightly restrict things landing late in 44 due to stability concerns (https://groups.google.com/d/msg/mozilla.dev.planning/jDiniugtwgU/ByzJN8H-AgAJ). I'm not familiar with this code, and we don't have a tested and reviewed patch, so a risk evaluation can't be made yet.


Firefox 44 is already out the door in final RC testing, for release on Tuesday the 26th, so any change would be _extraordinary_ at this point.

Not my call, but given that Firefox releases are initially throttled, and users tend to update slowly anyway, I'd tend to think that the least-worst option at this point would be for BenB to update his addon to workaround the problem (and for us to help expedite that through any AMO process), release 44 basically as planned, and take a fix for a 44.0.1 point release to address any other addons using it.

I also feel compelled to note that this kind of problem is exactly why it's important for addon authors to be testing throughout beta. :-( It would have been trivial to fix this if it had been reported anywhere between the beginning of December to just a few days ago.

Comment 29

[Florian Quèze [:florian]]

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

Comment 30

[Florian Quèze [:florian]]

Attachment: Patch v2

Actually verified locally that the tests pass this time.

Comment 31

[Sören Hentzschel]

(In reply to Justin Dolske [:Dolske] from comment #28)
> I also feel compelled to note that this kind of problem is exactly why it's
> important for addon authors to be testing throughout beta. :-( It would have
> been trivial to fix this if it had been reported anywhere between the
> beginning of December to just a few days ago.

I agree that it's important to test add-ons not only with the latest stable release of Firefox. That's the reason why I test my add-ons in the latest stable release and with Firefox Nightly. My assumption was: if the add-on works in the latest stable release and in Firefox Nightly, it can't be broken between these releases. ;)

There is also another problem: my add-on was not affected in December. It used NewTabURL.jsm in Firefox < version 44 and nsIAboutNewTabService for Firefox 44+ because NewTabURL.jsm was deprecated and I wanted to avoid console spamming. Then the deprecation was reverted a few days ago and with the latest update of my add-on I removed the obsolete code path to simplify the add-on again. I filed this ticket one day after the release of the latest update of my add-on, the old version was not affected. So my "fix" for this problem was to revert my add-on to an old version.

Comment 32

[Justin Dolske [:Dolske]]

Oh, one other piece of info that would be helpful for release-drivers:

In the currently-affected addons, is there any user impact other than the obviously-sucky experience of having a broken looking new tab (ala attachment 8711218 [details])? Is other browser and addon functionality ok? (I assumed so, since no one mentioned anything, but want to be sure.)

Comment 33

[Justin Dolske [:Dolske]]

(In reply to Florian Quèze [:florian] [:flo] from comment #30)
> Created attachment 8711267 [details] [diff] [review]
> Patch v2
> 
> Actually verified locally that the tests pass this time.

Someone familiar with the code also needs to verify that re-adding NewTabURL.jsm has relevant code working as expected, especially if the test coverage is not comprehensive. This code hasn't been running on 44 for the whole beta cycle, and we've been bitten before by uplifting/enabling code that doesn't actually work due to release-specific changes. (For example, a JS feature used by the code being disabled until the following release.)

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Is there a test we could add that would have caught this?

Comment 35

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8711267 [details] [diff] [review]
Patch v2

Review of attachment 8711267 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js
@@ +24,5 @@
>    Assert.equal(aboutNewTabService.newTabURL, "about:newtab", "Newtab URL should be the about:newtab");
>  
>    // change newtab page to remote
>    Services.prefs.setBoolPref("browser.newtabpage.remote", true);
> +  let expected = AppConstants.RELEASE_BUILD ? "about:newtab" : "about:remove-newtab";

typo: it should be about:remote-newtab

::: browser/components/newtab/tests/xpcshell/test_NewTabURL.js
@@ +23,5 @@
>    Assert.equal(NewTabURL.get(), "about:newtab", "Newtab URL should be the about:newtab");
>  
>    // change newtab page to remote
>    Services.prefs.setBoolPref("browser.newtabpage.remote", true);
> +  let expected = AppConstants.RELEASE_BUILD ? "about:newtab" : "about:remove-newtab";

as above

Comment 36

[Olivier Yiptong [:oyiptong]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #34)
> Is there a test we could add that would have caught this?

We don't have any code that uses this module since aboutNewTabService has been introduced. The flag that prevents the module from being included in the build also prevents the module's tests from being included.

It currently is only used by addon developers.

Comment 37

[Olivier Yiptong [:oyiptong]]

Note that we also need an uplift to 45, as this module is also not present in 45: http://mxr.mozilla.org/mozilla-aurora/source/browser/components/newtab/moz.build

It is back in 46, however.

Comment 38

[Ben Bucksch (:BenB)]

I cannot reproduce the screenshot I attached. It was produced by my product manager using "44.0a2 (2015-12-14) Firefox Developer Edition".
I am testing with FF 44.0b9 and I don't see such a half-broken page.
Sorry for the confusion.

Thank you - Mike Conley, Florian, and Justin Dolske - that you're taking this seriously. Again sorry for stating that this more serious than it is (at least than what I can reproduce).

However, the newtab replacement - which is a fairly important feature of our extension - definitely is broken. The user sees the normal Firefox newtab, which for him is wrong, because he normally sees our modified, branded newtab. It's bad and will confuse users, but it's not as bad as a completely broken page.

My goal is that users will not be switched back to FF newtab, even for a short period, because it will baffle and confuse them.

We can in theory fix this in our extension, but if FF44 is released as-is, we'll have to carry 3 different ways to override NewTab, for various FF releases.

More difficult is we'll have to get a fix of our extension out before FF44 hits end users. We'll need to go through QA and release approval etc. on our side as well. Plus, users will need to download the update - extension updates are throttled on Firefox client side as well, and there will be a random amount of users who get the FF44 release before they get the extension update, even if we were to get it out Monday morning, which is unlikely.

Comment 39

[Lawrence Mandel [:lmandel] (use needinfo)]

From the comment thread I gather that this is:

1. a known (and inadvertent) regression
2. that affects more than 1M users in know add-on bustage (comment 23)
3. and may impact more in unknown add-on bustage (comment 28)
4. for which we have an isolated fix (comment 36)

We do not want be in a position where we have to ship a 44.0.1 point release to fix an issue that we were aware of before the release. I suggest that we pursue the fix, qualify a new build, and even ship a day or two late if we have to.

Comment 40

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

Comment on attachment 8711267 [details] [diff] [review]
Patch v2

Review of attachment 8711267 [details] [diff] [review]:
-----------------------------------------------------------------

Module the about:remove-newtab -> about:remote-newtab issue that oyiptong spotted, this looks right to me.

Above all, we're packaging NewTabURL.jsm and testing it, so this should fix the add-on compatibility issues.

Comment 41

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

s/Module/Modulo

Comment 42

[Florian Quèze [:florian]]

Attachment: Patch v3 (typo fixed)

Comment 43

[Florian Quèze [:florian]]

Comment on attachment 8711326 [details] [diff] [review]
Patch v3 (typo fixed)

Requesting approval for 44 and 45.

Approval Request Comment
[Feature/regressing bug #]: bug 1224950
[User impact if declined]: add-ons overriding the new tab page will be broken.
[Describe test coverage new/current, TreeHerder]: Verified on a local build with the patch that NewTabURL.jsm works as expected. Covered by unit tests. Tests are green on try.
[Risks and why]:  Very low. This just packages again one file that was unpackaged by accident, and re-enables unit tests.
[String/UUID change made/needed]: none.

Comment 44

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #29)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4864815aec35

Ben, could you please confirm that the builds from this try push are fixed for you?

Comment 45

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8711326 [details] [diff] [review]
Patch v3 (typo fixed)

Taking it. You can, please, land that in m-r directly. Otherwise, I will do it later this week end (understand: when my small troll leaves me enough time to do it)

I guess this does not impact android, right?

Comment 46

[Sylvestre Ledru [:Sylvestre]]

If I understand correctly the uplift request, 45 is affected too.

Comment 47

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8711326 [details] [diff] [review]
Patch v3 (typo fixed)

[Triage Comment]
45 is still in aurora

Comment 48

[Florian Quèze [:florian]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #45)
> Comment on attachment 8711326 [details] [diff] [review]
> Patch v3 (typo fixed)
> 
> Taking it. You can, please, land that in m-r directly.

https://hg.mozilla.org/releases/mozilla-release/rev/7eabe4d30cde

> I guess this does not impact android, right?

No android impact.

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a4743600b750

Comment 50

[Florian Quèze [:florian]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #34)
> Is there a test we could add that would have caught this?

This API was actually covered by xpcshell tests, but bug 1224950 disabled them accidentally.

Comment 51

[Ben Bucksch (:BenB)]

I've tested it against my extension, the currently released version, without any workaround hotfix in my extension.
With the 44.0b9 build, I see Firefox NewTab.
With the latest 2016-01-24 02:56 "candidate 44.0 build 3" from http://ftp.mozilla.org/pub/firefox/candidates/44.0-candidates/build3/linux-x86_64/de/firefox-44.0.tar.bz2 , I see our own NewTab again.

So, this is VERIFIED FIXED.

Thank you VERY MUCH, a BIG thank you to all involved, but particularly to:
* Florian Quèze for providing the patch
* Mike Conley and Justin Dolske for taking it seriously, and
* Sylvestre Ledru and RyanVM and lmandel for taking and merging it.
and everybody else involved, and: all this on a weekend!

Really, this was awesome, and responsible, and top notch.
A bow to your professionalism and empathy. Firefox at its best.

Comment 52

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Ben Bucksch (:BenB) from comment #51)
> I've tested it against my extension, the currently released version, without
> any workaround hotfix in my extension.
> With the 44.0b9 build, I see Firefox NewTab.
> With the latest 2016-01-24 02:56 "candidate 44.0 build 3" from
> http://ftp.mozilla.org/pub/firefox/candidates/44.0-candidates/build3/linux-
> x86_64/de/firefox-44.0.tar.bz2 , I see our own NewTab again.
> 
> So, this is VERIFIED FIXED.
> 
> Thank you VERY MUCH, a BIG thank you to all involved, but particularly to:
> * Florian Quèze for providing the patch
> * Mike Conley and Justin Dolske for taking it seriously, and
> * Sylvestre Ledru and RyanVM and lmandel for taking and merging it.
> and everybody else involved, and: all this on a weekend!
> 
> Really, this was awesome, and responsible, and top notch.
> A bow to your professionalism and empathy. Firefox at its best.

Yay! Thank you for the verification and also for your good kind words.