1245970 - Two "start a conversation..." menuitems under Tools menu in Firefox 45b2

Status: RESOLVED WONTFIX

(Hello (Loop) :: Client, defect, P2)

Description

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Regression on beta that should get fixed

Doesn't go away with a restart. Doesn't happen on a clean profile, but happens on my existing beta profile.

Happy to debug with the browser debugger and friends what on earth is going on. But yeah, we shouldn't be doing this.

Comment 1

[Mike de Boer [:mikedeboer]]

Let's hope this will not be RDF related and need to touch nsBrowserGlue...

Comment 2

[:Gijs (he/him)]

https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/browser/extensions/loop/bootstrap.js#294

where is menuItem defined? And if it isn't, doesn't this mean it ends up an undeclared global, and if that's true, doesn't that mean that it would only ever append 1 menuitem even if there's multiple windows?

Doesn't really solve this issue, but very confusing, and I don't understand why eslint didn't freak out about this.

Comment 3

[:Gijs (he/him)]

Ugh, find in page failed me. Never mind!

Maybe setupBrowserUI is getting called twice for my window?

Comment 4

[:Gijs (he/him)]

So the bootstrap.js menuItem doesn't create an oncommand attribute, and yet one of the two menuitems in my window does have one of those attributes. Feels like somehow a previous version of the code is somehow still interfering with it?

Ah, so it seems that  bug 1229933 did not get uplifted to beta, and that's why:

https://mxr.mozilla.org/mozilla-beta/search?string=menu_openLoop&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-beta

Comment 5

[:Gijs (he/him)]

It would probably make sense to audit for any other changes to code under browser/extensions/loop/ or (browser/base that mentions loop) that were made in 46 before development moved to the add-on repo, because we're now using that to push stuff into 45, and that is going to break stuff if it conflicts with things that are "different" in 45.

Comment 6

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Affecting beta only, so not treating as a P1

Comment 7

[:Gijs (he/him)]

(In reply to Emma Humphries [:emceeaich] (UTC-8) NEEDINFO? me from comment #6)
> Affecting beta only, so not treating as a P1

I don't understand this deprioritization. It's affecting beta so we're releasing this in a few weeks, rather than months. Shouldn't that lead to a higher prioritization rather than a lower one?

Comment 8

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

It's not being worked on in the 1.2 release, so it's a P2 instead of a P1. That's following the guidelines that the Hello team is using for the Priority file.

Comment 9

[Mark Banner (:standard8)]

As described, this particular bug seems to only affect users who have installed the beta add-on from the AMO site (or manually etc). Hence, it'll only affect a small population in the worst case.

Realistically, this is going to be fixed by uplifting bug 1229933 as soon as we resolve our bugs that are currently blocking uplifts.

Worst case, we could "fix" this in the add-on after the 45 release, but before the next release of the add-on by adding a simple if statement.

Comment 10

[:Gijs (he/him)]

(In reply to Mark Banner (:standard8) from comment #9)
> As described, this particular bug seems to only affect users who have
> installed the beta add-on from the AMO site (or manually etc). Hence, it'll
> only affect a small population in the worst case.

I don't think I ever installed the add-on manually / from AMO. Isn't this the "go faster" automatically distributed add-on?

Comment 11

[Mark Banner (:standard8)]

(In reply to :Gijs Kruitbosch from comment #10)
> (In reply to Mark Banner (:standard8) from comment #9)
> > As described, this particular bug seems to only affect users who have
> > installed the beta add-on from the AMO site (or manually etc). Hence, it'll
> > only affect a small population in the worst case.
> 
> I don't think I ever installed the add-on manually / from AMO. Isn't this
> the "go faster" automatically distributed add-on?

When I asked on IRC, about if the add-on was showing in Tools -> Add-ons, you said yes. Given that system add-ons don't show in the add-on manager I therefore assumed that you had somehow installed an additional add-on. This would be consistent with bug 1229933 not having been uplifted.

The only other issue I know of that could be bug 1246168, but that would only happen if the Hello add-on updated whilst you were running FF, and since we haven't done any releases yet that's unlikely.

Comment 12

[Mike de Boer [:mikedeboer]]

Interestingly, the browser-chrome menu-item is still there: https://hg.mozilla.org/releases/mozilla-beta/file/tip/browser/base/content/browser-menubar.inc#l503
This should have been removed in a commit similar to https://hg.mozilla.org/releases/mozilla-aurora/rev/ac9c0f02ef33

Comment 13

[Mike de Boer [:mikedeboer]]

Stealing this, if you don't mind Mark.

Comment 14

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: remove menuitem from default browser chrome

Comment 15

[Mike de Boer [:mikedeboer]]

I left the locale changes out, because the l10n team won't be happy with a change on a string-frozen branch.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 8717348 [details] [diff] [review]
Patch v1: remove menuitem from default browser chrome

This is right, but I don't want to land this yet, until we get the add-on uplifted to 45 - the current version of the add-on installed in 45 doesn't have this menu option at the moment (I explicitly disabled it last time we uplifted to avoid string issues).

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Tracking to make sure this is fixed before release day.

Comment 18

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.1: remove menuitem from default browser chrome

Approval Request Comment
[Feature/regressing bug #]: Loop/ Hello uplift
[User impact if declined]: Duplicate menu item in the Tools menu.
[Describe test coverage new/current, TreeHerder]: only applies to Beta
[Risks and why]: trivial
[String/UUID change made/needed]: n/a

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8719543 [details] [diff] [review]
Patch v1.1: remove menuitem from default browser chrome

Taking it.
should be in 45 beta 6.
Thanks

Comment 20

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e176b12c95c8

Comment 21

[Bogdan Maris, Desktop QA]

As I understand from the comments here the initial issue was reproduced when hello was not an add-on. I could not reproduce the initial issue though using 45 beta 2 using clean and old, loaded profile. 

Since the fix entered beta yesterday then it should arrive in 45 Beta 7 (where Hello will come as an add-on). 


Was the fix targeted for Hello as add-on or not, or both? And what build should be used to verify the fix? Since I could not repro the initial issue I think Gijs could take a quick look at this using he's profile.

Comment 22

[Carsten Book [:Tomcat]]

backed out by request from sylvestre in https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=8bf2c5452d44

Comment 23

[Mike de Boer [:mikedeboer]]

This was backed out, so no fix available on beta.