TEST-UNEXPECTED-FAIL | [snip]/newmailaccount/test-newmailaccount.js - 18 failing subtests

RESOLVED FIXED in Thunderbird 66.0

Status

defect
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

Trunk
Thunderbird 66.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

6 months ago
TEST-UNEXPECTED-FAIL | [snip]/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_get_an_account
TEST-UNEXPECTED-FAIL | [snip]/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_restored_ap_tab_works
TEST-UNEXPECTED-FAIL | [snip]/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_can_dismiss_account_provisioner
TEST-UNEXPECTED-FAIL | [snip]/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_can_switch_to_existing_email_account_wizard
TEST-UNEXPECTED-FAIL | [snip]/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_flip_flop_from_provisioner_menuitem
TEST-UNEXPECTED-FAIL | [snip]/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_persist_name_in_search_field
TEST-UNEXPECTED-FAIL | [snip]/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_html_characters_and_ampersands
TEST-UNEXPECTED-FAIL | [snip]/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_throws_console_error_on_corrupt_XML
TEST-UNEXPECTED-FAIL | [snip]/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_internal_link_opening_behaviour
TEST-UNEXPECTED-FAIL | [snip]/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_window_open_link_opening_behaviour
TEST-UNEXPECTED-FAIL | [snip]/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_external_link_opening_behaviour
TEST-UNEXPECTED-FAIL | [snip]/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_return_to_provisioner_on_error_XML
TEST-UNEXPECTED-FAIL | [snip]/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_disabled_fields_when_searching
TEST-UNEXPECTED-FAIL | [snip]/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_search_button_disabled_if_no_lang_support
TEST-UNEXPECTED-FAIL | [snip]/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_provider_language_wildcard
TEST-UNEXPECTED-FAIL | [snip]/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_search_button_disabled_if_no_query_on_init
TEST-UNEXPECTED-FAIL | [snip]/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_get_new_account_focuses_existing_ap_tab
TEST-UNEXPECTED-FAIL | [snip]/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_per_address_prices

https://taskcluster-artifacts.net/Q2xjbUY-TvuuDj2SwH5oSA/0/public/logs/live_backing.log
Various timeouts:
EXCEPTION: Timed out waiting for search results to arrive.
EXCEPTION: Timeout waiting for the content tab page to load.
and more. Search for "EXCEPTION:" in the log.

I've confirmed that the C-C changeset from bug 1515903 is *NOT* at fault. Hence it must come from M-C.

M-C last good: e16b548dc14c58efd713bfac10a5944fd6
M-C first bad: 38dac70dcec66ea4c5f14da8ae600aa6cc
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e16b548dc14c58efd713bfac10a5944fd6&tochange=38dac70dcec66ea4c5f14da8ae600aa6cc

Maybe
f256f0e01e82 Olli Pettay - Bug 1514791, don't generate click if path from mousedown.target to mouseup.target contains a menupopup - mark menupopup interactive content for now, r=masayuki

I'll try a local backout. Nice bustage, just right for the holidays :-(
Flags: needinfo?(geoff)
Flags: needinfo?(ben.bucksch)
Flags: needinfo?(acelists)
Assignee

Updated

