Closed Bug 1555633 Opened 6 months ago Closed 5 months ago

Clean up Mailnews URL handling after bug 1550945

Categories

(MailNews Core :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 69.0

People

(Reporter: jorgk, Assigned: neil)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

+++ 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?

Flags: needinfo?(ben.bucksch)

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.

Flags: needinfo?(ben.bucksch)

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?

(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.

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.

Attached patch Owl compatibility hack #1 (obsolete) — Splinter Review
Assignee: nobody → neil
Attached patch Owl compatibility hack #2 (obsolete) — Splinter Review
Attached patch Owl compatibility hack #3 (obsolete) — Splinter Review

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).

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

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

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.

(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!

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.

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

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

Patch should work as-is (thus r+), but will be improved slightly.
Attachment #9069195 - Flags: feedback+
Attachment #9069194 - Flags: review-
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?
Attachment #9069196 - Flags: review-

Looking forward for to the final version ;-)

Attached patch Fix JSAccount (obsolete) — Splinter Review

Explicitly proxies the call for clarity.

Attachment #9069194 - Attachment is obsolete: true
Attachment #9069195 - Attachment is obsolete: true
Attachment #9069196 - Attachment is obsolete: true
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.

(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.

Attachment #9069607 - Attachment description: Owl compatibility hack → Fix JSAccount
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.
Attachment #9069607 - Flags: review+
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?

(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.

@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.

Keywords: checkin-needed

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

Thanks!

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ec6a2a472a5e
New URI handling for JS Account based schemes. r=BenB

Status: NEW → RESOLVED
Closed: 5 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

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.

Target Milestone: --- → Thunderbird 69.0
Blocks: 1560639
Regressions: 1560692
Pushed by mozilla@jorgk.com:
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

Please do a try run next time.

Status: RESOLVED → REOPENED
Flags: needinfo?(neil)
Resolution: FIXED → ---
Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/37270fb55bca
New URI handling for JS Account-based schemes. r=BenB

Status: REOPENED → RESOLVED
Closed: 5 months ago5 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

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

You need to log in before you can comment on or make changes to this bug.