1685222 - Uncaught SyntaxError: redeclaration of const THUNDERBIRD_THEME_PREVIEWS (duplicate observer created each time 'About Thunderbird' et alii get shown)

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[Worcester12345]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:85.0) Gecko/20100101 Firefox/85.0

Steps to reproduce:

Go to "Error Console"

Actual results:

Found this:
Uncaught SyntaxError: redeclaration of const THUNDERBIRD_THEME_PREVIEWS
<anonymous> chrome://messenger/content/aboutAddonsExtra.js:1
observe chrome://messenger/content/specialTabs.js:1719
aboutAddonsExtra.js:1:1
<anonymous> chrome://messenger/content/aboutAddonsExtra.js:1
observe chrome://messenger/content/specialTabs.js:1719

Expected results:

Not found this: ^^^

Comment 1

[Richard Marti (:Paenglab)]

I don't see this error on Daily nor latest beta.

THUNDERBIRD_THEME_PREVIEWS is defined here: https://searchfox.org/comm-central/search?q=THUNDERBIRD_THEME_PREVIEWS&case=false&regexp=false&path=

Geoff, is this a problem with the map? You are much more better than I in JS, could you look at this?

Comment 2

[Thomas D. (:thomas8)]

From code analysis, here's reliable STR, tested on TB 78.6.0 (64-bit) and Daily 86.0a1 (2021-01-06) (64-bit).

STR:

1.) Start TB with devtools, clear error console, ensure that about:addons-Tab is closed.
2.) Help > About Thunderbird [sic], open and close the about dialog. Repeat this step N times (say N = 5) [sic, for better analysis].
3.) Open add-ons tab: Tools > Add-ons
4.) Check error console, and how many occurrences of the same error (see below).

Actual result:

For N=5 in step 2, you'll find 5 instances of this error:

Uncaught SyntaxError: redeclaration of const THUNDERBIRD_THEME_PREVIEWS
    <anonymous> chrome://messenger/content/aboutAddonsExtra.js:1
    observe chrome://messenger/content/specialTabs.js:1717
aboutAddonsExtra.js:1:1
SyntaxError: redeclaration of const THUNDERBIRD_THEME_PREVIEWS aboutAddonsExtra.js:1:1

Code/error analysis ("what's going on here?"):

• Each time when you open aboutDialog.xhtml (or some others), it loads specialTabs.js:
  https://searchfox.org/comm-central/rev/1b4fed73cdfad9fb97f7faa0e3b5df0594a58b4d/mail/base/content/aboutDialog.xhtml#36
  <script src="chrome://messenger/content/specialTabs.js"/>

• Each time specialTabs.js gets loaded, it adds another duplicate global document observer to Services.obs:
  https://searchfox.org/comm-central/rev/1b4fed73cdfad9fb97f7faa0e3b5df0594a58b4d/mail/base/content/specialTabs.js#1725
  Services.obs.addObserver(documentObserver, "chrome-document-interactive");

• So now we have a number of duplicate observers listening for the same thing (not good for memory/performance I guess...), and each observer will fire and load aboutAddonsExtra.js if document.location.href == "... aboutaddons.html":
  https://searchfox.org/comm-central/rev/1b4fed73cdfad9fb97f7faa0e3b5df0594a58b4d/mail/base/content/specialTabs.js#1709-1725

    Services.scriptloader.loadSubScript(
      "chrome://messenger/content/aboutAddonsExtra.js",
      document.defaultView
    );
  

• Each time aboutAddonsExtra.js gets loaded by the duplicate observers, it will start with declaring the constant, and multiple declarations fail - good for us to uncover this nasty thing!
  https://searchfox.org/comm-central/rev/57c0aa5097805106b46b4d49b8a171077751483f/mail/base/content/aboutAddonsExtra.js#7
  const THUNDERBIRD_THEME_PREVIEWS = new Map([

So to fix this bug, we may want to disentangle this and if we cannot eliminate the observer for about:addons, at least ensure that we don't create more than one instance of it. Geoff will know best how to fix this.

Comment 3

[Geoff Lankow (:darktrojan)]

Guess who the idiot was that put that there? :-)

Thanks for figuring this out Thomas. I'll move the observer somewhere else where it can exist only once.

Comment 4

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1685222 - Fix how aboutAddonsExtra works to account for upstream changes. r?ThomasD

Comment 5

[Worcester12345]

(In reply to Thomas D. (:thomas8) from comment #2)

From code analysis,
...

• Each time aboutAddonsExtra.js gets loaded by the duplicate observers, it will start with declaring the constant, and multiple declarations fail - good for us to uncover this nasty thing!
  ...
  So to fix this bug, we may want to disentangle this ... Geoff will know best how to fix this.

So, is there a way for me to clear this bug from "error console", and to test that it is now working better?
Thanks.

Comment 6

[Thomas D. (:thomas8)]

(In reply to Worcester12345 from comment #5)

So, is there a way for me to clear this bug from "error console", and to test that it is now working better?
Thanks.

Hey Worcester, if you're a little bit patient, we'll fix this for you and you can test this in Daily.
If you're impatient, you can try the patch in attachment 9195797 [details].
If you're not building, you can unzip omni.ja of your Daily into the folder where it resides, and tweak the two files manually as the patch does (specialTabs.js, MailGlue.jsm), then restart Daily to test.
Hth.

Comment 7

[Worcester12345]

I'll wait. I thought it was already fixed and waiting to test. Just say when (and how).

Comment 8

[Thomas D. (:thomas8)]

(In reply to Worcester12345 from comment #7)

I'll wait. I thought it was already fixed and waiting to test. Just say when (and how).

Thank you! For most bugs, it'll only be fixed when the bug is marked FIXED.
The patch for this bug is being worked on in Phabricator, see attachment 9195797 [details].

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/46a5ae8ca59c
Fix how aboutAddonsExtra works to account for upstream changes. r=ThomasD

Comment 10

[Geoff Lankow (:darktrojan)]

Of course there's a situation where this doesn't work – it's stupidly complicated for something so simple. And of course I forgot to do a Try run before I landed it.

Waiting for the load event like I did doesn't work in the case where the AOM is opened to the details view of an extension, because browserBundle isn't replaced before it is used. I'm going back to what I first proposed (sort of) because it removes a bunch of unnecessary complications. After a Try run, of course.

Comment 11

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1685222 - Move aboutAddonsExtra observer to MailGlue.jsm, where only one copy of it will be registered. r?ThomasD

Comment 12

[Geoff Lankow (:darktrojan)]

Backed out the first patch:
https://hg.mozilla.org/comm-central/rev/26860585c072a3b271c225ee9ec30e18a933686a

Comment 13

[Thomas D. (:thomas8)]

Geoff, pls set checkin-needed if a try-run with your latest patch looks good.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/55562799bce8
Move aboutAddonsExtra observer to MailGlue.jsm, where only one copy of it will be registered. r=ThomasD