Ubuntu
firefox package

globalmenu extension pollutes main window javascript scope

Bug #767966 reported by Nils Maier
12
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Global menubar extension
Status tracked in Trunk
1.0
Fix Released
High
Chris Coulson
Global menubar extension 1.0.3
Trunk
Fix Released
Medium
Unassigned
Global menubar extension 2.0
firefox (Ubuntu)
Fix Released
High
Chris Coulson
Ubuntu natty-updates
Natty
Fix Released
High
Chris Coulson
Ubuntu natty-updates
Oneiric
Fix Released
Medium
Unassigned
thunderbird (Ubuntu)
Fix Released
High
Chris Coulson
Ubuntu natty-updates
Natty
Fix Released
High
Chris Coulson
Ubuntu natty-updates
Oneiric
Fix Released
Medium
Unassigned

Bug Description

REASON FOR SRU:
Polluting the global namespace isn't friendly and will break either the globalmenu extension or other extensions that people install.

--------------------------------------------------

Binary package hint: firefox

The globalmenu extension overlays browser.xul and loads firefoxMenu.js, adding a lot of new variables/functions to the shared scope, at least:
nsIObserverService !
uIGlobalMenuLoader
uIGlobalMenuService
observer !
Observer() !
onLoad() !
onUnload() !
PlacesMenuUnityImpl()
HistoryMenuUnityImpl()
(! marks names with high chance of conflicts)

This is a problem as the regular browser code, all other extensions overlaying the browser window and this extension share the same scope, i.e. the same namespace.
If other extensions happen to define the same scope-global names, then conflicts will occur. Whoever loads last wins. Hence there is a high probability of heisenbugs (depending on the other extensions a user installs).
I'm a volunteer addons.mozilla.org editor, performing required code reviews an extension, this would be a definite show-stopper bug to any public listing.

You can either "hide" your stuff within a function, use some "namespace" variable, or a mixture of both, e.g.:
if (!com) var com = {};
if (!com.canonical) com.canonical = {};
(function() {
  // local/"hidden" stuff
  function load() {
    removeEventListener("load", load, false);
    ...
  }
  addEventListener("load", load, false);

  // globally visible
  this.Unity = { // |this| is com.canonical
    PlacesMenuUnityImpl: ...
  };
}).call(com.canonical);

(in the xul overlay you would then change all references to PlacesMenuUnityImpl to com.canonical.Unity.PlacesMenuUnityImpl)

Also see:
https://developer.mozilla.org/en/XUL_School/JavaScript_Object_Management

See original description
Revision history for this message
Nils Maier (testnutzer123) wrote :

Ok, some code for you...
This is my first real launchpad and bzr attempt, so please bear with me ;)

Revision history for this message
Chris Coulson (chrisccoulson) wrote :

Good catch, thanks!

I've not had time to review it properly yet, but just at first glance, this looks wrong to me:

--- extensions/globalmenu/chrome/content/firefoxOverlay.xul 2011-04-16 19:37:53 +0000
+++ extensions/globalmenu/chrome/content/firefoxOverlay.xul 2011-04-21 02:18:45 +0000
@@ -38,13 +38,4 @@

 <overlay id="globalmenu-extension-browser-overlay" xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
   <script type="application/x-javascript" src="chrome://globalmenu/content/firefoxMenu.js" />
-
- <menupopup id="goPopup"
- onpopupshowing="if (!this.parentNode._placesView)
- new HistoryMenuUnityImpl(event);"/>
-
- <menupopup id="bookmarksMenuPopup"
- onpopupshowing="PlacesCommandHook.updateBookmarkAllTabsCommand();
- if (!this.parentNode._placesView)
- new PlacesMenuUnityImpl(event, 'place:folder=BOOKMARKS_MENU');"/>
 </overlay>

