1399517 - Extension browser action buttons disappear after Nightly update

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[Mike Conca [:mconca]]

Attachment: Buttons before Nightly update

I have a couple of extensions that put browser action buttons on the UI (Grammerly and uBlock Origin) next to the standard Downloads and Library buttons (see fx-pre-update.jpg).

When Nightly prompts me to update and restart via the hamburger menu, the browser action buttons from my extensions do not reappear (see fx-post-update.jpg).

about:addons shows the extensions are still installed and enabled and, indeed, they appear to be working correctly (ads are still being blocked).  Disabling and re-enabling does not make the buttons reappear. The buttons DO reappear if I simply close the browser and start it again.

The behavior seems consistent over the past 3-4 Nightly updates.

Comment 1

[Mike Conca [:mconca]]

Attachment: Buttons after Nightly update

Comment 2

[Mike Conca [:mconca]]

about:support

Application Basics
------------------

Name: Firefox
Version: 57.0a1
Build ID: 20170913100125
Update Channel: nightly
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
OS: Windows_NT 10.0
Multiprocess Windows: 1/1 (Enabled by default)
Web Content Processes: 5/4
Stylo: true (enabled by default)
Google Key: Found
Mozilla Location Service Key: Found
Safe Mode: false

Comment 3

[Shane Caraveo (:mixedpuppy)]

This is likely a similar problem to bug 1385630.  The startup cache (for extensions) is invalidated on an update/restart, which also causes extensions to initialize after the rest of the UI has.  We will need some mechanism to delay session restore for extension UI.

Comment 4

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> This is likely a similar problem to bug 1385630.  The startup cache (for
> extensions) is invalidated on an update/restart, which also causes
> extensions to initialize after the rest of the UI has.  We will need some
> mechanism to delay session restore for extension UI.

I don't really follow. At least for the button case, CUI should still know the correct placement for the button. When the webextension code eventually calls createWidget, the widgets should just be getting inserted automatically. If they're not, that's a CUI bug. If we're not calling createWidget, then I don't really understand why we wouldn't do that...

Comment 5

[Andy McKay]

I've been unable to reproduce this on OS X or Windows.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Hrm, I'm actually unable to reproduce this myself, at least with using the same str from the sidebar bug (ie. delete compatibility.ini and restart).  browserActions are always built after delayed startup, so as Gijs said, CUI should already be handling this.  

Mike, can you try deleting compatibility.ini from your profile after shutting down firefox, then start firefox.  This should make firefox work somewhat like an update restart.  I'd be interested if it does or does not reproduce the problem, then again when the next update happens if you still have the issue.

Being unable to repro, moving off p1, but I'm still concerned.

Comment 7

[Mike Conca [:mconca]]

Deleting compatibility.ini from my profile reproduces it perfectly.

Comment 8

[Mike Conca [:mconca]]

Still happening for me on the latest Nightly, and I'm still able to reproduce by deleting compatability.ini.  Anything I can do to help debug this?  Logs?

Comment 9

[:Gijs (he/him)]

