1253933 - unable to overwrite functions in addons with dom.compartment_per_addon = true

Status: RESOLVED INVALID

(Thunderbird :: Add-Ons: General, defect)

Description

[intendentedelleacque]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20160210125511

Steps to reproduce:

In many of my extension, I use this code to "extend" native javascript functions:

var nativeFunctionCopy = nativeFunc();
var nativeFunc = function() {
   nativeFunctionCopy.apply();
   <my code>
};




Actual results:

From Thunderbird 45 it dosen't work anymore, because "nativeFunction" in my example above results "undefined". 

I couldn't find any workaround against this and this is a big problem, because it breaks a lot of my functions. 

Why does this happen?

Comment 1

[intendentedelleacque]

Sorry, I made a typo, this is the correct one:
From Thunderbird 45 it dosen't work anymore, because "nativeFunctionCopy" in my example above results "undefined".

Comment 2

[intendentedelleacque]

Is maybe a side unwanted effect of https://bugzilla.mozilla.org/show_bug.cgi?id=995610 ?

Comment 3

[Tooru Fujisawa [:arai]]

Can you provide the actual code instead of pseudo code?

Comment 4

[Tooru Fujisawa [:arai]]

also, bug 995610 just adds a console warning, it shouldn't affect the behavior.

Comment 5

[Jim Porter (:squib)]

