1555633 - Clean up Mailnews URL handling after bug 1550945

Status: RESOLVED FIXED

(MailNews Core :: General, task)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1550945 +++

In bug 1536744, M-C removed nsIProtocolHandler.newURI and hard-coded all schemes in the system here:
https://searchfox.org/mozilla-central/source/netwerk/base/nsNetUtil.cpp
(sadly Searchfox hasn't updated at time of writing, look for scheme.EqualsLiteral("http").

We followed with
https://hg.mozilla.org/comm-central/file/tip/mailnews/base/util/nsNewMailnewsURI.cpp

I'll post real Searchfox perma-links when the become available.

That file hard-codes all schemes used in Thunderbird, including Calendar, but so far doesn't cater for JS Account which might be broken now although there are no test failures (keep in mind the JS Account tests which are switched off, see bug 1447492).

So while bug 1550945 fixed the bustage (some tests still failing at time of writing), there is scope for some clean-up:

1. Maybe fix the hard-coded include paths at the top of the file
2. Make sure JS Account is adequately catered for
3. Consider removing some "simple" URL implementations, see bug 1550945 comment #0, quote: "remove some of the implementations".

Ben, is this something your team can handle?

Comment 1

[Ben Bucksch (:BenB)]

I don't know how this is supposed to work now. In JSAccount, it's the JS implementation that supplies the URL scheme. We use e.g. "owl" and "owl-ews" in Owl.

Comment 2

[Jorg K (CEST = GMT+2)]

There is a fallback at the end:
https://hg.mozilla.org/comm-central/file/tip/mailnews/base/util/nsNewMailnewsURI.cpp

Does OWL still work in today's Daily?

Comment 3

[[e-mail]]

(In reply to Jorg K from comment #2)

Does OWL still work in today's Daily?

To cut a long story short, Owl does not work in today's Daily.

Comment 4

[Jorg K (CEST = GMT+2)]

http://lists.thunderbird.net/pipermail/maildev_lists.thunderbird.net/2018-March/001052.html
Quote:

So I concluded a year ago that JsAccount is not likely to be a viable
path forward for either new account types, or for JS implementations of
existing account types. Interesting experiment, and it can be made to
work, but just too complex and fragile.

I vaguely recall a discussion with the gist that in order to make URIs thread-safe, the ability to create them in JS code, also adding new schemes, would be discontinued at some stage. Sadly I can't find it now.

Comment 5

[[e-mail]]

Attachment: Owl compatibility hack #1

Comment 6

[[e-mail]]

Attachment: Owl compatibility hack #2

Comment 7

[[e-mail]]

Attachment: Owl compatibility hack #3

Comment 8

[[e-mail]]

I came up with a few approaches to allow Owl to continue to work in Trunk versions of Thunderbird. I would have attached them earlier, but unfortunately my build predated yesterday's latest fixes from bug 1550945 that allowed inline images to display. Even so, for #3 I had to switch from nsMsgMailNewsUrl to JaUrl, otherwise the security checks didn't work (I didn't have time to find out why).

Comment 9

[Jorg K (CEST = GMT+2)]

Nice, the master at work :-) - Please let me know which approach works best for you. Are you planning to port ExQuilla to TB 68? Ben posted somewhere (where? - I can't find it now) that Owl hasn't quite reached feature-parity with ExQuilla, so maybe a solution that works for both would be best.

EDIT: Ben's quote at http://lists.thunderbird.net/pipermail/maildev_lists.thunderbird.net/2019-May/001636.html

Comment 10

[Ben Bucksch (:BenB)]

Owl already works for TB 68, and we have an ExQuilla for Thunderbird 68 in the testing phase and plan to release it soon.

Comment 11

[Ben Bucksch (:BenB)]

I think #1 and #2 could be combined, in that you create it directly, if it's the main thread, and otherwise using the runnable.
I don't really understand #3, we'll have to talk about that internally.

Comment 12

[Jorg K (CEST = GMT+2)]

(In reply to Ben Bucksch (:BenB) from comment #10)

Owl already works for TB 68, and we have an ExQuilla for Thunderbird 68 in the testing phase and plan to release it soon.

Great news!

Comment 13

[Ben Bucksch (:BenB)]

Neil and I discussed this.

• Gecko moved the creation of the URI object into the thread that calls NewURI. That means they need to hardcode all the code for NewURI for all protocols and make sure that it's all thread safe, because the actual protocol handler might live on another thread than whatever created the URI object.
• Owl and ExQuilla implement NewURI themselves, and they also set the folder object on the URL. This folder object is then stored in the URL object and can be retrieved via GetFolder from a member variable. Thunderbird calls GetFolder() in various places (also for IMAP and POP3).
• With patch 3, Owl can no longer set the folder object on the URL, because Owl's implementation of NewURI is not called anymore, so we have to use a workaround to get the folder object from the server URL and query the folder every time. That's slower and also hardcodes everything in TB C++. It moves the code from Owl to TB and makes it less efficient and less flexible.
• With patch 2, TB continues to call NewURI implementation in Owl/ExQuilla, and they can continue to set the folder on the URL. However, we need to dispatch to the main thread, because Owl lives there.
• Patch 2 already avoids the dispatch, if we are already on the main thread. But it might be slightly faster to make the query explicit, by integrating patch 1 into patch 2. But that's strictly optional.

Comment 14

[Ben Bucksch (:BenB)]

Neil will integrate patch 1 into patch 2, and post it.

Comment 15

[Ben Bucksch (:BenB)]

Comment on attachment 9069195 [details] [diff] [review]
Owl compatibility hack #2

Patch should work as-is (thus r+), but will be improved slightly.

Comment 16

[Ben Bucksch (:BenB)]

Comment on attachment 9069196 [details] [diff] [review]
Owl compatibility hack #3

@Neil: Are there hunks from this patch that are useful and we should integrate?

Comment 17

[Jorg K (CEST = GMT+2)]

Looking forward for to the final version ;-)

Comment 18

[[e-mail]]

Attachment: Fix JSAccount

Explicitly proxies the call for clarity.

Comment 19

[[e-mail]]

Comment on attachment 9069196 [details] [diff] [review]
Owl compatibility hack #3

(In reply to Ben Bucksch from comment #16)
> @Neil: Are there hunks from this patch that are useful and we should integrate?

>diff --git a/mailnews/jsaccount/src/JaUrl.h b/mailnews/jsaccount/src/JaUrl.h
>--- a/mailnews/jsaccount/src/JaUrl.h
>+++ b/mailnews/jsaccount/src/JaUrl.h
>@@ -42,7 +42,10 @@ class JaBaseCppUrl : public nsMsgMailNew
>   NS_DECL_NSIMSGMESSAGEURL
>   NS_DECL_MSGIJAURL
>   NS_DECL_NSIINTERFACEREQUESTOR
>-  JaBaseCppUrl() {}
>+  JaBaseCppUrl() {
>+    mCanonicalLineEnding = false;
>+    m_urlType = eDisplay;
>+  }

This is the only part that is useful independently of the rest of the patch, but we could add initialisation code for these variables in Owl anyway.

Comment 20

[[e-mail]]

(In reply to comment #19)

but we could add initialisation code for these variables in Owl anyway.

Actually, we should definitely initialise the URL type; automatic resizing of attached images displayed inline isn't working in Owl right now.

Comment 21

[Ben Bucksch (:BenB)]

Comment on attachment 9069607 [details] [diff] [review]
Fix JSAccount

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

::: mailnews/base/util/nsNewMailnewsURI.cpp
@@ +97,5 @@
> +    contractID += scheme;
> +    bool isRegistered = false;
> +    compMgr->IsContractIDRegistered(contractID.get(), &isRegistered);
> +    if (isRegistered) {
> +      auto NewURI = [&aSpec, &aCharset, &aBaseURI, aURI, &contractID, &rv]() -> auto {

wow. I didn't know that C++ had inline functions now. That's fantastic.

Are there other uses of this syntax in the tree, just to make sure that it's OK to use it with the current compilers? I assume yes, because we're using clang, but just checking.

This is really great.

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 9069607 [details] [diff] [review]
Fix JSAccount

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

::: mailnews/base/util/nsNewMailnewsURI.cpp
@@ +107,5 @@
> +      if (NS_IsMainThread()) {
> +        NewURI();
> +      } else {
> +        nsCOMPtr<nsIRunnable> task = NS_NewRunnableFunction("NewURI", NewURI);
> +        mozilla::SyncRunnable::DispatchToThread(mozilla::GetMainThreadEventTarget(), task);

Excuse the ignorance, isn't there a NS_DispatchToMainThread?

Comment 23

[[e-mail]]

(In reply to Jorg K from comment #22)

Comment on attachment 9069607 [details] [diff] [review]
Fix JSAccount

Review of attachment 9069607 [details] [diff] [review]:

::: mailnews/base/util/nsNewMailnewsURI.cpp
@@ +107,5 @@

•  if (NS_IsMainThread()) {
  

•    NewURI();
  

•  } else {
  

•    nsCOMPtr<nsIRunnable> task = NS_NewRunnableFunction("NewURI", NewURI);
  

•    mozilla::SyncRunnable::DispatchToThread(mozilla::GetMainThreadEventTarget(), task);
  

Excuse the ignorance, isn't there a NS_DispatchToMainThread?

That's not synchronous.

Comment 24

[Ben Bucksch (:BenB)]

@Jörg: The patch already has review (and you can see that we did the review properly). Owl is completely broken on trunk without this patch.

Comment 25

[Jorg K (CEST = GMT+2)]

Well, I was wondering why you never requested check-in. I'll land it in the next batch.

Comment 26

[Ben Bucksch (:BenB)]

Thanks!

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ec6a2a472a5e
New URI handling for JS Account based schemes. r=BenB

Comment 28

[Jorg K (CEST = GMT+2)]

Actually, there were more things to do in this bug, see the list of 1. to 3. in comment #0. Looks like I have to file another bug now.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9bbc721229fd
Backed out changeset ec6a2a472a5e for causing test failures in chrome/test/unit/test_no_remote_registration.js. a=backout DONTBUILD

Comment 30

[Jorg K (CEST = GMT+2)]

Please do a try run next time.

Comment 31

[[e-mail]]

Attachment: Fix test

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9f0fb7107a708b94982202fb7e355bd23f4418a7

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/37270fb55bca
New URI handling for JS Account-based schemes. r=BenB

Comment 33

[Jorg K (CEST = GMT+2)]

Blame any strange formatting on clang-reformat:
https://hg.mozilla.org/comm-central/rev/37270fb55bca#l3.48