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

UNCONFIRMED
Unassigned

Status

defect
UNCONFIRMED
3 years ago
9 months 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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)
Posted file 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

3 years 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

3 years 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?
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

3 years 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

3 years 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?
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

3 years 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.
Reporter

Comment 26

3 years 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

3 years ago
Whiteboard: [addon:QuoteAndComposeManager]
Component: Untriaged → Add-Ons: General
You need to log in before you can comment on or make changes to this bug.