6 months ago
Summary: TEST-UNEXPECTED-FAIL | [snip\/newmailaccount/test-newmailaccount.js - 18 failing subtests → TEST-UNEXPECTED-FAIL | [snip]/newmailaccount/test-newmailaccount.js - 18 failing subtests
Assignee

Comment 1

6 months ago
Still working at M-C rev 30d8fe076fd0, so new range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=30d8fe076fd0&tochange=38dac70dcec66ea4c5f14da8ae600aa6cc

I'll try a local backout next.
Assignee

Comment 2

6 months ago
OK, still failing with local backout of rev f256f0e01e82, Bug 1514791. So it's not that.

I see a lot of these errors:
JavaScript error: chrome://messenger/content/newmailaccount/accountProvisioner.js, line 291: NS_ERROR_FAILURE:
    let name = EmailAccountProvisioner.storage.getItem("name") ||
               nameElement.value;
No idea what could be wrong there.

Comment 3

6 months ago
EmailAccountProvisioner.storage is 

XPCOMUtils.defineLazyGetter(EmailAccountProvisioner, "storage", function() {
  return getLocalStorage("accountProvisioner");
});

function getLocalStorage(page) {
  var url = "chrome://content/messenger/accountProvisionerStorage/" + page;
      
  var uri = Services.io.newURI(url);
  var principal = Services.scriptSecurityManager.createCodebasePrincipal(uri, {});
  return Services.domStorageManager.createStorage(null, principal, url);
}

Any changes in m-c to domStorageManager? Or changes from Services.scriptSecurityManager ?
Flags: needinfo?(acelists)
Assignee

Comment 4

6 months ago
Well, since this line mentions storage, perhaps it's this:
0f7da6819c47 Jan Varga - Bug 1510410 - Enable Next Generation Local Storage Implementation on Nightly; r=asuth
https://hg.mozilla.org/mozilla-central/rev/0f7da6819c47#l1.12

We should try setting the pref to its original value.
Assignee

Updated

6 months ago
Keywords: leave-open
Assignee

Updated

6 months ago
Flags: needinfo?(geoff)
Flags: needinfo?(ben.bucksch)
Assignee

Comment 7

6 months ago
https://hg.mozilla.org/comm-central/rev/eb217bd92fbc8920837034696113676eb52c7a23
fix tests by disabling Next Generation Local Storage Implementation after bug 1510410. rs=bustage-fix

Jan and Andrew, what do we need to follow M-C here?
Flags: needinfo?(jvarga)
Flags: needinfo?(bugmail)
Assignee

Updated

6 months ago
Attachment #9033026 - Attachment description: 1515992-nextgen-storage.patch → 1515992-nextgen-storage.patch [landed in comment #7]

Comment 8

6 months ago
Ah, I see.

I think the problem is that new localStorage implementation uses QuotaManager which currently doesn't support origins like:
chrome://content/messenger/accountProvisionerStorage/ + page

Actually, in bug 1423917, some of the patches wants to make it explicitly obsolete to use anything but "chrome" (no chrome://something/...)

Andrew, Tom, maybe the origin parser should be able to parse chrome:// urls.
Flags: needinfo?(jvarga) → needinfo?(shes050117)
So, as I understand it, the choice of the principal "chrome://content/messenger/accountProvisionerStorage/" is arbitrary.  (Note: I was around when the account provisioner code was developed, but not directly involved in its implementation.

I'd suggest Thunderbird change it to something like "about:accountProvisioner".  The origin parser will be happy with that and it's no worse than the current arbitrary choice.


The issue of whether the origin parser should be able to parse that origin is somewhat separate, I think.  And I think the answer is probably "no".  Firefox has complete control over the chrome protocol's use in the codebase and we haven't needed it thus far.  And there's a simple solution for Thunderbird here.
Flags: needinfo?(bugmail)
Assignee

Comment 10

6 months ago
Thanks Andrew, is this what you suggested? Doesn't work:
[3740, Main Thread] WARNING: 'NS_FAILED(rv)', file c:/mozilla-source/comm-central/dom/localstorage/LocalStorageManager2.cpp, line 134
JavaScript error: chrome://messenger/content/newmailaccount/accountProvisioner.js, line 37: NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIDOMStorageManager.createStorage]
Attachment #9033095 - Flags: feedback?(bugmail)
Assignee

Comment 11

6 months ago
Works it I use something like "http://www.example.com/accountProvisioner" like test_localStorageFromChrome.xhtml does these days, they use: http://example.com/tests/dom/tests/mochitest/localstorage/frameChromeSlave.html

We could use www.thunderbird.net/accountProvisioner :-)
Assignee

Comment 12

6 months ago
Posted patch 1515992-NGLS.patch - working (obsolete) — Splinter Review
How about this?
Attachment #9033096 - Flags: feedback?(bugmail)
Comment on attachment 9033095 [details] [diff] [review]
1515992-NGLS.patch - WIP - NOT working

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

::: mail/test/mozmill/newmailaccount/test-newmailaccount.js
@@ +1200,5 @@
>   */
>  function test_search_button_disabled_if_no_query_on_init() {
>    // We have to do a little bit of gymnastics to access the local storage
>    // for the accountProvisioner dialog...
> +  let url = "about:accountProvisioner";

all the other about: pages are lower case, so no camel casing please
Comment on attachment 9033096 [details] [diff] [review]
1515992-NGLS.patch - working

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

::: mail/components/newmailaccount/content/accountProvisioner.js
@@ +29,5 @@
>   * @param {String} page The page to get the localstorage for.
>   * @return {nsIDOMStorage} The localstorage for this page.
>   */
> +function getLocalStorage() {
> +  var url = "http://www.thunderbird.net/accountProvisioner";

If we go with a http address, why not use something that's not super real, like
http://accountprovisioner.thunderbird.localhost/
Good idea.

But if you want non-existing hostnames, please use .invalid, which was created for that purpose, i.e.
https://accountprovisioner.thunderbird.invalid/
Assignee

Comment 17

6 months ago
Assignee: nobody → jorgk
Attachment #9033095 - Attachment is obsolete: true
Attachment #9033096 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #9033095 - Flags: feedback?(bugmail)
Attachment #9033096 - Flags: feedback?(bugmail)
Attachment #9033112 - Flags: review?(mkmelin+mozilla)
Attachment #9033112 - Flags: feedback?(bugmail)
Attachment #9033112 - Flags: feedback?(ben.bucksch)
Attachment #9033112 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9033112 [details] [diff] [review]
1515992-NGLS.patch (v3)

Looks good to me.

I've checked the |page| parameter, and it's called only in one place with a hardcoded value, so it should be fine to remove the parameter.
Flags: needinfo?(shes050117)
Attachment #9033112 - Flags: review+
Attachment #9033112 - Flags: feedback?(ben.bucksch)
Attachment #9033112 - Flags: feedback+
Assignee

Updated

6 months ago
Keywords: leave-open

Comment 19

6 months ago
Is the stored data temporary for the session?
Is it OK to change the URL under which we store the data and we do not need to pick any previously saved data under the old URL already existing in user profile?

Comment 20

6 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/061051149fae
change account provisioner's dummy URL for storage to be compatible with NGLS. r=mkmelin,BenB
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Assignee

Updated

6 months ago
Target Milestone: --- → Thunderbird 66.0
Attachment #9033112 - Flags: feedback?(bugmail)
You need to log in before you can comment on or make changes to this bug.