727390 - Tab Mix Plus has the tendency to break things badly

Status: RESOLVED WONTFIX

(addons.mozilla.org Graveyard :: Policy, defect)

Description

[info]

A bunch of users complained that URL Fixer 4.0 completely breaks the browser's location bar. After being unable to reproduce the issue or recognize a problematic scenario in my code I guessed that another add-on is messing things up - and Tab Mix Plus immediately came to my mind. Turns out that my guess was correct. I released URL Fixer 4.0.1 with some code to work around Tab Mix Plus issues.

Tab Mix Plus heavily uses the anti-pattern listed under http://adblockplus.org/blog/five-wrong-reasons-to-use-eval-in-an-extension as "5. Rewriting browser’s functions". It takes a built-in function, stringifies it, changes it with some regular expressions and recompiles using eval(). Now if another extension does the same thing properly (like URL Fixer) it will replace the original function by a closure - and recompiling a closure results in a broken function because external variables cannot be found. And then it doesn't help that URL Fixer took extra precautions to make sure that the original function is always called, browser functionality will still be broken.

Searching Tab Mix Plus source code for Tabmix.newCode (the function doing these manipulations) brings up 125 hits - this is asking for trouble. Should an extension like this really be featured? I would rather expect a big warning: "This extension can break your browser badly if your environment deviates from the one that it expects".

Comment 1

[info]

Tab Utilities is another featured extension taking the same approach and also causing the same problem.

Comment 2

[Jorge Villalobos [:jorgev] (he/him)]

We allow the eval splicing technique for tab enhancement add-ons, given that the Firefox tab code is mostly composed of very large and monolithic functions that can't be easily modified in other ways. If there are better ways for multiple add-ons to tap into the particular functions your add-on needs, we can look into that and tell add-on developers to change their code. However, as you're describing things it sounds like either all need to play nice or they all need to use eval.

Comment 3

[info]

Here is what I suggested in my communication with onemen:

> I tried to figure out what a proper solution would look like in your case.
> Unfortunately, the code is full of "work-arounds" and it is unclear what the
> actual indent is here. From what it looks like, the point of the whole
> exercise is to control where a new link will be opened. The best solution
> seems to be hooking the functions used to actually open the link. For Firefox
> 4 they are openUILink, gBrowser.loadOneTab and loadURI; Firefox 10 uses
> openUILinkIn and gBrowser.loadURIWithFlags. You only need to intercept the
> calls while in handleCommand, that would work like this (here only
> oldOpenUILinkIn is being intercepted):
>
> let oldHandleCommand = gURLBar.handleCommand;
> gURLBar.handleCommand = function(event)
> {
>   let oldOpenUILinkIn = window.openUILinkIn;
>   window.openUILinkIn = function(url, where, params)
>   {
>     params.inBackground = TabmixSvc.TMPprefs.getBoolPref("loadUrlInBackground");
>     // Check whether the default behavior needs to be overwritten
>     if (...)
>       gBrowser.loadURIWithFlags(...);
>     else
>       oldOpenUILinkIn.apply(this, arguments);
>   };
>   try
>   {
>     return oldHandleCommand.apply(this, arguments);
>   }
>   finally
>   {
>     window.openUILinkIn = oldOpenUILinkIn;
>   }
> };
> 
> Of course, the real code would be more complicated - you would have to hook
> more functions (particularly if you want to keep supporting Firefox 4) and add
> the decision logic. Still, it would probably be simpler than all the
> work-arounds you currently have there. And it would definitely be more robust.

Unfortunately, this approach was rejected:

> i don't like your solution to replace global function with private variable
> it prevent other extensions from working with the same function

Which is a bogus reason given that the way extensions currently "work with the same function" is by being aware of every single extension that touches that function. Tab Mix Plus has work-arounds for literally dozens of other extensions - the approach is extremely fragile and tends to cause catastrophic failure whenever an assumption fails. Chaining closures has its own issues of course (most importantly the assumption that no extension will be disabled during a browser session) but not nearly as bad.

Comment 4

[Jorge Villalobos [:jorgev] (he/him)]

Adding TMP dev for his opinion.

Comment 5

[tabmix.onemen]

i'm sure that we can find a way to enable all extensions to work together.
but we need to work together.

when we apply eval on modified function that have private code in it. (like URL Fixer do) the eval will brake.

i offered to URL Fixer developer to change 3 line in the code to prevent this problem

>  URL Fixer version 4.0.1, break Tabmix address bar functionality by blocking Tabmix changes to gURLBar.handleCommand.
>  i have no problem to change gURLBar.handleCommand.urlfixerOldHandler, but since you are using closure when calling oldHandler.apply(this, arguments);
>  
>  my changes have no effect (i apply these changes after your extensions replaced the original handleCommand)
>  
>  the simplest fix will be if you can change your code a bit and call urlfixerOldHandler from handleCommand
>  
>  - remove this line
>  + add this line
>  
>          exports.applyToWindow = function(window, corrector)
>          {
>              let urlbar = exports.getURLBar(window);
>              if (urlbar && urlbar.handleCommand && !("urlfixerOldHandler" in urlbar.handleCommand))
>              {
>                  // Handle new URLs being entered
>  -               let oldHandler = urlbar.handleCommand;
>  +              urlbar.handleCommand.urlfixerOldHandler = urlbar.handleCommand;
>                  urlbar.handleCommand = function()
>                  {
>                      try
>                      {
>                          let correction = corrector(window, urlbar.value);
>                          if (correction)
>                              urlbar.value = correction;
>                      }
>                      catch(e)
>                      {
>                          if (e == Cr.NS_BINDING_ABORTED)
>                              return;
>                          else
>                              Cu.reportError(e);
>                      }
>  -                   oldHandler.apply(this, arguments);
>  +                  this.handleCommand.urlfixerOldHandler.apply(this, arguments);
>                  }
>  -               urlbar.handleCommand.urlfixerOldHandler = oldHandler;
>                  urlbar.handleCommand.toString = doNotRecompile;
>              }

if this fix apply to URL Fixer then tabmix can check if handleCommand.urlfixerOldHandler exist and apply its changes to it instead of handleCommand

the fix i apply to the latest Tabmix development build was to apply tabmix changes to handleCommand before URL Fixer replace it.

Comment 6

[tabmix.onemen]

look like URL Fixer approach brake also Omnibar

Comment 7

[Jorge Villalobos [:jorgev] (he/him)]

I don't think there's a way to future-proof code that is spliced into Firefox code. We have generally accepted the eval solution because there are few good alternatives, and they aren't really better when it comes to interacting with other add-ons that may or may not use the same approach.

There's a larger problem that needs to be fixed here, which is that certain parts of the Firefox code is too monolithic to extend without these hacks. It's something that I have been giving some thought since before I joined Mozilla, and I want to tackle in the future, but the present keeps getting in the way. It's one my radar, though, and I hope I can work on it this year.

I don't think there's anything that can be done in this particular case.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Jorge Villalobos [:jorgev] from comment #7)
> There's a larger problem that needs to be fixed here, which is that certain
> parts of the Firefox code is too monolithic to extend without these hacks.

Agreed. If there are obvious wins here, file bugs. We know there's a lot of old code in browser.js and tabbrowser especially that needs cleaning up - if there's a list of specific stuff to target that would also help addon authors write reliable future-proof code, it makes it easier to prioritize that work.