I overlay this deliberately to override the default popupshowing handlers (because I'm providing my own implementation of PlacesMenu and HistoryMenu, called PlacesMenuUnityImpl and HistoryMenuUnityImpl). Without this, the bookmarks and history menu break in weird ways because Firefox *thinks* it is drawing the menus itself, and expects frames to exist and XBL bindings to have been loaded. The implementations I provide put the menus in to the same state they would be on Mac OSX (where there are no menu frames too).

You might want to have a look in browser/base/content/browser-menubar.inc and browser/components/places/content/browserPlacesViews.js to see why it is done this way.

Chris Coulson (chrisccoulson)
Changed in globalmenu-extension:
importance: Undecided → High
status: New → Triaged
affects: firefox (Ubuntu) → ubuntu
affects: Ubuntu Natty → firefox (Ubuntu Natty)
Changed in thunderbird (Ubuntu Natty):
importance: Undecided → High
status: New → Triaged
Changed in thunderbird (Ubuntu Oneiric):
status: New → Triaged
importance: Undecided → Medium
Changed in thunderbird (Ubuntu Natty):
milestone: none → natty-updates
Changed in firefox (Ubuntu Natty):
assignee: nobody → Chris Coulson (chrisccoulson)
Changed in thunderbird (Ubuntu Natty):
assignee: nobody → Chris Coulson (chrisccoulson)
Revision history for this message
Nils Maier (testnutzer123) wrote :

Yeah, I unfortunately didn't get that this was an deliberate onpopupshowing override.
I pushed a fix that uses another approach to the same branch: Just override the original object ctor, and try to be nice when doing so.

Revision history for this message
Chris Coulson (chrisccoulson) wrote :

Excellent, thanks. It seems to work ok so I'll merge that in to trunk now

Chris Coulson (chrisccoulson)
Changed in firefox (Ubuntu Oneiric):
status: Triaged → Fix Committed
Changed in firefox (Ubuntu Natty):
status: Triaged → Fix Committed
Chris Coulson (chrisccoulson)
Changed in thunderbird (Ubuntu Oneiric):
status: Triaged → Fix Committed
Changed in thunderbird (Ubuntu Natty):
status: Triaged → Fix Committed
Micah Gersten (micahg)
description: updated
Revision history for this message
Clint Byrum (clint-fewbar) wrote :

Per micahg's request I took a close look at this specific change:

http://bazaar.launchpad.net/~mozillateam/firefox/firefox-4.0.natty/revision/843

For its SRU worthiness. I think it should be fine. There is an infinitesimal risk of regression if some user actually tries to depend on the bits of the global menu that are in the namespace, but since its not a published API, they shouldn't be doing that.

So, +1 for SRU to natty.

Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package firefox - 4.0.1+build1+nobinonly-0ubuntu0.11.04.1

---------------
firefox (4.0.1+build1+nobinonly-0ubuntu0.11.04.1) natty-security; urgency=low

  * New upstream release v4.0.1 (FIREFOX_4_0_1_BUILD1)
    - see USN-1121-1

  * Fix LP: #767966 - globalmenu extension pollutes main window javascript
    scope
    - update globalmenu-extension to 1.0.3
 -- Chris Coulson <email address hidden> Fri, 22 Apr 2011 15:12:46 -0500

Changed in firefox (Ubuntu Natty):
status: Fix Committed → Fix Released
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package thunderbird - 3.1.10+build1+nobinonly-0ubuntu0.11.04.1

---------------
thunderbird (3.1.10+build1+nobinonly-0ubuntu0.11.04.1) natty-security; urgency=low

  * New upstream release v3.1.10 (THUNDERBIRD_3_1_10_BUILD1)
    - see USN-1122-2

  * Fix LP: #767966 - globalmenu extension pollutes main window javascript
    scope
    - update globalmenu-extension to 1.0.3
 -- Chris Coulson <email address hidden> Sun, 24 Apr 2011 03:21:04 -0500

Changed in thunderbird (Ubuntu Natty):
status: Fix Committed → Fix Released
Revision history for this message
Micah Gersten (micahg) wrote :

Asked pitti to copy from natty-security to oneiric

Changed in firefox (Ubuntu Oneiric):
status: Fix Committed → Fix Released
Changed in thunderbird (Ubuntu Oneiric):
status: Fix Committed → Fix Released
Revision history for this message
Chris Coulson (chrisccoulson) wrote :

Note that this is causing bug 777619 and all sorts of other weirdness with thunderbird (I've not figured out why yet)

Revision history for this message
Chris Coulson (chrisccoulson) wrote :

Huh:

I get "document.getElemenyById is not a function" in maybeMoveSpinner(). And, sure enough, the document object doesn't have this function defined (pretty much every single function is listed as undefined according to chromebug).

Revision history for this message
Chris Coulson (chrisccoulson) wrote :

Ignore my last comment, I think chromebug was lying.

Basically what is happening is that the observer in thunderbirdMenu.js never fires when the menu service comes online, so the code which moves the spinner from the menubar to the toolbar is never called

Revision history for this message
Chris Coulson (chrisccoulson) wrote :

Ah, thanks to Mike Conley who spotted that we are registering 2 load handlers, and 1 of them is meant to be an unload handler :)

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.
Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.