1432204 - Fallout from bug 1431913: Revisit calls to SetSpecInternal(), nsSMTPProtocolHandler.js, nsMsgMailNewsUrl::CloneInternal(), mutators for other classes derived from it

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Jorg K (CEST = GMT+2)]

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

Under bustage-fix pressure in bug 1431913 we mostly replaces SetSepc() with SetSpecInternal(). We also noticed other deficiencies in Mailnews URI handling. So this bug is to fix the fallout and has various aspects:

Revisit all calls to SetSpecInternal()
======================================
We'd have to talk to the Netwerk people what's going to happen with this method in the long run and when it's correct to call it.
Philipp prepared a patch in attachment 8944314 [details] [diff] [review] which aims at replacing the SetSpecInternal() calls with calls of
  NS_MutateURI(xxx).SetSpec(yyy).Finalize(zzz);
I believe that many of these calls are not necessary if you can create the correct URL object in the first place. In bug 1431913 comment #17 Valentin suggested to use
  NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID).SetSpec(yyy).Finalize(zzz)
which I believe creates the correct object straight away.

I believe that in many cases in nsImapService.cpp setting the entire spec is just a lazy way out when in fact just individual parts like the path could be manipulated.

nsSMTPProtocolHandler.js
========================
was fixed with a quick hack copied from LDAP:
https://hg.mozilla.org/comm-central/rev/1dd846f3e167ed1f8ddf4463239daaed86a00e38
There might be better ways to do it.

nsMsgMailNewsUrl::CloneInternal()
=================================
In an attempt to fix nsSMTPProtocolHandler.js I created attachment 8944332 [details] [diff] [review] basically trying to use mutate() on the freshly created SMTP URL. Whilst that is not my preferred approach, it showed various problems:
- nsSmtpUrl::Mutate() was missing.
- nsMsgMailNewsUrl::CloneInternal() does something really weird.
  It creates a new nsIURI object and QI's it to nsMsgMailNewsUrl.
  The patch shows how it should be done and is already done in LDAP,
  Address book and MailTo URLs.
  That needs further investigation.

Valentin also mentioned on IRC that nsImapUrl::CloneInternal may also be incorrect.

Doing mutate() on an nsMsgMailNewsUrl appeared to cause this error:
NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIURI.mutate]
which might originate from the ioService->NewURI() call in nsMsgMailNewsUrl::CloneInternal().

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1432204-replace-SetSpecInteral-stardardURL.patch (v1)

OK, here comes the low-hanging fruit: Replace the SetSpecInternal() calls for the standard URL. That's with a little boyscout clean-p thrown in.

Comment 2

[Jorg K (CEST = GMT+2)]

Valentin, before we move forward here, can you please answer some questions:
- What's the future of SetSpecInternal()? Where is it legal do use?
- Do you agree that SetSpecInternal() or some mutate call should be avoided where the
  spec is already known at creation time? So instead of "create + set spec"
  "create with the correct spec" is preferred, like I've done in attachment 8944463 [details] [diff] [review].
- If we only want to vary parts of the spec, we should use the appropriate functions,
  like SetPathQueryRef() instead of setting the entire spec.
- Do you agree that
  https://hg.mozilla.org/comm-central/rev/1dd846f3e167ed1f8ddf4463239daaed86a00e38#l2.10
  is clumsy? It's much preferable to create the URL object with the correct spec.
  Bug 1431913 comment #80 mentions a proper init method to be implemented
  in bug 1432187 (and removed from standard URLs in bug 1432257).

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1432204-nsMsgMailNewsUrl-clone.patch (v1)

This is from attachment 8944332 [details] [diff] [review]. This should correct the nsMsgMailNewsUrl clone.

Let's see how it performs:
https://hg.mozilla.org/try-comm-central/pushloghtml?changeset=21bfce07cc017b64cfa3ee7660b22a4368a386eb

I don't understand how that clone ever worked (unless of course it was never used): It created a nsIURI object which certainly isn't a nsMsgMailNewsUrl object.

In the derived classes, things didn't look much better:
nsImapUrl::CloneInternal()
nsMailboxUrl::CloneInternal()
nsNntpUrl::CloneInternal()

