If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Extension browser action buttons disappear after Nightly update

VERIFIED FIXED in Firefox 57

Status

()

Firefox
Toolbars and Customization
P1
normal
VERIFIED FIXED
10 days ago
3 days ago

People

(Reporter: mconca, Assigned: Gijs)

Tracking

57 Branch
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [reserve-photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments)

Created attachment 8907648 [details]
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.
Created attachment 8907649 [details]
Buttons after Nightly update
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
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.
Priority: -- → P1
(Assignee)

Comment 4

10 days ago
(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

10 days ago
I've been unable to reproduce this on OS X or Windows.
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.
Flags: needinfo?(mconca)
Priority: P1 → P2
Deleting compatibility.ini from my profile reproduces it perfectly.
Flags: needinfo?(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?
(Assignee)

Comment 9

9 days ago
(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.
Flags: needinfo?(mconca)
Created attachment 8908367 [details]
fx-no-compat-ini.txt
Flags: needinfo?(mconca)
Created attachment 8908368 [details]
fx-normal.txt
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.
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
(Assignee)

Comment 14

8 days ago
(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.
(Assignee)

Comment 15

8 days ago
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).
Component: WebExtensions: Frontend → Toolbars and Customization
Product: Toolkit → Firefox
(Assignee)

Comment 16

7 days ago
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!
Flags: needinfo?(mconca)
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.
Flags: needinfo?(mconca)
(Assignee)

Comment 18

6 days ago
(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.
Assignee: nobody → gijskruitbosch+bugs
Blocks: 1387077
Status: NEW → ASSIGNED
Priority: P2 → P1
Whiteboard: [reserve-photon-structure]
(Assignee)

Updated

6 days ago
Flags: qe-verify+
QA Contact: gwimberly
Iteration: --- → 57.3 - Sep 19
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

4 days ago
mozreview-review
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

4 days ago
mozreview-review
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
Attachment #8909361 - Flags: review?(mdeboer) → review+

Comment 23

4 days ago
mozreview-review
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.
Attachment #8909362 - Flags: review?(mdeboer) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 days ago
Depends on: 1401217

Comment 26

4 days ago
Pushed by gijskruitbosch@gmail.com:
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

4 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/83b48abc8433
https://hg.mozilla.org/mozilla-central/rev/685c28b080b3
Status: ASSIGNED → RESOLVED
Last Resolved: 4 days ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Assignee)

Comment 28

3 days ago
Mike emailed me to say this was fixed when he updated to the nightly with this fix. \o/
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.