(In reply to Mike Conca [:mconca] (Denver, CO, USA UTC-6) from comment #8)
> Still happening for me on the latest Nightly, and I'm still able to
> reproduce by deleting compatability.ini.  Anything I can do to help debug
> this?  Logs?

Can you reproduce on a clean profile where you just install the add-ons that you have this issue with? Does it happen if you have only ublock or only grammarly in the toolbar? Do you have any other add-ons installed that aren't in the toolbar?

As for logs, you can try toggling browser.uiCustomization.debug but I doubt it will tell us much here - you'd want to compare a log from a "normal" startup and one from a "removed compatibility.ini" startup.

Comment 10

[Mike Conca [:mconca]]

Attachment: fx-no-compat-ini.txt

Comment 11

[Mike Conca [:mconca]]

Attachment: fx-normal.txt

Comment 12

[Mike Conca [:mconca]]

I have been unable to reproduce this issue under a clean profile. On my default profile, I removed every add-on and started adding again. I can make the problem happen with either add-on installed, or both.

I've attached a console log with browser.uiCustomization.debug enabled that shows the events when compatibility.ini is removed as well as a second log that shows events for a "normal" startup. These were made with just uBlock installed. There are differences, and I think I can see where it seems to have difficulty building the extension button in the "no ini file" case (lines 68-70) versus the normal case (lines 49-51). I do not see these differences in a clean profile.

Comment 13

[Andy McKay]

CustomizableUI:Searching for ublock0_raymondhill_net-browser-action in toolbox.  CustomizableUI.jsm:958
CustomizableUI:No node for ublock0_raymondhill_net-browser-action found.  CustomizableUI.jsm:964
CustomizableUI:Unknown widget: ublock0_raymondhill_net-browser-action  CustomizableUI.jsm:787
vs 

CustomizableUI:Building ublock0_raymondhill_net-browser-action of type view  CustomizableUI.jsm:1412
CustomizableUI:Widget ublock0_raymondhill_net-browser-action has a view. Auto-registering event handlers.  CustomizableUI.jsm:1464
CustomizableUI:Widget ublock0_raymondhill_net-browser-action showing and hiding event handlers set.  CustomizableUI.jsm:1503

Comment 14

[:Gijs (he/him)]

(In reply to Andy McKay [:andym] from comment #13)
> CustomizableUI:Searching for ublock0_raymondhill_net-browser-action in
> toolbox.  CustomizableUI.jsm:958
> CustomizableUI:No node for ublock0_raymondhill_net-browser-action found. 
> CustomizableUI.jsm:964
> CustomizableUI:Unknown widget: ublock0_raymondhill_net-browser-action 
> CustomizableUI.jsm:787
> vs 
> 
> CustomizableUI:Building ublock0_raymondhill_net-browser-action of type view 
> CustomizableUI.jsm:1412
> CustomizableUI:Widget ublock0_raymondhill_net-browser-action has a view.
> Auto-registering event handlers.  CustomizableUI.jsm:1464
> CustomizableUI:Widget ublock0_raymondhill_net-browser-action showing and
> hiding event handlers set.  CustomizableUI.jsm:1503

Yes, but in the no compat.ini case we do do this later, specifically here:

CustomizableUI:Building ublock0_raymondhill_net-browser-action of type view  CustomizableUI.jsm:1412
CustomizableUI:Widget ublock0_raymondhill_net-browser-action has a view. Auto-registering event handlers.  CustomizableUI.jsm:1464
CustomizableUI:Widget ublock0_raymondhill_net-browser-action showing and hiding event handlers set.  CustomizableUI.jsm:1503
CustomizableUI:NotFoundError: Node was not found -- undefined:1252  CustomizableUI.jsm:2214

Note specifically the last line. So what must be happening is that this line fails:

https://hg.mozilla.org/mozilla-central/file/893fe1549e1e/browser/components/customizableui/CustomizableUI.jsm#l1252

The only way this fails is if the 'next' node is not actually in the container.

There are only 2 callsites for this method. One is when we initially populate/adjust areas on startup. That's *not* here, but the first bit that Andy already quoted, so it's not that one, so it must be this one: https://hg.mozilla.org/mozilla-central/file/893fe1549e1e/browser/components/customizableui/CustomizableUI.jsm#l1217 .

There, we've just determined the next node and insertion point, from here:

https://hg.mozilla.org/mozilla-central/file/893fe1549e1e/browser/components/customizableui/CustomizableUI.jsm#l1227

So there must be a bug in this:

https://hg.mozilla.org/mozilla-central/file/893fe1549e1e/browser/components/customizableui/CustomizableUI.jsm#l4558

Right before this code, in the log, the nav-bar placements were:

["back-button","forward-button","stop-reload-button","home-button","customizableui-special-spring1","urlbar-container","customizableui-special-spring2","downloads-button","library-button","screenshots_mozilla_org-browser-action","87677a2c52b84ad3a151a4a72f5bd3c4_jetpack-browser-action","ublock0_raymondhill_net-browser-action","pocket-button","a6438a1ae1a9893cabaa04fcbf2b58a235806f8e_temporary-addon-browser-action"]

What I'm not yet sure about is why it breaks... I'll try to work with Mike to figure this one out.

Comment 15

[:Gijs (he/him)]

Trypush with some more logging: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2676b4006ebd4b1ba998a2a9cf055daff6d98664

I have a suspicion of what's going on here, but it'd be good to have it confirmed before I try to work out how to fix it. Either way, this is going to be a Firefox issue, not a webextension one (though it's likely only webextensions trigger it at this stage).

Comment 16

[:Gijs (he/him)]

So Mike, can you get another one of those customization debug logs with a no-compat-ini situation from the build in this zip file:

https://queue.taskcluster.net/v1/task/Q0C7x5XPQmWGgtzrBimu_Q/runs/0/artifacts/public/build/target.zip

? Thanks!

Comment 17

[Mike Conca [:mconca]]

Attachment: fx-no-compat-ini-2.txt

Gijs, here you go. Note that I had both Grammerly and uBlock installed at the time I ran your build.

Comment 18

[:Gijs (he/him)]

(In reply to Mike Conca [:mconca] (Denver, CO, USA UTC-6) from comment #17)
> Created attachment 8909125 [details]
> fx-no-compat-ini-2.txt
> 
> Gijs, here you go. Note that I had both Grammerly and uBlock installed at
> the time I ran your build.

Super, this is very helpful. My guesses were wrong, here's the issue:

CustomizableUI:Considering inserting 87677a2c52b84ad3a151a4a72f5bd3c4_jetpack-browser-action before pocket-button  CustomizableUI.jsm:4576
CustomizableUI:pocket-button is not overflown  CustomizableUI.jsm:4589
CustomizableUI:Inserting 87677a2c52b84ad3a151a4a72f5bd3c4_jetpack-browser-action before pocket-button in nav-bar-customization-target  CustomizableUI.jsm:1217
CustomizableUI:Note that pocket-button is in a hbox with id pocket-button-box  CustomizableUI.jsm:1219

We now have pocket in a page action button, but we never removed the pocket-button id from existing toolbar placements. This is causing this bug, because we're trying to insert next to pocket, and although that button exists, it isn't anywhere near where we expect it to be. We can ensure that we remove pocket-button from the saved placements as a migration step inside CustomizableUI.

We should probably also update this button insertion code so it just avoids using nodes that aren't in the right container. I'll come up with patches for both of those issues today or tomorrow.

Comment 19

[:Gijs (he/him)]

Attachment: Bug 1399517 - CustomizableUI shouldn't try to insert items next to nodes that aren't in the right container,

Review commit: https://reviewboard.mozilla.org/r/180898/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180898/

Comment 20

[:Gijs (he/him)]

Attachment: Bug 1399517 - remove CustomizableUI placements for old buttons to avoid saved state corruption,

Review commit: https://reviewboard.mozilla.org/r/180900/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180900/

Comment 21

[Mike de Boer [:mikedeboer]]

Comment on attachment 8909361 [details]
Bug 1399517 - CustomizableUI shouldn't try to insert items next to nodes that aren't in the right container,

https://reviewboard.mozilla.org/r/180898/#review186094

Well, nits only from me here ;-) Good find!

::: commit-message-ffe6c:1
(Diff revision 1)
> +Bug 1399517 - don't try to insert next to nodes that aren't in the right container, r?mikedeboer

nit: please elaborate that this commit will be about CUI.

::: browser/components/customizableui/CustomizableUI.jsm:4575
(Diff revision 1)
> -        if (newNodeCanOverflow && this._collapsed.has(nextNodeId)) {
> -          let nextNode = this._list.getElementsByAttribute("id", nextNodeId).item(0);
> -          if (nextNode) {
> +        let nextNode = aNode.ownerDocument.getElementById(nextNodeId);
> +        if (newNodeCanOverflow && this._collapsed.has(nextNodeId) &&
> +            nextNode && nextNode.parentNode == this._list) {
> -            return [this._list, nextNode];
> +          return [this._list, nextNode];
> -          }
> +        }
> -        }
> +        if ((!nodeBeforeNewNodeIsOverflown || !newNodeCanOverflow) && nextNode &&

nit: The density at which this loop is written and the elaborate list of conditionals left me wishing there were inline comments sprinkled across that explain the flow in-situ. (Which you can copy to the similar loop above.)

::: browser/components/customizableui/test/browser_insert_before_moved_node.js:14
(Diff revision 1)
> +    CustomizableUI.createWidget({id: "real-button", label: "test real button"});
> +    CustomizableUI.addWidgetToArea("real-button", toolbar);
> +    CustomizableUI.addWidgetToArea("moved-button-not-here", toolbar);
> +    let placements = CustomizableUI.getWidgetIdsInArea(toolbar);
> +    Assert.deepEqual(placements.slice(-2), ["real-button", "moved-button-not-here"],
> +                     "Should have correct placements");

nit: I'm never sure what our convention is, but I think the most modern one would be to indent a statement break with two spaces, instead of aligning them with the first argument.

::: browser/components/customizableui/test/browser_insert_before_moved_node.js:29
(Diff revision 1)
> +
> +    let button = document.getElementById("real-button");
> +    ok(button, "Button should exist");
> +    if (button) {
> +      is(button.parentNode, document.getElementById(toolbar).customizationTarget,
> +         "Button should be in the toolbar");

nit: ditto.

Comment 22

[Mike de Boer [:mikedeboer]]

Comment on attachment 8909361 [details]
Bug 1399517 - CustomizableUI shouldn't try to insert items next to nodes that aren't in the right container,

https://reviewboard.mozilla.org/r/180898/#review186416

Comment 23

[Mike de Boer [:mikedeboer]]

Comment on attachment 8909362 [details]
Bug 1399517 - remove CustomizableUI placements for old buttons to avoid saved state corruption,

https://reviewboard.mozilla.org/r/180900/#review186418

::: commit-message-4772a:1
(Diff revision 1)
> +Bug 1399517 - remove placements for old dead buttons, r?mikedeboer

nit: Again, please allow for a bit more room in your commit message to give this patch some context.

Comment 24

[:Gijs (he/him)]

Comment on attachment 8909361 [details]
Bug 1399517 - CustomizableUI shouldn't try to insert items next to nodes that aren't in the right container,

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

Comment 25

[:Gijs (he/him)]

Comment on attachment 8909362 [details]
Bug 1399517 - remove CustomizableUI placements for old buttons to avoid saved state corruption,

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

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/83b48abc8433
CustomizableUI shouldn't try to insert items next to nodes that aren't in the right container, r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/685c28b080b3
remove CustomizableUI placements for old buttons to avoid saved state corruption, r=mikedeboer

Comment 27

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

https://hg.mozilla.org/mozilla-central/rev/83b48abc8433
https://hg.mozilla.org/mozilla-central/rev/685c28b080b3

Comment 28

[:Gijs (he/him)]

Mike emailed me to say this was fixed when he updated to the nightly with this fix. \o/