Note that the "other three" URL clones are correct:
nsLDAPURL::CloneInternal()
nsAddbookUrl::CloneInternal()
nsMailtoUrl::CloneInternal()

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1432204-replace-SetSpecInteral-stardardURL.patch (V1b) [landed comment #13]

Oops, didn't compile on Mac, missing include. Adding Valentin's f+.

New try with the other patch as well:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c92366dabbda07a3842f2c7b3187aef4e06c180c

Comment 5

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

(In reply to Jorg K (GMT+1) from comment #2)
> Valentin, before we move forward here, can you please answer some questions:
> - What's the future of SetSpecInternal()? Where is it legal do use?

For example nsMsgMailNewsUrl::SetSpecInternal should be called from:
- the implementation of nsMsgMailNewsUrl::Mutator
- inside the nsMsgMailNewsUrl implementation and any classes that extend it
- C++ unit tests for nsMsgMailNewsUrl
- we should generally treat is as a private method

> - Do you agree that SetSpecInternal() or some mutate call should be avoided
> where the
>   spec is already known at creation time? So instead of "create + set spec"
>   "create with the correct spec" is preferred, like I've done in attachment
> 8944463 [details] [diff] [review].

Correct. 

> - If we only want to vary parts of the spec, we should use the appropriate
> functions,
>   like SetPathQueryRef() instead of setting the entire spec.

That is true. And we should also prefer to use a 
uri.mutate().setPathQueryRef("text").finalize()
rather than
uri.pathQueryRef = "text"; // This is going to become readonly, similar to spec.

> - Do you agree that
>  
> https://hg.mozilla.org/comm-central/rev/
> 1dd846f3e167ed1f8ddf4463239daaed86a00e38#l2.10
>   is clumsy? It's much preferable to create the URL object with the correct
> spec.
>   Bug 1431913 comment #80 mentions a proper init method to be implemented
>   in bug 1432187 (and removed from standard URLs in bug 1432257).

Correct!
Here is an example: https://searchfox.org/mozilla-central/rev/b7e3ec2468d42fa59d86c03ec7afeb209813f1d4/dom/base/test/chrome/test_bug682305.html#122

I've written down some examples here: https://github.com/valenting/gecko/wiki/Migrating-nsIURI-code-to-use-mutators

Note that for this to work well, all classes that implement nsIURI or extend one that does should override Mutate(), and should define their own mutator.
These are all the ones in Gecko: https://github.com/valenting/gecko/wiki/Threadsafe-URIs-progress#classes-implementing-nsiuri (actually there is one missing, which I implement here: https://reviewboard.mozilla.org/r/214678/diff/1#index_header)


I am happy to help out with feedback on patches.

Comment 6

[Jorg K (CEST = GMT+2)]

Hmm, https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c92366dabbda07a3842f2c7b3187aef4e06c180c
crashed left, right and centre. So let's just try with the harmless first patch:
https://hg.mozilla.org/try-comm-central/rev/9ba3dc0ffc4b82452539c8066b2ab7d9db0cf340

Comment 7

[:aceman]

Comment on attachment 8944514 [details] [diff] [review]
1432204-replace-SetSpecInteral-stardardURL.patch (V1b) [landed comment #13]

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

This looks good, but I get a single failure xpcshell in mailnews/news/test/unit/test_bug540288.js locally. Can you try that?

Comment 8

[Jorg K (CEST = GMT+2)]

I'll wait for the try from comment #6, but since you asked:
  mozilla/mach xpcshell-test mailnews/news/test/unit/test_bug540288.js
passes. The patch is basically mostly cosmetic and no change to functionality.

On a different note: I'd really like to understand what that incomplete cloning business is about for nsMsgMailNewsUrl and derived classes. It's so different from the "other three" clones. Without being able to clone, we can forget the mutators, since all the do by default is clone:
https://searchfox.org/mozilla-central/source/__GENERATED__/dist/include/nsIURIMutator.h#44

Comment 9

[Jorg K (CEST = GMT+2)]

Green try.

Comment 10

[:aceman]

Comment on attachment 8944514 [details] [diff] [review]
1432204-replace-SetSpecInteral-stardardURL.patch (V1b) [landed comment #13]

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

Thanks.

Comment 11

[Jorg K (CEST = GMT+2)]

I think we will land the bits as we go along, we have until the next branch date: 2018-03-12.

BTW, https://hg.mozilla.org/try-comm-central/rev/9784520e32c3cd797f6985b0eef6d020795da355, you missed one (test_nsIMsgFolderListenerIMAP.js).
https://dxr.mozilla.org/comm-central/search?q=specs.push(URI.spec)%3B&redirect=false

And you could do this one as well:
https://dxr.mozilla.org/comm-central/source/mailnews/imap/test/unit/test_imapUndo.js#39

Comment 12

[:aceman]

(In reply to Jorg K (GMT+1) from comment #11)
> https://hg.mozilla.org/try-comm-central/rev/
> 9784520e32c3cd797f6985b0eef6d020795da355, you missed one
> (test_nsIMsgFolderListenerIMAP.js).

I skipped it intentionally as it was more complicated. But I can try, pushing to an array may not have sideeffects which would need adding the message be ordered after them.

> And you could do this one as well:
> https://dxr.mozilla.org/comm-central/source/mailnews/imap/test/unit/
> test_imapUndo.js#39

Hm, why didn't you convert this in your patch? It seems to me there was no need to touch message.spec, it was no URL, just a generic {} object, where we could set .spec freely.

Comment 13

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/941a1c2bda737375f1c3040e99d1a59531c4129f
replace SetSpecInteral() on stardard URL. r=aceman

Comment 14

[:aceman]

Attachment: simplify addMessagesToServer [landed comment #19]

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

Comment 15

[Jorg K (CEST = GMT+2)]

Any reason you're doing a try with Mozmill only when you're changing Xpcshell tests?
The changes looks good, but better safe than sorry.

Comment 16

[:aceman]

Because I messed it up :) New try up.

Comment 17

[:aceman]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8498599672a1579be1be6c22370906cb4382f2b3

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8944930 [details] [diff] [review]
simplify addMessagesToServer [landed comment #19]

Thanks, nice that you also removed the argument 'localFolder' where is wasn't required.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fb8fb3ab759c
simplify addMessagesToServer() in imap tests. r=jorgk

Comment 20

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

(In reply to Jorg K (GMT+1) from Bug 1432908 comment #7)
> (In reply to Valentin Gosu [:valentin] from comment #6)
> > Also, I don't know if this is in some of the other patches or not, but
> > classes extending nsMsgMailNewsUrl (nsSmtpUrl, nsImapUrl, etc) need to
> > override the Mutate() method, and have their own mutator implementation (and
> > correct Clone() or StartClone() implementations)
> Yes, I'm aware of that, that's meant to happen in bug 1432204. You pointed
> out that something was fishy in the nsMsgMailNewsUrl::CloneInternal(), but
> it's been hard to work out why it's doing what it's doing and how to replace
> it without causing breakage. But that's for bug 1432204 to discuss. Please
> comment there and give us more background. Currently everything appears to
> be working even without the deficiencies we detected.

According to pattern we use in nsSimpleURI it would go like this:
nsMsgMailNewsUrl should have method with this signature:
virtual nsMsgMailNewsUrl* StartClone();

Each class that extends nsMsgMailNewsUrl would override this method
nsMsgMailNewsUrl* nsMsgMailNewsUrl::StartClone() { return new nsMsgMailNewsUrl(); }
nsMsgMailNewsUrl* nsSmtpUrl::StartClone() { return new nsSmtpUrl(); }

Then, in nsMsgMailNewsUrl::CloneInternal() instead of creating a new URI via ioService->NewURI(), you instead get it by calling StartClone().

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: 1432204-nsMsgMailNewsUrl-clone.patch (v2) WIP

Here's an updated patch. The system starts with this patch, but when I try to view a message, I crash due to endless recursion in nsContentUtils::GetUTFOrigin(). Looks like something fails since nsMsgMailNewsUrl is an nsIURIWithPrincipal.

Comment 22

[Jorg K (CEST = GMT+2)]

I guess one problem is that the clone never really clones anything. I really don't understand what that used to do:
https://dxr.mozilla.org/comm-central/rev/cd538cb90f754d0b625c6294a0c18b243905e3e5/mailnews/base/util/nsMsgMailNewsUrl.cpp#615
The clone of a nsMsgMailNewsUrl was simply an nsIURI object with the given spec and nothing else.

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: 1432204-nsMsgMailNewsUrl-clone.patch (v3) WIP

OK, copying at least two attributes/member variables across lets me view messages without going into the endless recursion.

So let's see how far we get with this:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ded48973a132ff59261be15927441ffadf1ffce6

Valentin, what do you think of this: This goes into the right direction but it's really unclear with attributes need to be cloned ... or where CloneInternal() was actually called before. I understand that in the new scheme of things with mutators, the clone is called in BaseURIMutator::InitFromURI()
https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/netwerk/base/nsIURIMutator.idl#37

But so far, I don't think we use that for nsMsgMailNewsUrl and friends yet.

Comment 24

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Comment on attachment 8949237 [details] [diff] [review]
1432204-nsMsgMailNewsUrl-clone.patch (v3) WIP

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

Looks good.

Comment 25

[Jorg K (CEST = GMT+2)]

Thanks. You didn't comment on the fact that this is an incomplete clone, I'm only copying two attributes so far. For the mutator stuff to work, this needs to be an exact copy, right? The same goes for the derived classes.

Comment 26

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

I'm not sure on what each specific member does, so I can't be sure.
Technically, the object you return from StartClone() can be already initialized with all the members in place, and I assume this is what we'll want here.

Comment 27

[Jorg K (CEST = GMT+2)]

Comment on attachment 8949237 [details] [diff] [review]
1432204-nsMsgMailNewsUrl-clone.patch (v3) WIP

What do you think of this? I doesn't make things worse and it's a step in the right direction.

Comment 28

[:aceman]

Comment on attachment 8949237 [details] [diff] [review]
1432204-nsMsgMailNewsUrl-clone.patch (v3) WIP

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

::: mailnews/base/util/nsMsgMailNewsUrl.cpp
@@ +640,5 @@
> +
> +  // Clone attributes.
> +  // XXX there are plenty more, here we clone the essential(??) ones (or else we crash).
> +  clone->m_principal = m_principal;
> +  clone->m_isPrincipalURL = m_isPrincipalURL;

What about attributes specific to the subclasses? Will they need to override whole CloneInternal() ? Or will there by some FinishClone() method to override?

Comment 29

[Jorg K (CEST = GMT+2)]

Subclasses have their own CloneInternal(), for example:
https://dxr.mozilla.org/comm-central/rev/f842916b9211d69fdda4075aa3dc8b69e37a25ec/mailnews/imap/src/nsImapUrl.cpp#1164
After the call to nsMsgMailNewsUrl::CloneInternal() they can do anything they like, including copy attributes over.

Comment 30

[Kent James (:rkent)]

I'm not sure I agree with the direction this is going, using StartClone(). The root issue that is trying to be solved here seems to be:

(In reply to Jorg K (GMT+1) from comment #3)
> 
> I don't understand how that clone ever worked (unless of course it was never
> used): It created a nsIURI object which certainly isn't a nsMsgMailNewsUrl
> object.
> 

I don't think that is correct, and the previous code does work.

nsIOService::NewURI does not create a generic URI, it searches for a protocol handler, and that protocol handler creates the URI. So in the case of IMAP for example, even though nsImapUrl::CloneInternal goes through the base class nsMsgMailNewsUrl::CloneInternal, the new URI is actually created using the protocol handler nsImapService::NewURI, which creates the correct nsImapUrl.

In the patch, you've bypassed that roundabout polymorphic behavior in nsMsgMailNewsUrl::CloneInternal, so you've created an alternate way of specifying new URIs, which also requires maintenance (and cannot work polymorphically in an extension like the old way.)

I think (but have not actually confirmed) this is going to create serious, possibly insurmountable problems in JsAccount which relies on the quasi-polymorphic behavior to create a correct URI class from the XPCOM protocol handler, and will have no way to override StartClone().

Let me propose an alternative.

Comment 31

[Kent James (:rkent)]

Re Comment 0: "In bug 1431913 comment #17 Valentin suggested to use
  NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID).SetSpec(yyy).Finalize(zzz)
which I believe creates the correct object straight away."

In the original comment, there was an important clarifier: "NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID) - or other contract ID"

So the statement above "creates the correct object straight way" is not really accurate, the statement creates a standardURL. You would need to vary the contract ID to create any other kind of URL.

In the vast majority of cases in the code though NS_STANDARDURLMUTATOR_CONTRACTID is correct, because what is being created is what I call a parsing URL. The goal is not to create an actual URL that would be used in protocols, but instead you simply want to set the spec and then use that URL to parse various string parts of the URI according to nsIURI.

On another point, "Doing mutate() on an nsMsgMailNewsUrl appeared to cause this error: NS_ERROR_MALFORMED_URL" occurs when you create an empty base URL. An empty base URL needs SetSpecInternal to get it initial spec set correctly. But you have to be careful, if you change the protocol there you'll end up with a mismatch between spec and URL class.

This all represents my understanding that I've gained in the last few days trying to deal with the fallout of these issues myself, so of course I could have some misunderstandings.

Comment 32

[Jorg K (CEST = GMT+2)]

Posting this without having read comment #31 only in reply to comment #30:

Kent, I'm glad I caught your attention and we meet in this bug. Let me give you some background.

In bug 1431204 M-C made nsIURI.spec read-only. As a result, the SetSpec() methods were withdrawn. Instead a whole new system with so-called mutators is coming, more below. In bug 1431913 we landed this:
https://hg.mozilla.org/comm-central/rev/56a1e0215340 - Use SetSpecInternal() instead
https://hg.mozilla.org/comm-central/rev/603955ee98d3 - Some JS tweaks, some using .mutate()
https://hg.mozilla.org/comm-central/rev/866b5c60d753 - minor LDAP follow-up
https://hg.mozilla.org/comm-central/rev/1dd846f3e167 - Fix SMTP protocol handler (**)
There were some more landings, for chat and calendar, but they're not important here.

The main part of bug 1431913 is simply switching to SetSpecInternal() which works for the moment. While SetSpecInternal() has its valid uses, see comment #5 here, we are no using it is places where the new mutator scheme should be used.

Philipp had a much grander plan for bug 1431913, attachment 8944314 [details] [diff] [review] where he tried to switch to the new scheme immediately. For example take a look at his suggestion for nsImapService.cpp. Sadly he patch didn't work. Under bustage-fix pressure we ran out of time and went with "SetSpecInternal()" everywhere.

Now we're here to review those calls and see why Philipp's patch didn't work. Looking at bug 1431913 comment 59 which was about the original fix for the SMTP protocol handler, Valentin suggested that nsMsgMailNewsUrl::CloneInternal() might not work correctly. In the end, I landed (**) instead, see above.

The new mutators rely on a working clone, see 
https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/netwerk/base/nsIURIMutator.idl#37

So let me repeat: We originally started looking at the CloneInternal() since attachment 8944328 [details] [diff] [review] didn't work and we went for
https://hg.mozilla.org/comm-central/rev/1dd846f3e167

Coming back to your comment #30. You're saying that nsIOService::NewURI() takes the spec and goes looking for a protocol handler that will create the URI object. Fine. In that case our approach (quoting from attachment 8944328 [details] [diff] [review])
     newURI: function (aSpec, aOriginCharset, aBaseURI) {
       var url = Components.classes["@mozilla.org/messengercompose/smtpurl;1"]
                           .createInstance(Components.interfaces.nsIURI);
-      url.spec = aSpec;
+      url = url.mutate().setSpec(aSpec).finalize();
would have been a little fatal, since the .mutate() would have triggered another clone which would have again come the same place. I'm not sure why that didn't die an an infinite recursion.

So in summary: I'm happy for my patch in attachment 8949237 [details] [diff] [review] being undesired. If the clone already works, maybe there isn't a need to make any changes, or perhaps the m_principal and m_isPrincipalURL should be copied across.

I'm happy to learn what your alternative will be. While we're in clean-up mode, please review:
https://hg.mozilla.org/comm-central/rev/1dd846f3e167

Comment 33

[Jorg K (CEST = GMT+2)]

(In reply to Kent James (:rkent) from comment #31)
> Re Comment 0: "In bug 1431913 comment #17 Valentin suggested to use
>   NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID).SetSpec(yyy).Finalize(zzz)
> which I believe creates the correct object straight away."
> 
> In the original comment, there was an important clarifier:
> "NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID) - or other contract ID"
> 
> So the statement above "creates the correct object straight way" is not
> really accurate, the statement creates a standardURL. You would need to vary
> the contract ID to create any other kind of URL.
Yes, my comment #0 was referring to standard URLs only, and I have landed
https://hg.mozilla.org/comm-central/rev/941a1c2bda737375f1c3040e99d1a59531c4129f
in comment #13 that replaces "create + SetSpecInternal()" with one call.
I think there in no doubt about the correctness of that change.

> On another point, "Doing mutate() on an nsMsgMailNewsUrl appeared to cause
> this error: NS_ERROR_MALFORMED_URL" ...
Yes, this is what we observed in the SMTP patch and I didn't pursue it further.

> ... occurs when you create an empty base
> URL. An empty base URL needs SetSpecInternal to get it initial spec set
> correctly.
As I said: I didn't pursue the issue further.

Personally, I find the mutate() approach a little "wasteful". Why create a clone when you're trying to set up the thing in the first place. So my aim was to get the thing right from the start, which is what the standard URL patch quoted above represents and also the landed bits in nsSMTPProtocolHandler.js.

===

The next aim of this clean-up would be to visit all calls of SetSpecInternal() to decide what to do with them, like the ones in nsImapService.cpp. Apparently also SetPath() (now called SetPathQueryRef()), will go, see comment #5.

Comment 34

[Kent James (:rkent)]

(In reply to Jorg K (GMT+1) from comment #33)

> 
> > On another point, "Doing mutate() on an nsMsgMailNewsUrl appeared to cause
> > this error: NS_ERROR_MALFORMED_URL" ...
> Yes, this is what we observed in the SMTP patch and I didn't pursue it
> further.
> 
> > ... occurs when you create an empty base
> > URL. An empty base URL needs SetSpecInternal to get it initial spec set
> > correctly.
> As I said: I didn't pursue the issue further.

I'm not sure I was correct here anymore, I tried creating an empty standard or simple url then mutated,
and it worked fine. I'll need to investigate why I was having issues earlier.

> 
> Personally, I find the mutate() approach a little "wasteful". Why create a
> clone when you're trying to set up the thing in the first place. So my aim
> was to get the thing right from the start, which is what the standard URL
> patch quoted above represents and also the landed bits in
> nsSMTPProtocolHandler.js.

I believe that the base urls, if you create them from a url spec, don't really do a clone, even though they go through a mutator. I also do not really understand the deeper thinking behind using this type of constructor:

NS_MutateURI(NS_SIMPLEURIMUTATOR_CONTRACTID)
           .SetSpec(aSpec)
           .Finalize(_retval);

I suppose that the goal is to make uris immutable for thread safety.

I've now spent half a day on this, and I really can't figure out what needs doing, if anything. I suppose it would be worth at some point going through our code and refactoring it to more closely follow what they do in core Mozilla, but that is a big project, and it has little if any payoff as long as most of our protocols are main thread.

Imap is quite a mess, it really overloads the URL with a lot of state information. I suppose that we could refactor that so that the url is not created until we know the full spec, but right now that is not how it is done - they create a partial URL from a standard template, then add lots of state information including fixing the final spec.

So if you have some specific tasks I could consider them, but for now it seems to me it is better to chase regressions, if any.

Comment 35

[Jorg K (CEST = GMT+2)]

(In reply to Kent James (:rkent) from comment #34)
> So if you have some specific tasks I could consider them, but for now it
> seems to me it is better to chase regressions, if any.
As I said, review: https://hg.mozilla.org/comm-central/rev/1dd846f3e167

So what's the story about "Let me propose an alternative"? Are you now saying that we should leave the clone code "as is"?

> I really can't figure out what needs doing, if anything.
I think the aim is still refactoring to use less SetSpecInternal().

Comment 36

[Kent James (:rkent)]

I looked at https://hg.mozilla.org/comm-central/rev/1dd846f3e167 and it seems fine to me. For js protocol handlers, there really is little choice other than to define a C++ method accessible from JS that calls, one way or another, SetSpecInternal as is done here. I've got a patch for JaBaseCppUrl (bug 1418011) that I will need for JsAccount that takes a similar approach, I just call it SetSpec instead of init().

And I as going to propose an alternative to that other patch, but looking it over I don't see anything wrong with the old code, so I am proposing doing nothing.

Comment 37

[Jorg K (CEST = GMT+2)]

Comment on attachment 8949237 [details] [diff] [review]
1432204-nsMsgMailNewsUrl-clone.patch (v3) WIP

As per Kent's comments in comment #36 we're not going ahead with this. So we're done here. Any further follow up in bug 1440693.