1417209 - Thunderbird 57.0b1 hangs before any windows come up with any of numerous add-ons (using type="application/javascript;version=" in their code)

Status: VERIFIED FIXED

(Core :: General, defect)

Description

[Eyal Rozenberg]

Remove Duplicate Messages (Alternate) is a relatively-popular Thunderbird extension for doing, well, you can guess what:

https://addons.mozilla.org/en-US/thunderbird/addon/remove-duplicate-messages-alte/

It doesn't do anything "crazy" as extensions go; and it's compatible with Thunderbird up to the latest 56 beta. However, if you try to run Thunderbird 57.0b1 with it - it hangs, not even emitting any console output. I tried this with a (mostly) clean profile after disabling other extensions.

Being the extension author, I've gotten similar reports from users. For now this appears to happen at least for x86_64 + Linux and i686 + Windows.

Comment 1

[:aceman]

Hi. I use the addon and have looked at the code. I can confirm it prevents main TB window to open even with TB59.
So in the code I found:
1. It uses nsIScriptableDateFormat, which was removed, see bug 1346549.
2. It uses the no longer supported construct "catch (ex if ex instanceof ExceptionType)", see bug 1251419
3. StopIteration object is also away, bug 1413867, albeit in later TB versions.
4. I found statement "MsgService .streamMessage(msgURI, MsgStream, msgWindow, null, false, null);". Why is there a space before dot?

5. In xul files, JS files are linked in with <script type="application/x-javascript" ... > OR <script type="application/x-javascript;version=1.7" ... > . Removing the "x-" prefix and removing the javascript version actually made TB start up properly. So this one is the critical point, after which you may fix the other problems. This may be bug 1390106.

Actually there is a hint in the console output of TB when it is hung without the main window:
WARNING: NS_ENSURE_TRUE(JSVersion(mLangVersion) != JSVERSION_UNKNOWN) failed: file /var/SSD/TB-hg/mozilla/dom/xul/nsXULElement.cpp, line 2699

This is in the standard/error output of the application in the terminal (I use Linux). I don't know if you need a debug build of TB to see this, but I do have one.

Comment 2

[:aceman]

Point 5. may be what kills also some other addons, that we got report from.

Jorg, you had that nice list of changes affecting addons, can you add points 1., 2., 3., 5. to it?

Comment 3

[Jorg K (CEST = GMT+2)]

With Quickfolders TB also doesn't start.

I'll keep the list of problems in mind when I publish the definitive list.

Comment 4

[Eyal Rozenberg]

