Closed Bug 951612 Opened 10 years ago Closed 10 years ago

On browser shutdown ABP and Session Manager 'Undo Close' button positions on Bookmark Toolbar are not saved correctly.

Categories

(WebExtensions :: General, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: Virtual, Assigned: morac)

References

(Blocks 1 open bug)

Details

(Keywords: nightly-community, Whiteboard: [Australis:P-])

Attachments

(2 files)

When I have Undo Close button on the left and AdBlock Plus button on the right. All placed in the right part of the Bookmarks Toolbar. On browser restart the button places will be swapped. I also tried many configurations with FVD, Downloads & Customize and Configure buttons and this only happens only with AdBlock Plus and Session Manager button.
I use latest Session Manager 0.8.0.9pre20131215b and latest AdBlock Plus 2.4.0.3748 on Nightly 32bit on Windows 7 64bit.
Whiteboard: [Australis]
If this is really specific to these add-ons I'd be very surprised if this were a core issue, so P-'ing and not tracking for now.
Component: Toolbars and Customization → Extension Compatibility
Summary: On browser shutdown add-ons button positions on Bookmark Toolbar are not saved correctly. → On browser shutdown ABP and session manager 'undo close' button positions on Bookmark Toolbar are not saved correctly.
Whiteboard: [Australis] → [Australis:P-]
For reference, the issue is likely that Adblock Plus version in question uses the new CustomizableUI API whereas Session Manager doesn't yet from what I can tell.
FWIW, it is known that the ABP icon disappears from the nav-bar after a Firefox restart.
Summary: On browser shutdown ABP and session manager 'undo close' button positions on Bookmark Toolbar are not saved correctly. → On browser shutdown ABP and Session Manager 'Undo Close' button positions on Bookmark Toolbar are not saved correctly.
After going to chrome://browser/content/ I'm not seeing any Session Manager 'Undo Close' button, so maybe this will be some hint.
Flags: needinfo?(morac99-firefox2)
Flags: needinfo?(morac99-firefox2)
I'm having this issue with my NoScript button. I usually have it to the left of the Location Window on my Nav Bar and when Nightly is relaunched, it moves to the right of the Location Window. This has only been happening with the last few builds.
I also get this with LastPass. I want to put it in the menu but it keeps moving back.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: On browser shutdown ABP and Session Manager 'Undo Close' button positions on Bookmark Toolbar are not saved correctly. → On browser shutdown some of the add-ons buttons positions on Bookmark Toolbar are not saved correctly.
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #9)
> I also get this with LastPass. I want to put it in the menu but it keeps
> moving back.

This is a totally different issue, though, because it's (a) a different add-on, and (b) a different location, and (c) about the location as a whole rather than just a reordering of two buttons that do remember the toolbar they're supposed to be in. Two of those also apply to comment #8.

Can we please keep one bug per issue? Something about forests and trees and all that.

For separate issues, search (also for closed bugs please!) and file separate bugs if the issues haven't been filed and debugged already. Bonus points for reproducible testcases where you can show that the issue really isn't with the add-on. There's not really anything we can do about it otherwise.

If lastpass is still packing an old version of the SDK, for instance, the problem will likely go away if they stop doing that and rely on the SDK shipping in Firefox instead. The SDK bits were certainly what I recall the problem being last time this was reported (although it could be I'm misremembering, as there's too many of these and people keep adding comments to random other add-on-and-customization-related bugs).

