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

UNCONFIRMED
Unassigned

Status

Thunderbird
Untriaged
UNCONFIRMED
a year ago
a year ago

People

(Reporter: intendentedelleacque, Unassigned)

Tracking

45 Branch

Thunderbird Tracking Flags

(thunderbird_esr45?)

Details

(Whiteboard: [addon:QuoteAndComposeManager])

Attachments

(1 attachment)

1.21 KB, application/x-xpinstall
Details
(Reporter)

Description

a year ago
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?
(Reporter)

Comment 1

a year ago
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".
(Reporter)

Comment 2

a year ago
Is maybe a side unwanted effect of https://bugzilla.mozilla.org/show_bug.cgi?id=995610 ?
Can you provide the actual code instead of pseudo code?
Flags: needinfo?(intendentedelleacque)
also, bug 995610 just adds a console warning, it shouldn't affect the behavior.
(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.
(Reporter)

Comment 6

a year ago
(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.
Flags: needinfo?(intendentedelleacque)
(Reporter)

Comment 7

a year ago
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.
(Reporter)

Comment 8

a year ago
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
1. is the original code available anywhere?
2. where/when is the |var OnLoadCardViewOriginal8848 = OnLoadCardView;| assignment executed?
Flags: needinfo?(intendentedelleacque)
(Reporter)

Comment 10

a year ago
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?
(Reporter)

Comment 11

a year ago
(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
Flags: needinfo?(intendentedelleacque)
Created attachment 8728049 [details]
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
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.
(Reporter)

Comment 14

a year ago
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?
(Reporter)

Comment 15

a year ago
*perfect setting = pref set
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?
tracking-thunderbird45: --- → +
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.
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"); }
(Reporter)

Comment 19

a year ago
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?
(Reporter)

Comment 20

a year ago
(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.
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

a year ago
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.
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.
Summary: unable to overwrite functions in addons → unable to overwrite functions in addons with dom.compartment_per_addon = true
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.
(Reporter)

Comment 25

a year ago
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.

Updated

a year ago
tracking-thunderbird45: + → ---
tracking-thunderbird_esr45: --- → ?
(Reporter)

Comment 26

a year ago
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!

Updated

a year ago
Whiteboard: [addon:QuoteAndComposeManager]
You need to log in before you can comment on or make changes to this bug.