1552851 - Account manage files importing AccountManager.js inherit unwanted event handlers for dialogaccept/cancel

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[:aceman]

Some files in /mailnews/base/prefs/content import AccountManager.js which then runs
document.addEventListener("dialogcancel", onNotAccept);
document.addEventListener("dialogaccept", (event) => {if (!onAccept(true)) { ... }}

Thus, all the subdialogs also get these handlers even if not applicable. Those functions are only valid in the main AM window when whole AM is closed.

Comment 1

[:aceman]

E.g. after closing the Manage identities->Edit identity dialog with Cancel, I get
ReferenceError: reference to undefined property "contentFrame" in AccountManager.js:324:7

Comment 2

[:aceman]

Attachment: 1552851.patch

Here is the patch. It shuffles some imports and functions around so that the files do not need import AccountManager.js . I passes mozmill/account for me and we will see better on try once it works again.

Comment 3

[Jorg K (CEST = GMT+2)]

Is this good for review?

Comment 4

[:aceman]

Comment on attachment 9066192 [details] [diff] [review]
1552851.patch

If the try run at https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=574de3a8e4e486a85e35762b55e7d09f97629c46 is successful then yes please.

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 9066192 [details] [diff] [review]
1552851.patch

Review of attachment 9066192 [details] [diff] [review]:
-----------------------------------------------------------------

Looks OK, try is green apart from the currently expected failures.

::: mailnews/base/prefs/content/am-junk.js
@@ +110,5 @@
>  }
>  
>  function onPreInit(account, accountValues) {
> +  if (top.getAccountValue(account, accountValues, "server", "type", null, false) == "pop3")
> +    gDeferredToAccount = top.getAccountValue(account, accountValues,

Why adding `top.` here and below?

Comment 6

[:aceman]

(In reply to Jorg K (GMT+2) from comment #5)

::: mailnews/base/prefs/content/am-junk.js
@@ +110,5 @@

}

function onPreInit(account, accountValues) {

• if (top.getAccountValue(account, accountValues, "server", "type", null, false) == "pop3")
• gDeferredToAccount = top.getAccountValue(account, accountValues,

Why adding top. here and below?

When I remove import of AccountManager.js from am-junk.js I lose access to the function getAccountValue(). I didn't want to move that one to amUtils.js as it is dependent on other functions and global variables (which should only exist once), so I let it in AccountManager.js. Now, AccountManager.js is loaded in AccountManager.xul with is the top level document and the AM pages are separate documents inside an <iframe>. Calling 'top.' from them accesses the functions in the parent document (AccountManager.xul). We have the needed function there so we do not need to import it again via AccountManager.js. Notice there are already some calls with 'top.' in the AM files. We could actually not import anything in the frames and call all functions with 'top.', if somebody would be brave enough for that rewrite :)
I have tested this returns the correct values of "pop3" or "imap" with this 'top.' prefix.
It also makes eslint happy even though it sees no definition of getAccountValue().

Comment 7

[Jorg K (CEST = GMT+2)]

Thanks for the explanation. Good to go, right?

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/db2315124020
move shared functions from AccountManager.js to other AM utility files so they can be safely imported without whole of AccountManager.js, to prevent adding unwanted event handlers. r=jorgk

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 9066192 [details] [diff] [review]
1552851.patch

I guess we want to backport this to prepare for "AM in tab", right?

Comment 10

[:aceman]

You want this in 68 regardless of AM in tab, otherwise AM is broken (at least the editing of identities).

Comment 11

[Jorg K (CEST = GMT+2)]

TB 68 beta:
https://hg.mozilla.org/releases/comm-beta/rev/822ebd75ac33b145efc22f4e3529aafba9074a78