(I'm also putting the abp tracker bug back on not because I think this is necessarily an ABP-caused issue but because it's certainly related (and that's what the tracker's for, AFAICT), and until someone's debugged the end of it I'm not placing bets on "whose fault" it is)
Blocks: abp
Status: NEW → UNCONFIRMED
Ever confirmed: false
Summary: On browser shutdown some of the add-ons buttons positions on Bookmark Toolbar are not saved correctly. → On browser shutdown ABP and Session Manager 'Undo Close' button positions on Bookmark Toolbar are not saved correctly.
Sure, but why you also mark this bug as unconfirmed... ;d
(In reply to Virtual_ManPL [:Virtual] from comment #11)
> Sure, but why you also mark this bug as unconfirmed... ;d

Because I don't know of anyone reproducing with the newest versions of ABP (the one that works with Australis) and session manager, and because the person moving it to NEW said they reproduced a different issue with different buttons?

I'd love some detailed steps to reproduce that work from a clean profile. :-)
Virtual_ManPL, could you please address the questions in comment 12 using the latest nightly/abp?
Flags: needinfo?(BernesB)
Sure thing, as stated in first post you need:
1. Create a new profile with latest Nightly from https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/
2, Install latest nightly Adblock Plus add-on from https://downloads.adblockplus.org/devbuilds/adblockplus/
3. Install latest nightly Element Hiding Helper for Adblock Plus add-on from https://downloads.adblockplus.org/devbuilds/elemhidehelper/ (don't know if it's needed, but installing it won't hurt)
4. Install latest nightly Session Manager add-on from https://addons.mozilla.org/en-US/firefox/addon/session-manager/versions/
5. Place Adblock Plus button and Session Manager button on the right side in bookmarks toolbar, Adblock Plus button on the right and Session Manager button on the left (you can also add the Download button to the far right like it's on screenshot)
6. Restart browser
7. Enjoy swapped button places

I hope it's clear now. :)
Flags: needinfo?(BernesB)
Thank you very much.
Confirmed in nightly 29.0a1 (2014-01-29), win 7 x64 using the STR in comment 14.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to Paul Silaghi, QA [:pauly] from comment #6)
> FWIW, it is known that the ABP icon disappears from the nav-bar after a
> Firefox restart.
This is no longer repro using latest nightly Adblock Plus add-on from https://downloads.adblockplus.org/devbuilds/adblockplus/
(In reply to Paul Silaghi, QA [:pauly] from comment #16)
> (In reply to Paul Silaghi, QA [:pauly] from comment #6)
> > FWIW, it is known that the ABP icon disappears from the nav-bar after a
> > Firefox restart.
> This is no longer repro using latest nightly Adblock Plus add-on from
> https://downloads.adblockplus.org/devbuilds/adblockplus/

That issue was fixed in Adblock Plus 2.4.1 which introduced proper Australis support. However, the Adblock Plus build mentioned in comment 3 already had this fix so nothing changed as far as the bug here is concerned.
(In reply to Paul Silaghi, QA [:pauly] from comment #16)
> (In reply to Paul Silaghi, QA [:pauly] from comment #6)
> > FWIW, it is known that the ABP icon disappears from the nav-bar after a
> > Firefox restart.
> This is no longer repro using latest nightly Adblock Plus add-on from
> https://downloads.adblockplus.org/devbuilds/adblockplus/

I can still reproduce it with latest Adblock Plus 2.4.1.3762
Alright, so here's what happens. Session manager uses a 'buttons.js' library. (CC-ing Erik Vold too, because his name is in the file)

It tries to use currentset to obtain a DOM node before which to insert its item. The relevant code goes something like this:


79       let before = null;
80       if (idx != -1) {
81         // Need to get ids for separators so can insert in front of them if need be.
82         let separators = toolbar.getElementsByTagName("toolbarseparator");
83         let j=0;
84         for (let i =0; ((i < currentset.length) && (j < separators.length)); ++i) 
85           if (currentset[i] == "separator")
86             currentset[i] = separators[j++].id
87 
88         // inserting the button before the first item in `currentset`
89         // after `idx` that is present in the document
90         for (let i = idx + 1; i < currentset.length; ++i) {
91           before = $(currentset[i]);
92           if (before) {
93             break;
94           }
95         }
96       }
97       toolbar.insertItem(button.id, before);
98       if (newInstall) {
99         toolbar.setAttribute("currentset", toolbar.currentSet);
100         doc.persist(toolbar.id, "currentset");
101       }


Where idx is the index in the toolbar's currentset, and currentset is the comma-split version of the currentset property/attribute.

Now, unfortunately, ABP inserts its toolbar button later than session manager. So what happens is, when it does:

91           before = $(currentset[i]);

before is null. It thus keeps before as null, which means the node gets inserted at the end of the toolbar by insertItem, which happily updates the CUI position database, and now it's been changed forever more (and ABP abides by the new info and sticks its toolbar button in the suggested spot).

I guess the simplest solution would be to, at the start of this whole function, to just do:

if ('CustomizableUI' in window) {
  if (CustomizableUI.getPlacementOfWidget(buttonId)) {
    CustomizableUI.ensureWidgetPlacedInWindow(buttonId);
    return;
  }
  // insert in default place - might be able to fall through to the current thing.
}

I'm moving this to tech evangelism :: Addons. Michael, do you want to take a look at this? :-)
Component: Extension Compatibility → Add-ons
Product: Firefox → Tech Evangelism
Version: 29 Branch → Trunk
I took a look at it and even using the CustomizableUI.getPlacementOfWidget, I still run into the same problem since CustomizableUI.ensureWidgetPlacedInWindow always returns false as the button isn't a true widget.  It doesn't matter if the button is already in the toolbar or not.

So other than completely converting over to the new toolbar API and converting my buttons into Widgets, I'm in the same exact place I was before.

I supposed I could try waiting until all addons have been enabled before adding Session Manager's buttons, but that doesn't really solve the underlying problem.
(In reply to Michael Kraft [:morac] from comment #20)
> I took a look at it and even using the CustomizableUI.getPlacementOfWidget,
> I still run into the same problem since
> CustomizableUI.ensureWidgetPlacedInWindow always returns false as the button
> isn't a true widget.  It doesn't matter if the button is already in the
> toolbar or not.

I don't really follow. ensureWidgetPlacedInWindow doesn't care if the button is a 'true widget' (do you mean an API-style widget?). Here's the code:

  ensureWidgetPlacedInWindow: function(aWidgetId, aWindow) {
    let placement = this.getPlacementOfWidget(aWidgetId);
    if (!placement) {
      return false;
    }
    let areaNodes = gBuildAreas.get(placement.area);
    if (!areaNodes) {
      return false;
    }
    let container = [...areaNodes].filter((n) => n.ownerDocument.defaultView == aWindow);
    if (!container.length) {
      return false;
    }
    let existingNode = container[0].getElementsByAttribute("id", aWidgetId)[0];
    if (existingNode) {
      return true;
    }

    let placementAry = gPlacements.get(placement.area);
    let nextNodeId = placementAry[placement.position + 1];
    this.insertNodeInWindow(aWidgetId, container[0], nextNodeId, true);
    return true;
  },

In what circumstance are you seeing this failing? Have you tried using the browser debugging to see why this is returning false?
Flags: needinfo?(morac99-firefox2)
(Sorry for the double post, I just noticed: my sample code in comment #19 left out the aWindow parameter - without that it won't work. Perhaps that is the problem?)
I was using the window parameter, it didn't make a difference.  I actually tested using the scratchpad to see what result I would get for ensureWidgetPlacedInWindow and it always returned false.

I did not debug it, but I can say that the first two checks would pass, so it must be the container check that's failing.  I didn't try the debugger as last time I used it, it wasn't working correctly.  If it's been fixed, I can try that.


I did come up with a work around.  I simply delay loading the toolbar buttons.  It's not the best solution, but it works.  Of course it could cause other addons to run into the same problem as I did, but once all add-ons move to the new CustomizableUI API that shouldn't be an issue.
Flags: needinfo?(morac99-firefox2)
Ah I missed the second parameter for ensureWidgetPlacedInWindow.   Once I added that, the buttons were added, but they still aren't added in the correct order so it didn't really help.

It actually made things slightly worse as the undo button was placed at the end of the toolbar instead of prior to the next available button.
(In reply to Michael Kraft [:morac] from comment #24)
> Ah I missed the second parameter for ensureWidgetPlacedInWindow.   Once I
> added that, the buttons were added, but they still aren't added in the
> correct order so it didn't really help.
> 
> It actually made things slightly worse as the undo button was placed at the
> end of the toolbar instead of prior to the next available button.

This is odd. Can you briefly describe STR so I can debug why this is broken, if you're not able to debug this yourself? (the browser debugger should work, I used it to get to comment #19 in the first place)
Looking at it, using ensureWidgetPlacedInWindow causes one of the two Session Manager toolbar buttons to get placed at the end of the toolbar, despite getWidgetIdsInArea and getPlacementOfWidget both indicating that they should be in the correct spot.

I'm not sure if that's a bug with ensureWidgetPlacedInWindow or something else.

> This is odd. Can you briefly describe STR so I can debug why this is broken,
> if you're not able to debug this yourself? (the browser debugger should
> work, I used it to get to comment #19 in the first place)

I would, but this only seems to happen at startup.  Is there a way to have the debugger kick off before all open windows?  I can't find any documentation stating how to do that, if it's possible.

Basically all  I did was copy and paste your code snippet after line 52 in button.js, positioned the buttons with the session manager first, undo second and ADP third and then closed Firefox and opened it (nightly load).
(In reply to Michael Kraft [:morac] from comment #26)
> Looking at it, using ensureWidgetPlacedInWindow causes one of the two
> Session Manager toolbar buttons to get placed at the end of the toolbar,
> despite getWidgetIdsInArea and getPlacementOfWidget both indicating that
> they should be in the correct spot.
> 
> I'm not sure if that's a bug with ensureWidgetPlacedInWindow or something
> else.
> 
> > This is odd. Can you briefly describe STR so I can debug why this is broken,
> > if you're not able to debug this yourself? (the browser debugger should
> > work, I used it to get to comment #19 in the first place)
> 
> I would, but this only seems to happen at startup.  Is there a way to have
> the debugger kick off before all open windows?  I can't find any
> documentation stating how to do that, if it's possible.
> 
> Basically all  I did was copy and paste your code snippet after line 52 in
> button.js, positioned the buttons with the session manager first, undo
> second and ADP third and then closed Firefox and opened it (nightly load).

Hrm. I noticed there were a couple of changes necessary to make the code actually work, wrt references to variables, not having a window and whatnot, and ended up inserting:


    let window = doc.defaultView;
    if ('CustomizableUI' in window) {
      if (window.CustomizableUI.getPlacementOfWidget(button.id)) {
        window.CustomizableUI.ensureWidgetPlacedInWindow(button.id, doc.defaultView);
        return;
      }
      // insert in default place - might be able to fall through to the current thing.
    }

This worked a charm, in my experience.
Looking at the insertNodeInWindow function in CustomizableUI, it looks like it suffers from the same problem.  It calls findInsertionPoints with the aNextNodeId which is the widget position grabbed from getPlacementOfWidget + 1.  findInsertionPoints does a getElementById on aNextNodeId  which will return null since the element does not exist yet.  

http://mxr.mozilla.org/projects-central/source/ux/browser/components/customizableui/src/CustomizableUI.jsm#928

The end result is it will call insertBefore with a null referenceElement, which explains would explain why it gets added at the end of the toolbar.
(In reply to :Gijs Kruitbosch from comment #27)
>     let window = doc.defaultView;
>     if ('CustomizableUI' in window) {
>       if (window.CustomizableUI.getPlacementOfWidget(button.id)) {
>         window.CustomizableUI.ensureWidgetPlacedInWindow(button.id,
> doc.defaultView);
>         return;
>       }
>       // insert in default place - might be able to fall through to the
> current thing.
>     }
> 

That's the exact code I tried and it didn't work.  See comment #27 for the reason why.
Err comment #28
(In reply to Michael Kraft [:morac] from comment #28)
> Looking at the insertNodeInWindow function in CustomizableUI, it looks like
> it suffers from the same problem.  It calls findInsertionPoints with the
> aNextNodeId which is the widget position grabbed from getPlacementOfWidget +
> 1.  findInsertionPoints does a getElementById on aNextNodeId  which will
> return null since the element does not exist yet.  
> 
> http://mxr.mozilla.org/projects-central/source/ux/browser/components/
> customizableui/src/CustomizableUI.jsm#928
> 
> The end result is it will call insertBefore with a null referenceElement,
> which explains would explain why it gets added at the end of the toolbar.

But there is a difference, which is that ensureWidgetInWindow doesn't save the ("wrong") placement information (currentset), which means that by the time the extra nodes get inserted (ie, ABP shows up and tries to insert its node), it should be possible to do that in the right place.

I'm still wondering what we're doing differently that's leading to different outcomes.

That said, if you had a placement list roughly like this:

exists,tryingtoinsertthis,doesntexist,existsalready

then the code clearly does the wrong thing, and insert at the end when we should be inserting before the 'existsalready' element. I'll file a separate bug on that.
(In reply to :Gijs Kruitbosch from comment #31)
> That said, if you had a placement list roughly like this:
> 
> exists,tryingtoinsertthis,doesntexist,existsalready
> 
> then the code clearly does the wrong thing, and insert at the end when we
> should be inserting before the 'existsalready' element. I'll file a separate
> bug on that.


Filed bug 976792.
(In reply to :Gijs Kruitbosch from comment #31)
> But there is a difference, which is that ensureWidgetInWindow doesn't save
> the ("wrong") placement information (currentset), which means that by the
> time the extra nodes get inserted (ie, ABP shows up and tries to insert its
> node), it should be possible to do that in the right place.

Session Manager isn't saving the currentset either, except on the case of a new install, which isn't the case here.  For reasons I haven't determined yet, currentset is saving automatically any time a toolbar item gets inserted into the toolbar using the toolbar.insertItem function.


In any case, I've fixed this issue in my latest development version using an alternate method.
Is there anything left to do in this bug?
(In reply to Jorge Villalobos [:jorgev] from comment #34)
> Is there anything left to do in this bug?

I don't believe so, as Michael said he fixed it, and we fixed bug 976792, so this should work now.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Component: Add-ons → General
Product: Tech Evangelism → WebExtensions
You need to log in before you can comment on or make changes to this bug.