Last Comment Bug 832401 - Ghost Context menu item appears with specific set of add-ons installed
: Ghost Context menu item appears with specific set of add-ons installed
Status: RESOLVED FIXED
:
Product: Add-on SDK
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-18 10:36 PST by Jeff Griffiths (:canuckistani) (:⚡︎)
Modified: 2013-01-30 20:40 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
screenshuot of bug (45.84 KB, image/png)
2013-01-18 10:36 PST, Jeff Griffiths (:canuckistani) (:⚡︎)
no flags Details
xpi to repro with (144.32 KB, application/x-xpinstall)
2013-01-18 10:37 PST, Jeff Griffiths (:canuckistani) (:⚡︎)
no flags Details
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/746 (355 bytes, text/html)
2013-01-25 16:11 PST, Dave Townsend [:mossop]
evold: review+
Details

Description Jeff Griffiths (:canuckistani) (:⚡︎) 2013-01-18 10:36:24 PST
Created attachment 703972 [details]
screenshuot of bug

To repro:

1. install Evernote clearly: https://addons.mozilla.org/en-US/firefox/addon/clearly/
2. install Agilebits 1Password: https://agilebits.com/extensions/mac/index.html ( this could perhaps be any SDK-based add-on that uses the old context-menu module )
3. install the attached add-on that is built with Mossop's new module, including the fixes from bug 831643.

Result: a ghost / blank item appears in between the clearly item and the item from the attached xpi.
Comment 1 Jeff Griffiths (:canuckistani) (:⚡︎) 2013-01-18 10:37:34 PST
Created attachment 703974 [details]
xpi to repro with
Comment 2 Dave Townsend [:mossop] 2013-01-25 15:29:04 PST
We may want to re-evaluate this. The clearly extension isn't needed and I can reproduce it with 1Password and your test add-on. Unfortunately 1Password is minimized so it's tricky to see what is going on but I can make some guesses and this is an incompatiblity with the old/new context-menu modules.

The problem is that, frankly, the old module is pretty dumb. When you create a menuitem with it it attempts to put it into the context menu based on sort order against the other items created with the module. So it gets a list of all the menu items and figures out where in that list to put it. If it wants to add the new item to the end of the list (e.g. for the first item) then it adds it to the end of the context menu, ignoring the fact that other add-ons could have added other items after its separator, so you end up with:

----------------   old module's separator
----------------   new module's separator
Bug 831643         new module's menu item
Unlock 1Password   old module's menu item

Sometimes you won't see the bug. You will see it if an add-on using the old module adds a menu item and then delays (allowing a new-module add-on to initialise) before adding more menu items. Or if you have two add-ons using the old module and an add-on using the new module initialises between them and the menu items are named right.

There is a potential fix (at least for the new module, there is nothing to be done about the fact that the old module has probably been showing this kind of breakage against other add-ons all the time). I can make the module inject its separator before the separator for the old module if it exists. The new module is smarter and regardless of what else adds stuff to the menu it should keep all its items grouped together.
Comment 3 Dave Townsend [:mossop] 2013-01-25 16:11:32 PST
Created attachment 706655 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/746

Pointer to Github pull-request
Comment 4 Dave Townsend [:mossop] 2013-01-25 16:13:07 PST
Attached a fix. I think since this is a purely cosmetic problem we probably shouldn't respin 1.13 for it. Should definitely take it in 1.14 and if we do a 1.13.1 for another reason then there too.

What do you think Jeff?
Comment 5 Jeff Griffiths (:canuckistani) (:⚡︎) 2013-01-25 16:24:28 PST
(In reply to Dave Townsend (:Mossop) from comment #4)
> Attached a fix. I think since this is a purely cosmetic problem we probably
> shouldn't respin 1.13 for it. Should definitely take it in 1.14 and if we do
> a 1.13.1 for another reason then there too.
> 
> What do you think Jeff?

Agreed. Let's land it for 1.14.
Comment 6 Erik Vold [:erikvold] (please needinfo? me) 2013-01-30 15:26:11 PST
(In reply to Jeff Griffiths (:canuckistani) from comment #1)
> Created attachment 703974 [details]
> xpi to repro with

do you have this source code posted some place so I can rebuild it with the patch?
Comment 7 Jeff Griffiths (:canuckistani) (:⚡︎) 2013-01-30 16:21:49 PST
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #6)
> (In reply to Jeff Griffiths (:canuckistani) from comment #1)
> > Created attachment 703974 [details]
> > xpi to repro with
> 
> do you have this source code posted some place so I can rebuild it with the
> patch?

https://github.com/canuckistani/bug_832401_test
Comment 8 Wes Kocher (:KWierso) 2013-01-30 16:23:55 PST
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #6)
> (In reply to Jeff Griffiths (:canuckistani) from comment #1)
> > Created attachment 703974 [details]
> > xpi to repro with
> 
> do you have this source code posted some place so I can rebuild it with the
> patch?

If you extract the xpi file (it's just a renamed .zip), the source is in resources/bug_831643
Comment 9 [github robot] 2013-01-30 18:59:26 PST
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/990a49a59a37825896665cf79eca870521fbc7ba
Bug 832401: Put the new context-menu separator before the old one.

https://github.com/mozilla/addon-sdk/commit/bad47e93ee8b0cf2afa95494ada53b056f9ba497
Merge pull request #746 from Mossop/bug832401

Bug 832401: Put the new context-menu separator before the old one. r=@erikvold
Comment 10 [github robot] 2013-01-30 19:25:18 PST
Commit pushed to stabilization at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/74bc46081b80651a9d1a60057921af3d8b68022e
Bug 832401: Put the new context-menu separator before the old one.
(cherry picked from commit 990a49a59a37825896665cf79eca870521fbc7ba)
Comment 11 [github robot] 2013-01-30 20:38:23 PST
Commit pushed to release at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/1836472e0ccb08681647d697b2c3494b24c8a7c4
Bug 832401: Put the new context-menu separator before the old one.
(cherry picked from commit 990a49a59a37825896665cf79eca870521fbc7ba)
(cherry picked from commit 74bc46081b80651a9d1a60057921af3d8b68022e)
Comment 12 [github robot] 2013-01-30 20:40:53 PST
Commit pushed to stabilization at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/1836472e0ccb08681647d697b2c3494b24c8a7c4
Bug 832401: Put the new context-menu separator before the old one.

Note You need to log in before you can comment on or make changes to this bug.