(In reply to intendentedelleacque from comment #0)
> var nativeFunctionCopy = nativeFunc();
> var nativeFunc = function() {
>    nativeFunctionCopy.apply();
>    <my code>
> };

This code is wrong and it should *never* have worked. Line 1 should read:

> var nativeFunctionCopy = nativeFunc;

It's not necessary to use `apply` in this case either; simply call the function directly.

Comment 6

[intendentedelleacque]

(In reply to Jim Porter (:squib) from comment #5)
> (In reply to intendentedelleacque from comment #0)
> > var nativeFunctionCopy = nativeFunc();
> > var nativeFunc = function() {
> >    nativeFunctionCopy.apply();
> >    <my code>
> > };
> 
> This code is wrong and it should *never* have worked. Line 1 should read:
> 
> > var nativeFunctionCopy = nativeFunc;

Of course this is my code, I just made another typo:

var nativeFunctionCopy = nativeFunc;
var nativeFunc = function() {
   nativeFunctionCopy.apply();
   <my code>
};

On TB45 beta, both nativeFunctionCopy and nativeFunc are returned as "undefined" after that.
I will provide later a minimal xpi test file.

Comment 7

[intendentedelleacque]

I couldn't prepare yet a minimal xpi, because the original code is complex.
But here is what happens:

setTimeout(function() {alert(OnLoadCardView)}, 1000); --> returns the original TB function
var OnLoadCardViewOriginal8848 = OnLoadCardView;
setTimeout(function() {alert(OnLoadCardView8848)}, 1000); --> returns "undefined"

The setTimeout is used because is a loading window.

Comment 8

[intendentedelleacque]

No sorry please don't consider it, there is a typo :(
This is the real code:

var OnLoadCardViewOriginal8848 = OnLoadCardView;
var OnLoadCardView = function() {
   OnLoadCardViewOriginal8848.apply(); --> this is the line 69
   ....
};

returns JavaScript error: chrome://morecols/content/abCardViewOverlayOverlay.js, line 69: TypeError: OnLoadCardViewOriginal8848 is undefined

Same code on TB38 is working perfectly

Comment 9

[Tooru Fujisawa [:arai]]

1. is the original code available anywhere?
2. where/when is the |var OnLoadCardViewOriginal8848 = OnLoadCardView;| assignment executed?

Comment 10

[intendentedelleacque]

Maybe I found a workaround: the above code fails, but this one seems work as expected:

var OnLoadCardViewOriginal8848 = OnLoadCardView;
OnLoadCardView = function() {
   OnLoadCardViewOriginal8848.apply(); --> this is the line 69
   ....
};

Why now does "var" make it fail?

Comment 11

[intendentedelleacque]

(In reply to Tooru Fujisawa [:arai] from comment #9)
> 1. is the original code available anywhere?
> 2. where/when is the |var OnLoadCardViewOriginal8848 = OnLoadCardView;|
> assignment executed?

1. The original code is here: https://freeshell.de/~kaosmos/morefunctionsforAB-TB3-0.7.3.xpi (in the file abCardViewOverlayOverlay.js

2. when the addressbook main window is loaded

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: test.xpi

here's minimal testcase
(I used |openLink| function instead of |OnLoadCardView| to minimize the breakage)

Steps to reproduce:
  1. run Thunderbird beta 45.0
  2. open Addon Manager
  3. install attached test.xpi
  4. restart Thunderbird
  5. open Error Console
  6. open Address Book

Result:
  Error Console shows following message, where |undefined| is the value of openLink in script in the overlay:

    openLink: undefined


[test.xul]

<?xml version="1.0"?>
<overlay xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" xmlns:html="http://www.w3.org/1999/xhtml">
<script type="application/javascript">
Components.utils.reportError("openLink: " + openLink);
var openLink = function() { alert("test"); };
</script>
</overlay>

[chrome.manifest]

content test jar:chrome/test.jar!/content/test/
overlay chrome://messenger/content/addressbook/addressbook.xul chrome://test/content/test.xul

Comment 13

[Tooru Fujisawa [:arai]]

it's caused by the behavior difference from dom.compartment_per_addon pref value.

the pref value is set to true by default on nightly by bug 1030420 (version 35).
and set to true by default on all branches by bug 1236754 (version 45).

flipping the pref value to false changes the result of comment #12 to show actual openLink function instead of undefined.

Comment 14

[intendentedelleacque]

Thanks a lot for the explanation.
 
So basically now, with the perfect setting to true, each extension runs in a separate environment and can't declare global variables valid outside it?

Comment 15

[intendentedelleacque]

*perfect setting = pref set

Comment 16

[Kent James (:rkent)]

I think we need to understand the implications of this for addon compatibility in TB45. Should we be flipping this to dom.compartment_per_addon = false in TB 45?

Comment 17

[Jim Porter (:squib)]

For what it's worth, I perform very similar overrides in Mail Summaries, and it's worked on nightlies this whole time. I expect there's another issue here.

Comment 18

[Tooru Fujisawa [:arai]]

Global variable declared in add-on script is still visible from Error Console

Components.utils.import("resource://gre/modules/Services.jsm", {}).Services.wm.getMostRecentWindow("mail:addressbook").openLink
> function () { alert("test"); }

Comment 19

[intendentedelleacque]

I'm pretty sure that there other extensions, over mine, that use that syntax above (with the "double var" declaration).

What does it happen now if two extensions overwrite the same function?
With the old setting (dom.compartment_per_addon = false), both the overwrites are executed, is it the same with the pref set to true?

Comment 20

[intendentedelleacque]

(In reply to Jim Porter (:squib) from comment #17)
> For what it's worth, I perform very similar overrides in Mail Summaries, and
> it's worked on nightlies this whole time. I expect there's another issue
> here.

Your code is different, that's why it works properly.

Comment 21

[Kent James (:rkent)]

My understanding of this preference is that it is largely needed for e10s, which we do not use in Thunderbird. Although like squib I also do a lot of function overrides yet have not experienced any issues, I do wonder what we gain by leaving it at default if it will break some extensions.

So the proposal we could consider would be to flip this extension to the old value for TB 45 only, but leave it to the new value for TB 46 and later. Are there any downsides to doing that? The upside is that we might eliminate some broken extensions. Could anyone speak either for or against this proposal?

Comment 22

[Magnus Melin [:mkmelin]]

Don't have a strong preference, but one slight downside is of course "putting off the inevitable": maybe unbreak one some old extension, but maybe have new ones break the next cycle.

Comment 23

[Wayne Mery (:wsmwk)]

At this late stage, if addon authors aren't generally aware of the change, or there's good possibility some "abandoned" addons will break, or we haven't asked users to hammer addons (which we haven't), I'd say model the old behavior.

Comment 24

[Kent James (:rkent)]

I don't really understand what exactly causes the broken behavior, does anyone else? Comment 19 mentions 'with the "double var" declaration' so is it redeclaring a global with a var at global scope that breaks? If so, that is pretty trivial to fix in an addon.

Comment 25

[intendentedelleacque]

The fixes are surely easy to do, it's just needed a clear documentation about this change, because otherwise it's very difficult (or even impossible) to detect.

To all appearances it seems contrary to the javascript syntax: you declare a global variable and then you get an error saying that variable is undefined, this is quite confusing.

Comment 26

[intendentedelleacque]

I'm finding out that this change has a deeper impact than expected: for example, with this extension https://freeshell.de/~kaosmos/quoteandcomposemanager-en.html installed, to send a message will fail because of the error "msgWindow is undefined", coming from the original Thunderbird function!

Comment 27

[Denis Jones]

This reproduction is based on a dummy app but it's also reproducible within a host app https://www.liteblue.one/

Comment 28

[John Bieling (:TbSync)]

A lot has changed since 5 years ago and currently there is no "compartment_per_addon" pref anymore, e10s has been enabled for Thunderbird and we have seen and resolved many issues related to that. With WebExtensions, the mechanism of overriding global functions is not supported anymore and if add-on developers run into issues creating their own Experiments, we will try to figure out what can be done at discuss.thunderbird.net.

Closing this bug as it no longer applies.