(In reply to comment #1)
I'll first say that no problem with an extension should cause TB to hang (as opposed to not loading the extension). Just saying that to justify the existence of this bug.

To your specific points - I would have updated all of that stuff if I had gotten some warnings about any of it - either in the AMO verification process or in the JS console when using the extension. I can't just be expected to follow the bugs. I guess I'll fix all that now. I've started with dropping the "x-" in "x-javascript" - but that didn't help. TB 57.0b1 starts with removedupes disabled, and hangs with removedupes enabled. Also, why did none of these prevent TB 56 from starting up?

Finally - I don't see the hint since I don't use a debug build.

Comment 5

[:aceman]

(In reply to Eyal Rozenberg from comment #4)
> (In reply to comment #1)
> I'll first say that no problem with an extension should cause TB to hang (as
> opposed to not loading the extension). Just saying that to justify the
> existence of this bug.

True, but we do not yet know why this causes TB to hang. It may be a problem in the javascript engine (that intentionally refuses the stripts) and could be fixed in m-c code where we do not have much influence.
 
> To your specific points - I would have updated all of that stuff if I had
> gotten some warnings about any of it - either in the AMO verification
> process or in the JS console when using the extension. I can't just be
> expected to follow the bugs.

In the past you could just query all the bugs in bugzilla having "addon-compat" keyword set, and you would get bugs/changes that affect addons.
I think Mozilla does no longer do this as they do not care about old XUL addons anymore so they change interfaces freely.
Some of the breaking changes were announced on the mozilla.org newsgroups (even I did personally and JorgK too).

> I guess I'll fix all that now. I've started
> with dropping the "x-" in "x-javascript" - but that didn't help. TB 57.0b1
> starts with removedupes disabled, and hangs with removedupes enabled.

I said to also remove the ';version=1.7' part. You should only have type="application/javascript" .

> Also why did none of these prevent TB 56 from starting up?

I pointed you to the bugs. The particular change rejecting JS scripts with 'version' specified only landed in 57 so that is where it started to hang.
 
> Finally - I don't see the hint since I don't use a debug build.
OK, thanks for the test.

Comment 8

[:aceman]

(In reply to :aceman from comment #5)
> (In reply to Eyal Rozenberg from comment #4)
> > (In reply to comment #1)
> > I'll first say that no problem with an extension should cause TB to hang (as
> > opposed to not loading the extension). Just saying that to justify the
> > existence of this bug.
> 
> True, but we do not yet know why this causes TB to hang. It may be a problem
> in the javascript engine (that intentionally refuses the stripts) and could
> be fixed in m-c code where we do not have much influence.

On the other hand this was the power of addons in FF/TB that you could do almost anything even take the application down (call internal interfaces and provoke crashes or hangs in obscure places like this one).

In the new world of Webextensions Mozilla is pushing, you will probably not be able to kill the app. But the price will be that addons will be much more feature limited. Many people do not welcome this trend as it means using the Mozilla platform will not bring any benefit/distinguishing from other browsers.

Comment 9

[Eyal Rozenberg]

(In reply to :aceman from comment #8)
> On the other hand this was the power of addons in FF/TB that you could do almost anything even take the application down (call internal interfaces and provoke crashes or hangs in obscure places like this one).

You can provoke crashes with anything, basically - if there's a bug. Plus, like I said in comment #0 - mine is a very tame extension. It doesn't do file I/O, nor use the network, nor go behind the app's back to talk to the OS etc. Anyway, indeed, I can verify removing the ";version=1.7" avoids the hang.

Comment 10

[Axel Grude [:realRaven]]

(At least) The following of my Thunderbird addons are affected. Removing the Js version 1.7 helped resolve the startup problem:

QuickFolders: https://www.mozdev.org/bugs/show_bug.cgi?id=26439
quickFilters: https://www.mozdev.org/bugs/show_bug.cgi?id=26441
Zombie Keys: https://www.mozdev.org/bugs/show_bug.cgi?id=26442
QuickPasswords: https://www.mozdev.org/bugs/show_bug.cgi?id=26443

Working test versions are available there. 
There are also issues with Menu On Top but since it is restartless these are not as serious and they do not prevent Thunderbird from showing its main window.

Comment 11

[Wayne Mery (:wsmwk)]

affected list of top 50 addons ranked by ADI, per aceman from a list supplied by sancus,  
slug	                        created	        Last updated	ADI
remove-duplicate-messages-alte	2007-03-13	2015-03-07	72,274
gmail-conversation-view	        2009-12-10	2016-05-26	62,466
quickfolders-tabbed-folders	2006-08-27	2016-11-28	26,214
zindus	                        2007-11-28	2012-09-01	24,723
dropbox-for-filelink	        2012-08-20	2012-08-31	24,044
smarttemplate4	                2011-06-23	2016-09-23	21,417

Comment 12

[Wayne Mery (:wsmwk)]

We should assess tomorrow, Thursday, whether to request blocklist against the highest ADI affected versions of addons for >=57.0b1, so we can move betas forward even if they aren't all fixed.  Bug 1359469 is an example blocklist request.  Maybe we should assess the ADI ranks 51-100?

(interesting side note - bp-2465d5ec-b8f4-467b-aa15-c79660171115 is 57.0b1 has both the conversations addon and quickfolders.  Four more from the same user - all crash at ~130 seconds 
bp-69d91526-9a19-40aa-b1e8-954db0171115 bug 1405381
bp-8053d38f-d57f-4c90-9aa1-85ba80171115
bp-ae630977-83ca-4627-b786-bf7a10171115
bp-f657c937-c9af-4e48-8c59-87f560171115 )

Comment 13

[:aceman]

It was found in bug 1390106 that hanging the application wasn't really the intended behaviour of the Javascript engine. The versioned scripts should just be rejected and the execution should continue normally, bringing the app UI/windows up.
Masatoshi already has an idea how to fix it (bug 1390106 comment 21).
Masatoshi, the fix worked for me, but there was no message about the rejection of the script. Neither in Browser console nor n the terminal in the debug output. Could that be added in the fix?

Comment 14

[Magnus Melin [:mkmelin]]

What's the story with add-ons and compatible by default? We said we would drop that. Is it already in place or what do we need to do to make it happen?
I'm asking because that would also mean add-ons aren't broken on update like they (apparently) are now.

Comment 15

[Wayne Mery (:wsmwk)]

Where and when did we we say that?

Comment 16

[Magnus Melin [:mkmelin]]

Might have just talked about it at council meetings. It's pretty obvious it needs to happen though.

Comment 17

[Masatoshi Kimura [:emk]]

Attachment: Bug 1417209 - Do not fail when version parameter is found.

I used ReportToConsoleNonLocalized to make the patch upliftable.

Review commit: https://reviewboard.mozilla.org/r/200302/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200302/

Comment 19

[:aceman]

Comment on attachment 8928969 [details]
Bug 1417209 - Do not fail when version parameter is found.

https://reviewboard.mozilla.org/r/200302/#review205576

::: dom/xul/nsXULContentSink.cpp:872
(Diff revision 1)
>                nsAutoString versionName;
>                rv = parser.GetParameter("version", versionName);
>  
>                if (NS_SUCCEEDED(rv)) {
> -                  version = JSVERSION_UNKNOWN;
> +                  nsContentUtils::ReportToConsoleNonLocalized(
> +                      NS_LITERAL_STRING("Use of versioned JavaScript is deprecated. "

Isn't the versioned Javascript dropped, not just deprecated? The scripts are rejected completely.

::: dom/xul/nsXULContentSink.cpp:874
(Diff revision 1)
>  
>                if (NS_SUCCEEDED(rv)) {
> -                  version = JSVERSION_UNKNOWN;
> +                  nsContentUtils::ReportToConsoleNonLocalized(
> +                      NS_LITERAL_STRING("Use of versioned JavaScript is deprecated. "
> +                                        "Please remove the version parameter."),
> +                      nsIScriptError::warningFlag,

As the script is rejected and will not work, shouldn't the severity of the message be error?

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8928969 [details]
Bug 1417209 - Do not fail when version parameter is found.

https://reviewboard.mozilla.org/r/200302/#review205618

::: dom/xul/nsXULContentSink.cpp:876
(Diff revision 1)
> -                  version = JSVERSION_UNKNOWN;
> +                  nsContentUtils::ReportToConsoleNonLocalized(
> +                      NS_LITERAL_STRING("Use of versioned JavaScript is deprecated. "
> +                                        "Please remove the version parameter."),
> +                      nsIScriptError::warningFlag,
> +                      NS_LITERAL_CSTRING("XUL Document"),
> +                      nullptr);

Can we get filename and line number information out of the parser node so this console message points directly to the script tag in question?

Comment 21

[:aceman]

That would be awesome.

Comment 22

[Ton [:Tonnes]]

Fwiw: I’ve emailed Eyal twice recently (for an l10n issue and) to say this started happening for me between the 56.a1 August 1 and 57a1 August 3 TB build on Windows for me. I just checked the August 2 build and it still launches, so the range is in the 56 to 57 jump. (Due to TB Daily updates being broken at the time, I would have found out earlier that day if they were not.)

LGB: https://ftp.mozilla.org/pub/thunderbird/nightly/2017/08/2017-08-02-03-02-07-comm-central/
FBB: https://ftp.mozilla.org/pub/thunderbird/nightly/2017/08/2017-08-03-03-02-07-comm-central/

Comment 23

[Wayne Mery (:wsmwk)]

Are you sure there is nothing between 08-03 and 08-16 that doesn't work?
And which add causes the hang for you for 08-3 build?

Comment 24

[Masatoshi Kimura [:emk]]

Comment on attachment 8928969 [details]
Bug 1417209 - Do not fail when version parameter is found.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200302/diff/1-2/

Comment 25

[Masatoshi Kimura [:emk]]

Comment on attachment 8928969 [details]
Bug 1417209 - Do not fail when version parameter is found.

https://reviewboard.mozilla.org/r/200302/#review205576

> Isn't the versioned Javascript dropped, not just deprecated? The scripts are rejected completely.

Updated the message.

> As the script is rejected and will not work, shouldn't the severity of the message be error?

Changed this to nsIScriptError::errorFlag.

Comment 26

[Masatoshi Kimura [:emk]]

Comment on attachment 8928969 [details]
Bug 1417209 - Do not fail when version parameter is found.

https://reviewboard.mozilla.org/r/200302/#review205618

> Can we get filename and line number information out of the parser node so this console message points directly to the script tag in question?

Added filename and line number information.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/988c9c017c70
Do not fail when version parameter is found. r=mrbkap

Comment 28

[Jorg K (CEST = GMT+2)]

Uplifted to Thunderbird release branch on M-B:
https://hg.mozilla.org/releases/mozilla-beta/rev/0f5e48fc08a529b971cb401644d5a89ed001b61e

Comment 29

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/988c9c017c70

Comment 30

[:aceman]

Thank you very much emk, this works perfectly!

We are building Thunderbird 57 beta 2 with this fix included.

Comment 31

[Axel Grude [:realRaven]]

I have another problem after fixing with SmartTemplate4:

the options dialog cannot load and hangs Thunderbird. I can see no exceptions in Error Console.

Any ideas how to debug this?

Bug:
https://www.mozdev.org/bugs/show_bug.cgi?id=26446

Test Version:
https://www.mozdev.org/bugs/attachment.cgi?id=8341

Comment 32

[Jorg K (CEST = GMT+2)]

I wish I knew what you mean by "the options dialog cannot load". Where is that option dialog? Maybe you mean clicking on the little icon on the right of the status bar.

At times I get a small window with just the title bar and a close button. In the error console I see:
No chrome package registered for chrome://smarttemplate4-pl/skin/style.css

I noticed that when opening a compose window, I see this
TypeError: this.NotifyComposeBodyReadyNew is not a function[Learn More]  MsgComposeCommands.js:340:7
	NotifyComposeBodyReady chrome://messenger/content/messengercompose/MsgComposeCommands.js:340:7
	initListener/stateListener.NotifyComposeBodyReady chrome://smarttemplate4/content/smartTemplate-main.js:384:8
	InitEditor chrome://messenger/content/messengercompose/MsgComposeCommands.js:5530:3
	observe chrome://messenger/content/messengercompose/MsgComposeCommands.js:2839:9
in the error console. Looks like you're messing with those listeners in a bad way.

Comment 33

[Axel Grude [:realRaven]]

(In reply to Jorg K [Almost not working on Thunderbird (some bustage-fix only) due to non-renewal of contract] from comment #32)
> I wish I knew what you mean by "the options dialog cannot load". Where is
> that option dialog? Maybe you mean clicking on the little icon on the right
> of the status bar.

Yes, or by going to Addons > Extensions > SmartTemplate [Options] - that's the standard button for Addon Settings in Windows beside Disable / Remove, maybe it is labelled differently in Linux / Mac.

> 
> At times I get a small window with just the title bar and a close button. In
> the error console I see:
> No chrome package registered for chrome://smarttemplate4-pl/skin/style.css

I removed a path alias that had a platform modifier because of another error message referring to platform being now illegal / not supported (?) However the window should still come up even if some of the css rules are missing.

the lines I commented out in the chrome.manifest were:
content     smarttemplate4-pl         content/ platform
skin        smarttemplate4-pl  classic/1.0  skin/

the platform modifier is used to load platform-specific additional css rules from skin/mac, skin/unix and skin/win

when I reactivate the lines I am getting

Unrecognized chrome manifest modifier 'platform'.   chrome.manifest:53
Unrecognized chrome manifest modifier 'platform'.   chrome.manifest:15

(line 15 is the line before referring to skin smarttemplate4-pl, which I believe causes it; I have no idea why there eis another message for line 53) - I am not sure but is the use of platform now deprecated?

just checking I import a path containing smartTemplate4-pl in my main style.css:
@import 'chrome://smartTemplate4-pl/skin/style.css';
When I remove this line as well while debugging I still don't see any options window / errors in console and appear to be hanging the main window thread. The onLoad() function in settings.js doesn't appear to be called.




> 
> I noticed that when opening a compose window, I see this
> TypeError: this.NotifyComposeBodyReadyNew is not a function[Learn More] 
> MsgComposeCommands.js:340:7
> 	NotifyComposeBodyReady
> chrome://messenger/content/messengercompose/MsgComposeCommands.js:340:7
> 	initListener/stateListener.NotifyComposeBodyReady
> chrome://smarttemplate4/content/smartTemplate-main.js:384:8
> 	InitEditor
> chrome://messenger/content/messengercompose/MsgComposeCommands.js:5530:3
> 	observe
> chrome://messenger/content/messengercompose/MsgComposeCommands.js:2839:9
> in the error console. Looks like you're messing with those listeners in a
> bad way.

Yes I am wrapping NotifyComposeBodyReady once when composer is started, I will look into what may cause this one and update the bug. However it's obviously not related to the options / preferences dialog.

Comment 34

[Selena Deckelmann :selenamarie :selena]

Hi!  Please open a new bug in the appropriate Thunderbird component and make it related to this issue.

Comment 35

[Jorg K (CEST = GMT+2)]

Uplifted to Thunderbird release branch on M-B for TB 58 beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/c2375c4f2414

Comment 36

[Axel Grude [:realRaven]]

(In reply to Selena Deckelmann :selenamarie :selena use ni? pronoun: she from comment #34)
> Hi!  Please open a new bug in the appropriate Thunderbird component and make
> it related to this issue.

Ok will do once I have fixed all issues using the "cook book" for fixing v57 items 
https://wiki.mozilla.org/Thunderbird/Add-ons_Guide_57

...and once I have uploaded prereleases fixed as far as I can to my own bugs.

Comment 37

[Ton [:Tonnes]]

NI no longer needed.