1431913 - Port bug 1431204 to mailnews: Make nsIURI.spec readonly

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Richard Marti (:Paenglab)]

The build breaks with this error:
z:/Mozilla/comm-central/mailnews/local/src/nsLocalMailFolder.cpp(3115): error C2039: 'SetSpec': is not a member of 'nsIURL'
 2:45.66 z:\mozilla\tb\dist\include\nsIURL.h(25): note: see declaration of 'nsIURL'
 2:45.69 mozmake.EXE[4]: *** [z:/Mozilla/comm-central/mozilla/config/rules.mk:1049: nsLocalMailFolder.obj] Error 2

and on a lot more files.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: 1427092_composeAttachmentHeaderInPlaceAction.Take2.patch

This is a simple attempt to fix the bustage. But it still fails, also with other errors. Maybe a second bustage.

This needs to be done by someone which knows C++.

Comment 2

[Richard Marti (:Paenglab)]

Attachment: Bug1431913.patch

Oops, wrong patch

Comment 3

[Jorg K (CEST = GMT+2)]

This will need quite a bit of work since we call SetSpec() many times. I'll look at it in the afternoon.
At worse, something like this:
https://hg.mozilla.org/mozilla-central/rev/24e867a67e1f#l11.18

I'm not sure right now whether JS changes will be required as well, like here:
https://hg.mozilla.org/mozilla-central/rev/24e867a67e1f#l16.22

Comment 4

[Jorg K (CEST = GMT+2)]

Valentin, do we also need to change JS code? I haven't looked but we may use url.spec = ... in JS.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

JS code needs to replace
uri.spec = "something"
with
uri = uri.mutate().setSpec("something").finalize()

Comment 6

[Jorg K (CEST = GMT+2)]

OK, looking for |.spec = | I find these:
calendar:
.\providers\caldav\calDavCalendar.js:            calUri.spec = parts.shift();
.\providers\caldav\calDavCalendar.js:            calUri.spec = split2.join("/") + "/";
.\providers\wcap\calWcapSession.js:                        caluri.spec = defaultSpec + params;

editor, im, mail: none:

ldap:
.\xpcom\tests\unit\test_nsLDAPURL.js:  url2.spec = "ldap://localhost:389/dc=short??sub?(objectclass=*)";
.\xpcom\tests\unit\test_nsLDAPURL.js:  url.spec = "ldap://localhost/dc=short?" + newAttrs + "?one?(objectclass=*)";
.\xpcom\tests\unit\test_nsLDAPURL.js:  url.spec = "ldap://localhost/dc=short??one?(objectclass=*)";
.\xpcom\tests\unit\test_nsLDAPURL.js:  url.spec = "ldap://localhost/dc=short?abc,def,ghi,jkl?one?(objectclass=*)";

mailnews:
.\compose\src\nsSMTPProtocolHandler.js:      url.spec = aSpec;
.\extensions\newsblog\content\FeedUtils.jsm:      uri.spec = dt.mozGetDataAt(types[0], 0);
.\extensions\newsblog\content\FeedUtils.jsm:      uri.spec = dt.mozGetDataAt(types[1], 0).split("\n")[0];
.\extensions\newsblog\content\FeedUtils.jsm:          uri.spec = spec;
.\imap\test\unit\test_compactOfflineStore.js:    message.spec = URI.spec;
.\imap\test\unit\test_imapHdrStreaming.js:    message.spec = URI.spec;
.\imap\test\unit\test_imapStoreMsgOffline.js:    message.spec = URI.spec;
.\imap\test\unit\test_nsIMsgFolderListenerIMAP.js:    message.spec = URI.spec;
.\imap\test\unit\test_offlineCopy.js:    message.spec = URI.spec;
.\jsaccount\test\unit\test_fooUrl.js:    url.spec = "https://test.invalid/folder?isFoo=true&someBar=theBar";
.\jsaccount\test\unit\test_fooUrl.js:    url.spec = "https://test.invalid/folder#modules";
.\jsaccount\test\unit\test_fooUrl.js:    url.spec = "https://test.invalid/folder?isFoo=true&someBar=theBar";
.\jsaccount\test\unit\test_fooUrl.js:    url.QueryInterface(Ci.nsIURI).spec = "https://foo.invalid/bar/";
.\jsaccount\test\unit\test_fooUrl.js:    url.QueryInterface(Ci.nsIURI).spec = "https://foo.invalid/bar?part=1.4&dummy=stuff";
.\news\test\unit\test_bug540288.js:  uri.spec = "news://localhost:" + server.port + "/TSS1%40nntp.test";

We have our own classes for LDAP and Mailnews URLs, so I might implement SetSpec() for those and make that to the new M-C calls in C++ code so that our JS code wouldn't change. The calendar stuff needs looking at.

I'll start with C++.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1431913-url-readonly.patch - WIP (v1) - NOT WORKING!

This implements SetSpec() for out four URL classes:
nsILDAPURL, nsIAddbookUrl, nsIMsgMailNewsUrl and nsIMailtoUrl.

I've also replaced a two uses of of creating a standard URL and setting the spec with NS_NewURI() giving the desired spec to start with.

This compiles so far, but I identified 35 call sites marked FIXME where I simply commented out the call.

This bug is real hard to fix (on a weekend when I have no time at all) since TB uses URIs for everything and sets the spec left right and centre. All those 35 call sites need to be checked individually, but there will be some generic fixes.

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8944174 [details] [diff] [review]
1431913-url-readonly.patch - WIP (v1) - NOT WORKING!

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

I'm sorry this is such pain.
In the next few weeks I am going to be making more changes to all the attributes in nsIURI and associated interfaces:
https://gist.github.com/valenting/1f1550d756c01c9e732d8fcdfe9240b8

I'll do my best to split the breaking changes in separate patches so they can be reverted or delayed until MailNews consumers is fixed.
Changing all the call sites is quite boring and uninteresting, but since it doesn't require other knowledge of the code, maybe we can get some volunteers and crowd-source it.

Also, if there's anything else I can do to make your life easier, please let me know.

::: ldap/xpcom/public/nsILDAPURL.idl
@@ +19,5 @@
>   */
>  
>  [scriptable, builtinclass, uuid(8e3a6d33-2e68-40ba-8f94-6ac03f69066e)]
>  interface nsILDAPURL : nsIURI {
> +    void setSpec(in ACString aSpec);

Note that it's not necessary to add a new setSpec method to the interface.
setSpecInternal is already defined in nsIURI so all that needs to happen is for classes that implement nsIURI to have a method called SetSpecInternal - you can rename the old SetSpec to that.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1431913-url-readonly.patch - WIP (v2) - NOT WORKING!

Using SetSpecInternal() now, to be continued.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1431913-url-readonly.patch (v3)

This should do it. I went back to SetSpecInternal() like Richard already had it, I wonder while his didn't compile.

I'll do some final checks in a few hours.

Comment 11

[Jorg K (CEST = GMT+2)]

Make that: I wonder why his didn't compile.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 1431913-url-readonly.patch (v4)

This should be right now. All uses of "standard URL" + SetSpec() replaced with NS_NewURI(...) giving the correct spec straight away.

I'll fix the non-calendar JS bits now, try the tests locally and then this is going to land ;-)

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1431913-url-readonly.patch (v4b)

Oops, leftover from a previous experiment.

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 8944194 [details] [diff] [review]
1431913-url-readonly.patch (v4b)

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

Valentin, we have to fix a heap of code in a hurry, so what is the correct replacement?

::: mailnews/compose/src/nsSmtpService.cpp
@@ +175,5 @@
>  
>    nsCOMPtr<nsIMsgMailNewsUrl> url(do_QueryInterface(smtpUrl, &rv));
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  rv = url->SetSpecInternal(urlSpec);

Is it OK to call SetSpecInternal() here or should it be something along the lines of
NS_MutateURI(new nsIMsgMailNewsUrl::Mutator()).SetSpec(urlSpec).Finalize(url);

Comment 15

[Jorg K (CEST = GMT+2)]

Oh, while the nsIMsgMailNewsUrl::SetSpec calls can be replaced like this, for nsIURI::SetSpec can't be replaced with
  NS_MutateURI(new nsIURI::Mutator()).SetSpec(urlSpec).Finalize(url);
right?

Comment 16

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8944194 [details] [diff] [review]
1431913-url-readonly.patch (v4b)

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

> Is it OK to call SetSpecInternal() here or should it be something along the
> lines of
> NS_MutateURI(new
> nsIMsgMailNewsUrl::Mutator()).SetSpec(urlSpec).Finalize(url);

That would be the correct way, but for a quick way of avoiding build failures, calling SetSpecInternal is OK.
But there should be a plan in place to migrate to using the mutators as soon as possible.

Comment 17

[Valentin Gosu [:valentin] (he/him)]

(In reply to Jorg K (GMT+1) [Very limited availability 18-22 Jan 2018] from comment #15)
> Oh, while the nsIMsgMailNewsUrl::SetSpec calls can be replaced like this,
> for nsIURI::SetSpec can't be replaced with
>   NS_MutateURI(new nsIURI::Mutator()).SetSpec(urlSpec).Finalize(url);
> right?

You can still call nsIURI::SetSpecInternal for now.
NS_NewURI is good when creating a new URI without knowing the exact type.

Otherwise you can call the other mutators [1]
https://searchfox.org/mozilla-central/rev/b7e3ec2468d42fa59d86c03ec7afeb209813f1d4/netwerk/base/nsIURIMutator.idl#278-281

NS_MutateURI(nsIURI* aURI) - creates clone of aURI and you can change it
NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID) - or other contract ID - will instantiate a mutator that corresponds to a certain URI type.
or the one you mentioned, where you pass in the new mutator constructor directly.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: 1431913-js-part.patch

This fixes ldap/xpcom/tests/unit/test_nsLDAPURL.js.
Somehow we need to add instanceof Components.interfaces.nsILDAPURL, otherwise the attributes attribute is not found any more.

Comment 19

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Philipp's fix - v1

Hey Valentin, thanks so much for providing the help on this patch, we really appreciate your support. At your convenience, can you take a quick look at this patch to see if the general pattern is right?

I used the approach you mentioned for JS, and then mostly either:

+  rv = NS_MutateURI(mBaseURL)
+    .SetSpec(aSpec)
+    .Finalize(mBaseURL);

or something like:

+  return NS_MutateURI(new nsAddbookUrl::Mutator())
+    .SetSpec(aSpec)
+    .Finalize(_retval);

or for the standard url:

+  rv = NS_MutateURI(new mozilla::net::nsSimpleURI::Mutator())
+    .SetSpec(mURI)
+    .Finalize(url);

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: 1431913-js-part.patch

This *really* fixes all the tests touched here. I ran them.

I'm wondering what to do with nsSMTPProtocolHandler.js since as the LDAP test shows, we need to do an instanceof() later. So I don't think Philipp's will actually return a URL of the right class.

Also, I think in many cases we can create the URI with the right spec and don't need to set it later.

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: 1431913-js-part.patch (v2) [landed comment #35]

I think this is right. This doesn't cover test_fooUrl.js since that is very complicated stuff that needs a close look.

Comment 22

[Jorg K (CEST = GMT+2)]

There is more work here. Philipp's patch doesn't compile and my C++ patch throws up a million errors from nsMailboxUrl::ParseUrl(). GetFilePath(m_file) returns a leaf name, not a path. Maybe related to the path-less URL variant mailbox://nobody@local%20folders/xxx.

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: Philipp's fix - v1b - now compiling on Windows

Made it compile on Windows. With this patch, TB doesn't even show the folder pane. I don't have time to debug two patches. IMHO we need something that fixes the bustage quickly and can worry about replacing the SetSpecInternal() calls later, see Valentin's comment #16. We'd really need to inspect all calls to decide whether/how to replace them.

Comment 24

[Jorg K (CEST = GMT+2)]

Let's see how far this takes us:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=58f817f2d198c5f9b098a36f6e4ae96cfbf63b3d
This is with the failures in nsMailboxUrl::ParseUrl().

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: 1431913-url-readonly.patch (v4c)

Didn't compile one Mac:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f2c5a52c4a72a8e6fb2fc12f51021ab31be55928

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 8944228 [details] [diff] [review]
Philipp's fix - v1b - now compiling on Windows

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

Looking at the patch, I realised that I missed smileProtocolHandler.js. I'm not sure the replacement is right. I've done something else in nsSMTPProtocolHandler.js which is basically copied from the LDAP equivalent.

I really believe that we need to look at all cases individually and that the "global find and replace" approach isn't right, unless it's meant to be temporary like when using SetSpecInternal().

May I suggest not to pursue two "competing" patches here which have quite an overlap. As soon as I have something that works half-way, I'll land that and we can worry about the SetSpecInternal() calls later.

As I said before, I don't think there is much point to maintain "make url" and "set spec" when that can be replaced by "make uri with the desired spec". As I understand it, the spec can't be changed now (unless using the internal call) and mutate() actually makes a clone. So why make a clone of something that was created with the wrong spec and then correct it, if we can create the right thing from the start.

::: calendar/providers/wcap/calWcapSession.js
@@ +80,5 @@
>                  if (!regCal.isDefaultCalendar) {
>                      let [spec, params] = splitUriParams(regCal.uri);
>                      if (spec != defaultSpec) {
>                          log("fixing url of subscribed calendar: " + regCal.calId, session);
> +                        let caluri = regCal.uri.clone().mutate().setSpec(defaultSpec + params).finalize();

If I understand it correctly, the .mutate() already does a clone (see comment #17). So I think you can lose the .clone() here.

::: mailnews/base/util/nsMsgMailNewsUrl.cpp
@@ +427,5 @@
>  
>    // Now, set the rest.
> +  nsresult rv = NS_MutateURI(new mozilla::net::nsSimpleURI::Mutator())
> +    .SetSpec(aSpec)
> +    .Finalize(m_baseURL);

You're actually *in* nsMsgMailNewsUrl::SetSpecInternal() here. So why would you do a clone here? I think this is one of the places where SetSpecInternal() must be used.

Comment 27

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f2c5a52c4a72a8e6fb2fc12f51021ab31be55928
has quite a few test failures :-(

TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_bug155172.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_bug155172.js | - "EHLO test,AUTH PLAIN AHRlc3Quc210cEBmYWtlc2VydmVyAHNtdHB0ZXN0,AUTH LOGIN" == "EHLO test,AUTH PLAIN AHRlc3Quc210cEBmYWtlc2VydmVyAHNtdHB0ZXN0,AUTH LOGIN,AUTH PLAIN AHRlc3Quc210cEBmYWtlc2VydmVyAHdyb25n,MAIL F
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_smtpURL.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_smtpPasswordFailure2.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_smtpPasswordFailure2.js | - "EHLO test,AUTH PLAIN AHRlc3RzbXRwAHNtdHB0ZXN0,AUTH LOGIN,AUTH PLAIN AHRlc3RzbXRwAHNtdHB0ZXN0,AUTH LOGIN" == "EHLO test,AUTH PLAIN AHRlc3RzbXRwAHNtdHB0ZXN0,AUTH LOGIN,AUTH PLAIN AHRlc3RzbXRwAHNtdHB0ZXN0,AUTH LOGIN,AUTH PLAIN AHRlc3RzbXRw
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_smtpProtocols.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_smtpPasswordFailure3.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_smtpPasswordFailure3.js | - 2153066792 == 0
TEST-UNEXPECTED-FAIL | comm/mailnews/imap/test/unit/test_dod.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/jsaccount/test/unit/test_fooUrl.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/jsaccount/test/unit/test_fooUrl.js | test_nsIURI - [test_nsIURI : 53] "" == "test.invalid"
TEST-UNEXPECTED-FAIL | comm/mailnews/jsaccount/test/unit/test_jaMsgFolder.js | xpcshell return code: 0
TEST-UNEXPECTED-TIMEOUT | comm/mailnews/base/test/unit/test_copyThenMoveManual.js | Test timed out
TEST-UNEXPECTED-TIMEOUT | comm/mailnews/base/test/unit/test_junkWhitelisting.js | Test timed out
TEST-UNEXPECTED-TIMEOUT | comm/mailnews/base/test/unit/test_quarantineFilterMove.js | Test timed out
TEST-UNEXPECTED-TIMEOUT | comm/mailnews/base/test/unit/test_postPluginFilters.js | Test timed out
TEST-UNEXPECTED-TIMEOUT | comm/mailnews/imap/test/unit/test_localToImapFilterQuarantine.js | Test timed out
TEST-UNEXPECTED-TIMEOUT | comm/mailnews/imap/test/unit/test_localToImapFilterQuarantine.js | Test timed out
TEST-UNEXPECTED-TIMEOUT | comm/mailnews/local/test/unit/test_bug457168.js | Test timed out
TEST-UNEXPECTED-TIMEOUT | comm/mailnews/local/test/unit/test_duplicateKey.js | Test timed out
TEST-UNEXPECTED-TIMEOUT | comm/mailnews/local/test/unit/test_pop3AuthMethods.js | Test timed out
TEST-UNEXPECTED-TIMEOUT | comm/mailnews/local/test/unit/test_pop3DownloadTempFileHandling.js | Test timed out
TEST-UNEXPECTED-TIMEOUT | comm/mailnews/local/test/unit/test_pop3Download.js | Test timed out
TEST-UNEXPECTED-TIMEOUT | comm/mailnews/local/test/unit/test_pop3Duplicates.js | Test timed out
TEST-UNEXPECTED-TIMEOUT | comm/mailnews/local/test/unit/test_pop3FilterActions.js | Test timed out
TEST-UNEXPECTED-TIMEOUT | comm/mailnews/local/test/unit/test_pop3GSSAPIFail.js | Test timed out

Comment 28

[Jorg K (CEST = GMT+2)]

Attachment: 1431913-url-readonly.patch (v5) [landed comment #35 without the nsSMTPProtocolHandler.js hunk]

Reverting the removal of the use of the standard URL works a whole lot better locally and those errors go away and the test I tried was passing again:

Let's see:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4c4b6ed4f66f534c3687a9bfa5a028efa893ffd6

Let's hope there is no other bustage from the last merge.

Comment 29

[Jorg K (CEST = GMT+2)]

Comment on attachment 8944214 [details] [diff] [review]
1431913-js-part.patch (v2) [landed comment #35]

Alta88, is the feeds part correct? I don't think you need a "standard URL" there for what you're doing.

Comment 30

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

> 
> I really believe that we need to look at all cases individually and that the
> "global find and replace" approach isn't right, unless it's meant to be
> temporary like when using SetSpecInternal().
> 
> May I suggest not to pursue two "competing" patches here which have quite an
> overlap. As soon as I have something that works half-way, I'll land that and
> we can worry about the SetSpecInternal() calls later.
I don’t see our patches in competition, in the end I am interested in pushing the best possible approach which is likely a combination of our patches. This wasn’t an automated replace, I checked each instance separately. I am not too a fan of pushing a band-aid, especially since I suspect the right approach is not far.  My patch does need some work though thanks for picking it up.
> 
> As I said before, I don't think there is much point to maintain "make url"
> and "set spec" when that can be replaced by "make uri with the desired
> spec". As I understand it, the spec can't be changed now (unless using the
> internal call) and mutate() actually makes a clone. So why make a clone of
> something that was created with the wrong spec and then correct it, if we
> can create the right thing from the start.
I’ll let Valentin comment on that since I followed the m-c path there. I suspect that newURI has some extra logic that makes setting the spec faster

> 
> ::: calendar/providers/wcap/calWcapSession.js
> @@ +80,5 @@
> >                  if (!regCal.isDefaultCalendar) {
> >                      let [spec, params] = splitUriParams(regCal.uri);
> >                      if (spec != defaultSpec) {
> >                          log("fixing url of subscribed calendar: " + regCal.calId, session);
> > +                        let caluri = regCal.uri.clone().mutate().setSpec(defaultSpec + params).finalize();
> 
> If I understand it correctly, the .mutate() already does a clone (see
> comment #17). So I think you can lose the .clone() here.
Agreed, thanks
> 
> ::: mailnews/base/util/nsMsgMailNewsUrl.cpp
> @@ +427,5 @@
> >  
> >    // Now, set the rest.
> > +  nsresult rv = NS_MutateURI(new mozilla::net::nsSimpleURI::Mutator())
> > +    .SetSpec(aSpec)
> > +    .Finalize(m_baseURL);
> 
> You're actually *in* nsMsgMailNewsUrl::SetSpecInternal() here. So why would
> you do a clone here? I think this is one of the places where
> SetSpecInternal() must be used.
Because the url is not part of the class, but rather a member, so from the perspective of nsMsgMailNewsUrl this is an external url

Comment 31

[Jorg K (CEST = GMT+2)]

(In reply to Philipp Kewisch [:Fallen]  from comment #30)
> I am not too a fan of pushing a band-aid, especially since I
> suspect the right approach is not far.
What is really not far is branch day, it's tomorrow. I'm not a fan of carrying bustage across and then having to fix the beta via uplifts. What you call a "band-aid", is a valid f+ option which can be replaced in TB 60 with peace. We can inspect all the uses of SetSpecInternal() and replace the ones which aren't future proof.

In the meantime it would be great of you could build with my two patches so you get a functioning application and then make sure that the two calendar files are correct so they can be landed. We wanted to lose the |.clone()| in one of them. For bonus points check that test_fooUrl.js passes with your patch ;-) As I stated previously here or on IRC, the other tests you fixed won't pass like that.

Comment 32

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4c4b6ed4f66f534c3687a9bfa5a028efa893ffd6
reduces the test failures to 12:
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_bug155172.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_bug155172.js | - "EHLO test,AUTH PLAIN AHRlc3Quc210cEBmYWtlc2VydmVyAHNtdHB0ZXN0,AUTH LOGIN" == "EHLO test,AUTH PLAIN AHRlc3Quc210cEBmYWtlc2VydmVyAHNtdHB0ZXN0,AUTH LOGIN,AUTH PLAIN AHRlc3Quc210cEBmYWtlc2VydmVyAHdyb25n,MAIL F
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_smtpURL.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_smtpPasswordFailure2.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_smtpPasswordFailure2.js | - "EHLO test,AUTH PLAIN AHRlc3RzbXRwAHNtdHB0ZXN0,AUTH LOGIN,AUTH PLAIN AHRlc3RzbXRwAHNtdHB0ZXN0,AUTH LOGIN" == "EHLO test,AUTH PLAIN AHRlc3RzbXRwAHNtdHB0ZXN0,AUTH LOGIN,AUTH PLAIN AHRlc3RzbXRwAHNtdHB0ZXN0,AUTH LOGIN,AUTH PLAIN AHRlc3RzbXRw
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_smtpPasswordFailure3.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_smtpPasswordFailure3.js | - 2153066792 == 0
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_smtpProtocols.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/imap/test/unit/test_dod.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/imap/test/unit/test_dod.js | streamMessages - [streamMessages : 99] false == true
TEST-UNEXPECTED-FAIL | comm/mailnews/jsaccount/test/unit/test_fooUrl.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/jsaccount/test/unit/test_fooUrl.js | test_nsIURI - [test_nsIURI : 53] "" == "test.invalid"

test_fooUrl.js is expected since I haven't touched it, and the SMTP failures may be due to the change I made in nsSMTPProtocolHandler.js. So I'll try Philipp's |url.mutate().setSpec(aSpec).finalize();| maybe followed by an instanceof.

Comment 33

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8944228 [details] [diff] [review]
Philipp's fix - v1b - now compiling on Windows

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

::: calendar/providers/wcap/calWcapSession.js
@@ +80,5 @@
>                  if (!regCal.isDefaultCalendar) {
>                      let [spec, params] = splitUriParams(regCal.uri);
>                      if (spec != defaultSpec) {
>                          log("fixing url of subscribed calendar: " + regCal.calId, session);
> +                        let caluri = regCal.uri.clone().mutate().setSpec(defaultSpec + params).finalize();

correct. if the mutator is implemented correctly (haven't checked) no clone necessary.

::: chat/components/src/smileProtocolHandler.js
@@ +20,5 @@
>                   Ci.nsIProtocolHandler.URI_IS_UI_RESOURCE |
>                   Ci.nsIProtocolHandler.URI_IS_LOCAL_RESOURCE,
>    newURI: function SPH_newURI(aSpec, aOriginCharset, aBaseURI) {
>      let uri = Cc["@mozilla.org/network/simple-uri;1"].createInstance(Ci.nsIURI);
> +    return uri.mutate().setSpec(aSpec).finalize();

You might still want to make the URI immutable.
Finalize will do that in the future, but it doesn't make it immutable right now.

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +3131,5 @@
>  {
>    nsresult rv;
>    nsCOMPtr<nsIURL> url;
>  
> +  rv = NS_MutateURI(new mozilla::net::nsSimpleURI::Mutator())

Initially it was standard-url, not simple URL.
You can call NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID) instead

::: mailnews/base/util/nsMsgMailNewsUrl.cpp
@@ +207,5 @@
>  
>    rv = m_baseURL->GetSpec(urlstr);
>    NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = NS_MutateURI(new mozilla::net::nsSimpleURI::Mutator())

again, initial code used StandardURL not simpleURL

@@ +425,5 @@
>        mAttachmentFileName = start+FILENAME_PART_LEN;
>    }
>  
>    // Now, set the rest.
> +  nsresult rv = NS_MutateURI(new mozilla::net::nsSimpleURI::Mutator())

NS_MutateURI(m_baseURL).SetSpec(aSpec).Finalize(m_baseURL) should also work

::: mailnews/compose/src/nsMsgAttachmentHandler.cpp
@@ +855,5 @@
>    // if we have a resource fork, check the filename extension, maybe we don't need the resource fork!
>    if (sendResourceFork)
>    {
> +    nsCOMPtr<nsIURL> fileUrl;
> +    rv = NS_MutateURI(new mozilla::net::nsSimpleURI::Mutator())

standardURL not simple URI

::: mailnews/compose/src/nsMsgCopy.cpp
@@ +541,5 @@
>  
>    if (!aFolderURI) return NS_ERROR_NULL_POINTER;
>  
> +  nsCOMPtr<nsIURL> url;
> +  rv = NS_MutateURI(new mozilla::net::nsSimpleURI::Mutator())

standardURL not simpleURI
There are a bunch of these mismatches, make sure to fix them all.

::: mailnews/news/src/nsNewsFolder.cpp
@@ +1107,5 @@
>      nsCString serverURI;
>      rv = server->GetServerURI(serverURI);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    rv = NS_MutateURI(new mozilla::net::nsSimpleURI::Mutator())

standard, not simple

::: mailnews/news/src/nsNntpService.cpp
@@ +1219,5 @@
>      if (aBaseURI)
>      {
>        nsAutoCString newSpec;
>        aBaseURI->Resolve(aSpec, newSpec);
> +      return NS_MutateURI(new nsMsgMailNewsUrl::Mutator())

Does this mutator return the same as NS_NNTPURL_CONTRACTID?

Comment 34

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8944228 [details] [diff] [review]
Philipp's fix - v1b - now compiling on Windows

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

::: calendar/providers/wcap/calWcapSession.js
@@ +80,5 @@
>                  if (!regCal.isDefaultCalendar) {
>                      let [spec, params] = splitUriParams(regCal.uri);
>                      if (spec != defaultSpec) {
>                          log("fixing url of subscribed calendar: " + regCal.calId, session);
> +                        let caluri = regCal.uri.clone().mutate().setSpec(defaultSpec + params).finalize();

correct. if the mutator is implemented correctly (haven't checked) no clone necessary.

::: chat/components/src/smileProtocolHandler.js
@@ +20,5 @@
>                   Ci.nsIProtocolHandler.URI_IS_UI_RESOURCE |
>                   Ci.nsIProtocolHandler.URI_IS_LOCAL_RESOURCE,
>    newURI: function SPH_newURI(aSpec, aOriginCharset, aBaseURI) {
>      let uri = Cc["@mozilla.org/network/simple-uri;1"].createInstance(Ci.nsIURI);
> +    return uri.mutate().setSpec(aSpec).finalize();

You might still want to make the URI immutable.
Finalize will do that in the future, but it doesn't make it immutable right now.

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +3131,5 @@
>  {
>    nsresult rv;
>    nsCOMPtr<nsIURL> url;
>  
> +  rv = NS_MutateURI(new mozilla::net::nsSimpleURI::Mutator())

Initially it was standard-url, not simple URL.
You can call NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID) instead

::: mailnews/base/util/nsMsgMailNewsUrl.cpp
@@ +207,5 @@
>  
>    rv = m_baseURL->GetSpec(urlstr);
>    NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = NS_MutateURI(new mozilla::net::nsSimpleURI::Mutator())

again, initial code used StandardURL not simpleURL

@@ +425,5 @@
>        mAttachmentFileName = start+FILENAME_PART_LEN;
>    }
>  
>    // Now, set the rest.
> +  nsresult rv = NS_MutateURI(new mozilla::net::nsSimpleURI::Mutator())

NS_MutateURI(m_baseURL).SetSpec(aSpec).Finalize(m_baseURL) should also work

::: mailnews/compose/src/nsMsgAttachmentHandler.cpp
@@ +855,5 @@
>    // if we have a resource fork, check the filename extension, maybe we don't need the resource fork!
>    if (sendResourceFork)
>    {
> +    nsCOMPtr<nsIURL> fileUrl;
> +    rv = NS_MutateURI(new mozilla::net::nsSimpleURI::Mutator())

standardURL not simple URI

::: mailnews/compose/src/nsMsgCopy.cpp
@@ +541,5 @@
>  
>    if (!aFolderURI) return NS_ERROR_NULL_POINTER;
>  
> +  nsCOMPtr<nsIURL> url;
> +  rv = NS_MutateURI(new mozilla::net::nsSimpleURI::Mutator())

standardURL not simpleURI
There are a bunch of these mismatches, make sure to fix them all.

::: mailnews/news/src/nsNewsFolder.cpp
@@ +1107,5 @@
>      nsCString serverURI;
>      rv = server->GetServerURI(serverURI);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    rv = NS_MutateURI(new mozilla::net::nsSimpleURI::Mutator())

standard, not simple

::: mailnews/news/src/nsNntpService.cpp
@@ +1219,5 @@
>      if (aBaseURI)
>      {
>        nsAutoCString newSpec;
>        aBaseURI->Resolve(aSpec, newSpec);
> +      return NS_MutateURI(new nsMsgMailNewsUrl::Mutator())

Does this mutator return the same as NS_NNTPURL_CONTRACTID?

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/56a1e0215340
Port bug 1431204 to mailnews: Make nsIURI.spec readonly. rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/603955ee98d3
Port bug 1431204 to mailnews: Make nsIURI.spec readonly (JS part). rs=bustage-fix CLOSED TREE

Comment 36

[Jorg K (CEST = GMT+2)]

Given the time constraints I've landed what I had and what is known to be working apart from SMTP. I didn't land the change to since that wasn't right.

I'll rebase Philipp's patch now so we can see where the two approaches differ.

Comment 37

[Jorg K (CEST = GMT+2)]

Attachment: Philipp's fix - v1b - rebased

I've rebased Philipp's patch to what I've landed so we can see the difference in the two approached more clearly. Common hunks have of course disappeared.

As per Valentin's feedback, there is work to do, so I suggest to move this as the starting point to a follow-up bug.

Comment 38

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Valentin Gosu [:valentin] from comment #33)
> 
> ::: chat/components/src/smileProtocolHandler.js
> @@ +20,5 @@
> >                   Ci.nsIProtocolHandler.URI_IS_UI_RESOURCE |
> >                   Ci.nsIProtocolHandler.URI_IS_LOCAL_RESOURCE,
> >    newURI: function SPH_newURI(aSpec, aOriginCharset, aBaseURI) {
> >      let uri = Cc["@mozilla.org/network/simple-uri;1"].createInstance(Ci.nsIURI);
> > +    return uri.mutate().setSpec(aSpec).finalize();
> 
> You might still want to make the URI immutable.
> Finalize will do that in the future, but it doesn't make it immutable right
> now.

Hmm, this seems like a cludge codewise, but I guess we could do it via helper function. Does this sound right? Or can we get away with assuming finalize will take care of this at a later time?

function setUriSpec(uri, spec) {
  let newUri = uri.mutate().setSpec(spec).finalize();
  newUri.QueryInterface(Ci.nsIMutable).mutable = false;
  return newUri;
}

Do we also have to do something like this in C++? I didn't see any of this in the m-c patches.



> 
> ::: mailnews/base/util/nsMsgDBFolder.cpp
> @@ +3131,5 @@
> >  {
> >    nsresult rv;
> >    nsCOMPtr<nsIURL> url;
> >  
> > +  rv = NS_MutateURI(new mozilla::net::nsSimpleURI::Mutator())
> 
> Initially it was standard-url, not simple URL.
> You can call NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID) instead
Ah thanks, for some reason I thought this was the same thing. Fixing all instances.

> 
> ::: mailnews/news/src/nsNntpService.cpp
> @@ +1219,5 @@
> >      if (aBaseURI)
> >      {
> >        nsAutoCString newSpec;
> >        aBaseURI->Resolve(aSpec, newSpec);
> > +      return NS_MutateURI(new nsMsgMailNewsUrl::Mutator())
> 
> Does this mutator return the same as NS_NNTPURL_CONTRACTID?
Yeah I was not sure about this part. nsMsgMailNewsUrl is the base class for nsNntpUrl. nsNntpUrl does not have a mutator, and from the looks of it also does not have any setters. Would that be sufficient or do I need to do something fancy?

Comment 39

[Jorg K (CEST = GMT+2)]

Attachment: 1431913-smtp.patch - WIP (v1) - NOT WORKING

As per comment #32, all but two failures are in SMTP. The hunk in  nsSMTPProtocolHandler.js is as mentioned in comment #5 and also to be found in Philipp's patch. Sadly it doesn't work, neither without nor with the additional C++ code. The error shown is:

 0:01.98 pid:5512 JavaScript error: file:///c:/mozilla-source/comm-central/obj-i686-pc-mingw32/dist/bin/components/nsSMTPProtocolHandler.js, line 27: NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIURI.mutate]
 0:01.99 ERROR NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIURI.mutate]

Also, while we're in feedback mode, Valentin, I had to add 'instanceof' here:
https://hg.mozilla.org/comm-central/rev/603955ee98d37a04c231db0c5025f20b7d934a1b#l1.33
to make the test pass. Is that expected?

Philipp, if you keep working on the rebased patch, I think you can remove the hunks in the IMAP and News tests. message.mutate() threw an error and what I've landed fixes the tests in a sustainable fashion, so I think we're done with those. I also found it quite surprising to alter the "loop" variable, so I hope you like my solution:
https://hg.mozilla.org/comm-central/rev/603955ee98d37a04c231db0c5025f20b7d934a1b#l3.12

Sorry about the rush of which neither you nor I are a fan, but I'd like to get something running before tomorrow if possible.

Comment 40

[Valentin Gosu [:valentin] (he/him)]

(In reply to Philipp Kewisch [:Fallen]  from comment #38)
> > You might still want to make the URI immutable.
> > Finalize will do that in the future, but it doesn't make it immutable right
> > now.
> 
> Hmm, this seems like a cludge codewise, but I guess we could do it via
> helper function. Does this sound right? Or can we get away with assuming
> finalize will take care of this at a later time?
> 
> function setUriSpec(uri, spec) {
>   let newUri = uri.mutate().setSpec(spec).finalize();
>   newUri.QueryInterface(Ci.nsIMutable).mutable = false;
>   return newUri;
> }

This code is correct. I think we should do this for now, as finalize will not set it to be immutable until we finish with converting every setter to use mutators.

> Do we also have to do something like this in C++? I didn't see any of this
> in the m-c patches.

In C++, the best way of doing this is to call NS_TryToSetImmutable(uri) after finalizing.

> > > +      return NS_MutateURI(new nsMsgMailNewsUrl::Mutator())
> > 
> > Does this mutator return the same as NS_NNTPURL_CONTRACTID?
> Yeah I was not sure about this part. nsMsgMailNewsUrl is the base class for
> nsNntpUrl. nsNntpUrl does not have a mutator, and from the looks of it also
> does not have any setters. Would that be sufficient or do I need to do
> something fancy?

If it doesn't have a mutator it means it can't be constructed via a mutator.
Technically there should be a mutator for every nsIURI implementation.
Until a mutator is added, we can get away with calling SetSpecInternal on the Nntp URI.

Comment 41

[Valentin Gosu [:valentin] (he/him)]

(In reply to Jorg K (GMT+1) [Very limited availability 18-22 Jan 2018] from comment #39)
> Also, while we're in feedback mode, Valentin, I had to add 'instanceof' here:
> https://hg.mozilla.org/comm-central/rev/
> 603955ee98d37a04c231db0c5025f20b7d934a1b#l1.33
> to make the test pass. Is that expected?

Is tat the same as QueryInterface?
Since finalize returns nsIURI, we usually need to call something like .finalize().QueryInterface(Ci.nsIOtherInterface)
to get the correct object type.

Comment 42

[:aceman]

Comment on attachment 8944214 [details] [diff] [review]
1431913-js-part.patch (v2) [landed comment #35]

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

::: ldap/xpcom/tests/unit/test_nsLDAPURL.js
@@ +261,5 @@
>    // Set attributes via the url spec
>  
>    newAttrs = "abc,def,ghi,jkl";
> +  url = url.mutate().setSpec("ldap://localhost/dc=short?" + newAttrs + "?one?(objectclass=*)").finalize();
> +  url instanceof Components.interfaces.nsILDAPURL;

Maybe it would be cleaner to just use a new var and create a new URL instead of all this mess.

::: mailnews/compose/src/nsSMTPProtocolHandler.js
@@ +23,5 @@
>      newURI: function (aSpec, aOriginCharset, aBaseURI) {
>        var url = Components.classes["@mozilla.org/messengercompose/smtpurl;1"]
>                            .createInstance(Components.interfaces.nsIURI);
>  
> +      if (url instanceof Components.interfaces.nsISmtpUrl)

Would it work to do this directly?
var url = Components.classes["@mozilla.org/messengercompose/smtpurl;1"]
.createInstance(Components.interfaces.nsISmtpUrl)

::: mailnews/imap/test/unit/test_compactOfflineStore.js
@@ +48,2 @@
>    {
> +    mailbox.addMessage(new imapMessage(spec, mailbox.uidnext++, []));

It looks like these loops could be merged. Then even 'specs' wouldn't be needed.

Comment 43

[:aceman]

(In reply to Valentin Gosu [:valentin] from comment #41)
> Is tat the same as QueryInterface?
> Since finalize returns nsIURI, we usually need to call something like
> .finalize().QueryInterface(Ci.nsIOtherInterface)
> to get the correct object type.

AFAIK instanceof does an implicit QueryInterface to the given interface internally so the effects may be the same.
I would also prefer doing the .QueryInterface() directly if we know what interface we want and are not checking what the object is and also the 'var instanceof Ci.interface;' do look strange as standalone expressions.

Comment 44

[:aceman]

Comment on attachment 8944304 [details] [diff] [review]
1431913-smtp.patch - WIP (v1) - NOT WORKING

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

::: mailnews/compose/src/nsSMTPProtocolHandler.js
@@ +23,5 @@
>      newURI: function (aSpec, aOriginCharset, aBaseURI) {
>        var url = Components.classes["@mozilla.org/messengercompose/smtpurl;1"]
>                            .createInstance(Components.interfaces.nsIURI);
>  
> +      url = url.mutate().setSpec(aSpec).finalize();

If it doesn't work, do you need to .Qi(Ci.nsISmtpUrl) this as Valentin suggests? Maybe directly after creating so that the C++ code in nsSmtpUrl.h is run.

Comment 45

[Jorg K (CEST = GMT+2)]

(In reply to Valentin Gosu [:valentin] from comment #41)
> > Also, while we're in feedback mode, Valentin, I had to add 'instanceof' here:
> Is that the same as QueryInterface?
Yes, |xx instanceof yy| returns a bool, but it also modifies xx via some XPCOM magic to be "uplifted" to yy.

> Since finalize returns nsIURI, we usually need to call something like
> .finalize().QueryInterface(Ci.nsIOtherInterface)
> to get the correct object type.
Well, that explains the failure and why my fix worked. I might want to switch to the .QueryInterface() in a follow-up for added clarity.

The question which is more pressing right now is how to fix the SMTP issue which I tried (and failed) in attachment 8944304 [details] [diff] [review]. I guess that would also need the QI appended, but first we need to understand the error described in comment #39.

Comment 46

[:aceman]

Comment on attachment 8944304 [details] [diff] [review]
1431913-smtp.patch - WIP (v1) - NOT WORKING

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

::: mailnews/compose/src/nsSMTPProtocolHandler.js
@@ +23,5 @@
>      newURI: function (aSpec, aOriginCharset, aBaseURI) {
>        var url = Components.classes["@mozilla.org/messengercompose/smtpurl;1"]
>                            .createInstance(Components.interfaces.nsIURI);
>  
> +      url = url.mutate().setSpec(aSpec).finalize();

Also why can't this one be set directly to the right spec as you did in the other tests, e.g.
ioService.newURI(aSpec).QueryInterface(Ci.nsISmtpURL); ?

Comment 47

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #42)
> Maybe it would be cleaner to just use a new var and create a new URL instead
> of all this mess.
We're in bustage-fix more right now with branch day lurking tomorrow. Also I think setting the spec is a desired test case since they retrieve their special "attributes" later.
> 
> ::: mailnews/compose/src/nsSMTPProtocolHandler.js
> Would it work to do this directly?
That hunk didn't land. url.init() didn't work since there is no nsSmtpUrl:Init() function. This was copied from LDAP where nsLDAPURL::Init() exists. I wrote this before, but I lost the comment.

> It looks like these loops could be merged. Then even 'specs' wouldn't be
> needed.
Again, I wasn't looking for ways to make the code prettier, I was here to fix a heap of test failures.

I'll follow up on the QI issue in the LDAP test and try to get the SMTP stuff working.

Comment 48

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #44)
> >      newURI: function (aSpec, aOriginCharset, aBaseURI) {
> >        var url = Components.classes["@mozilla.org/messengercompose/smtpurl;1"]
> >                            .createInstance(Components.interfaces.nsIURI);
> >  
> > +      url = url.mutate().setSpec(aSpec).finalize();
> 
> If it doesn't work, do you need to .Qi(Ci.nsISmtpUrl) this as Valentin
> suggests? Maybe directly after creating so that the C++ code in nsSmtpUrl.h
> is run.
I'm not 100% sure about the semantic of 
> >        var url = Components.classes["@mozilla.org/messengercompose/smtpurl;1"]
> >                            .createInstance(Components.interfaces.nsIURI);
but I believe that does create a instance of the requested class and not nsIURI. So another .QI() wouldn't be correct.

> Also why can't this one be set directly to the right spec as you did in the
> other tests, e.g.
> ioService.newURI(aSpec).QueryInterface(Ci.nsISmtpURL); ?
I don't think that is correct. If you first created your object as subclass of nsIURI but pass the pointer around as pointing to the superclass, then you can query it back to the subclass.

If you create an nsIURI first, then the initialisation in the subclass won't run. Doing the .QI() will not fix that fact.

Perhaps his bug gives us the opportunity to learn more about how the stuff hangs together. Kent is a specialist with this test_fooUrl.js. That is tough stuff that's why I haven't even looked at it. Besides, only the test is broken, for SMTP the whole function is broken.

Comment 49

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8944304 [details] [diff] [review]
1431913-smtp.patch - WIP (v1) - NOT WORKING

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

I don't know enough about the Mutator class to know if it is that simple to just add in the Mutator, given it is already in the base class. The SMTP protocol handler part sounds right, because it is similar in C++ code of the m-c patch.

Comment 50

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Philipp's fix - v2 - parts will be moved to another bug.

I'm attaching this just for reference, I've spent about all the time I currently have on this and it needs someone with more regular C++ and Mozilla Platform experience to move forward. It adds contract ids for a few mutators, which makes use of NS_MutateURI easier.


Things left to do:
* figure out why it is crashing tests, I suspect I made a mistake with the pointers
* Add mutators for subclasses of nsMsgMailNewsUrl
* Fix callers of subclasses of nsMsgMailNewsUrl to use correct mutators and not setSpecInternal
* Find a solution for making urls immutable again, or consider assuming that finalize will eventually do this.

Jörg, if you are not picking this up in the current bug I'd appreciate if you could file a followup bug.

Comment 51

[Valentin Gosu [:valentin] (he/him)]

Note that we can backout the patch that changes the nsIURI interface until after the merge day.
There's no real requirement to have it in this release.
After everything is fixed in mailnews, I can land it again. In the meantime I could help with documentation and feedback as to how to properly implement the mutators and change the code.

Comment 52

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Thanks again the support Valentin! 

Jörg: also, I'd appreciate if you could pick the calendar parts out of my v2 patch. If you prefer I can land this in a separate bug though.

Comment 53

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+1) [Very limited availability 18-22 Jan 2018] from comment #48)
> > ioService.newURI(aSpec).QueryInterface(Ci.nsISmtpURL); ?
> I don't think that is correct.
Tried that:
 0:02.07 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService.newURI]" {file: "file:///c:/mozilla-source/comm-central/obj-i686-pc-mingw32/dist/bin/components/nsSMTPProtocolHandler.js" line: 25}]"

Comment 54

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8944304 [details] [diff] [review]
1431913-smtp.patch - WIP (v1) - NOT WORKING

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

::: mailnews/compose/src/nsSmtpUrl.h
@@ +131,5 @@
> +    }
> +
> +    NS_IMETHOD SetSpec(const nsACString & aSpec, nsIURIMutator** aMutator) override
> +    {
> +      NS_ADDREF(*aMutator = this);

if (aMutator) NS_ADDREF(*aMutator = this);

@@ +146,2 @@
>    // nsSmtpUrl
>    nsSmtpUrl();

nsSmtpUrl::Mutate needs to be return a new nsSmtpUrl::Mutator, similar to 
https://searchfox.org/mozilla-central/rev/e9a067a401e41017766f4ab90bc3906603273d51/netwerk/base/nsSimpleURI.cpp#889,893

Comment 55

[Jorg K (CEST = GMT+2)]

Valentin, thanks for the offer of the backout, but I've already landed C++ changes, mostly using the SetSpecInternal() stuff and some miscellaneous JS tweaks.

We're down to three test failures, the "foo" stuff, IMAP dod, and all of SMTP. If we can solve SMTP now, we can follow up on the others later. As I said, "foo" is very complicated and only used for JS account which is broken on truck due to other URL changes anyway.

Comment 56

[Jorg K (CEST = GMT+2)]

Attachment: 1431913-js-follow-up.patch

LDAP follow-up.

Comment 57

[Jorg K (CEST = GMT+2)]

Attachment: 1431913-js-follow-up.patch (v1b) [landed comment #64]

Oops, I forgot to take out the 'instanceof' :-(

Comment 58

[Jorg K (CEST = GMT+2)]

Attachment: 1431913-smtp.patch - WIP (v2) - NOT WORKING

With Valentin's help via IRC I put this together, but the failure is still the same:
 0:01.52 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIURI.mutate]" {file: "file:///c:/mozilla-source/comm-central/obj-i686-pc-mingw32/dist/bin/components/nsSMTPProtocolHandler.js" line: 27}]"

Comment 59

[Jorg K (CEST = GMT+2)]

Comment on attachment 8944328 [details] [diff] [review]
1431913-smtp.patch - WIP (v2) - NOT WORKING

OK, the feedback on IRC was that the error might occur if nsMsgMailNewsUrl::CloneInternal() fails. And that apparently doesn't do the right thing. More to follow.

Comment 60

[Jorg K (CEST = GMT+2)]

Attachment: 1431913-smtp.patch - WIP (v3) - NOT WORKING - parts will be moved to another bug.

This is what I've got at 2 AM :-( - It crashes running test_smtpURL.js.

Comment 61

[:aceman]

Comment on attachment 8944324 [details] [diff] [review]
1431913-js-follow-up.patch (v1b) [landed comment #64]

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

::: ldap/xpcom/tests/unit/test_nsLDAPURL.js
@@ +261,5 @@
>    // Set attributes via the url spec
>  
>    newAttrs = "abc,def,ghi,jkl";
> +  url = url.mutate().setSpec("ldap://localhost/dc=short?" + newAttrs + "?one?(objectclass=*)")
> +                    .finalize().QueryInterface(Ci.nsILDAPURL);

This looks better, I just wonder why we need the Qi. If url already was nsILDAPURL, what did change it? HOpefully the mutate() would preserve the type.

Comment 62

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #61)
> This looks better, I just wonder why we need the Qi.
See comment #41.

Comment 63

[Jorg K (CEST = GMT+2)]

Attachment: 1431913-protocol-handler.patch (v1) [landed comment #64]

Since the mutator isn't set up properly for SMTP URLs and there seem to be problem with the inner workings of nsMsgMailNewsUrl, here's a quick fix using an init function. We'll have to revisit the entire ULR management later anyway.

Comment 64

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/866b5c60d753
Port bug 1431204 to mailnews: Make nsIURI.spec readonly (JS part, follow-up). r=aceman
https://hg.mozilla.org/comm-central/rev/1dd846f3e167
fix SMTP protocol handler. rs=bustage-fix

Comment 65

[Jorg K (CEST = GMT+2)]

As expected, we're down to two test failures now:
TEST-UNEXPECTED-FAIL | comm/mailnews/imap/test/unit/test_dod.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/jsaccount/test/unit/test_fooUrl.js | xpcshell return code: 0
I'll take a look at test_dod.js now and I'll move "foo" to a follow-up bug while temporarily disabling it.

Comment 66

[Jorg K (CEST = GMT+2)]

Attachment: 1431913-calendar+chat.patch [chat part landed in comment #89]

Here is the Calendar and Chat part extracted from Philipp's v2 fix.

I'm not a Calendar or Chat peer. I cannot even test the Calendar part.

For the Chat part we have the following comments:
Comment #34:
You might still want to make the URI immutable.
Comment #38 and comment #40:
> function setUriSpec(uri, spec) {
>   let newUri = uri.mutate().setSpec(spec).finalize();
>   newUri.QueryInterface(Ci.nsIMutable).mutable = false;
>   return newUri;
> }
This code is correct.

So you might want to add that.

Comment 67

[Jorg K (CEST = GMT+2)]

Comment on attachment 8944365 [details] [diff] [review]
1431913-calendar+chat.patch [chat part landed in comment #89]

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

::: calendar/providers/caldav/calDavCalendar.js
@@ +456,5 @@
>      },
>  
>      setCalHomeSet: function(removeLastPathSegment) {
>          if (removeLastPathSegment) {
>              let calUri = this.mUri.clone();

No need to clone here, you dropped the .clone() in the hunk just above.

::: calendar/providers/wcap/calWcapSession.js
@@ +80,5 @@
>                  if (!regCal.isDefaultCalendar) {
>                      let [spec, params] = splitUriParams(regCal.uri);
>                      if (spec != defaultSpec) {
>                          log("fixing url of subscribed calendar: " + regCal.calId, session);
> +                        let caluri = regCal.uri.mutate().setSpec(defaultSpec + params).finalize();

This looks right but needs testing. If the mutator isn't working as expected, this will fail. What type of URI/URL are we dealing with here? http(s)?

::: chat/components/src/smileProtocolHandler.js
@@ +20,5 @@
>                   Ci.nsIProtocolHandler.URI_IS_UI_RESOURCE |
>                   Ci.nsIProtocolHandler.URI_IS_LOCAL_RESOURCE,
>    newURI: function SPH_newURI(aSpec, aOriginCharset, aBaseURI) {
>      let uri = Cc["@mozilla.org/network/simple-uri;1"].createInstance(Ci.nsIURI);
> +    return uri.mutate().setSpec(aSpec).finalize();

As per the previous comment, you might want to use:
  let newUri = uri.mutate().setSpec(spec).finalize();
  newUri.QueryInterface(Ci.nsIMutable).mutable = false;
  return newUri;
I also think that this needs testing.

Comment 68

[Jorg K (CEST = GMT+2)]

Comment on attachment 8944349 [details] [diff] [review]
1431913-protocol-handler.patch (v1) [landed comment #64]

Let's r+ this for the moment and then revisit the hole area in a follow-up bug.

Comment 69

[Jorg K (CEST = GMT+2)]

Attachment: 1431913-test-dod.patch

That was easy. I'll land this now since the branch will happen any minute.

Comment 70

[Jorg K (CEST = GMT+2)]

Attachment: 1431913-test-foourl.patch

I've tried
mozilla/mach xpcshell-test mailnews/jsaccount/test/unit/test_fooUrl.js
with the hunk from Philipp's fix v2. As expected, it didn't work. It runs into the same problems that the SMTP stuff did:
 0:02.32 ERROR NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIURI.mutate]
test_nsIURI@c:/mozilla-source/comm-central/obj-i686-pc-mingw32/_tests/xpcshell/mailnews/jsaccount/test/unit/test_fooUrl.js:52:11
run_test@c:/mozilla-source/comm-central/obj-i686-pc-mingw32/_tests/xpcshell/mailnews/jsaccount/test/unit/test_fooUrl.js:164:5

So I'll disable this for now and we'll look at it in a follow-up bug. BTW, JS Account is broken anyway due to bug 1418011.

Comment 71

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d10e311b217a
fix test_dod.js. rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/ec015f717cb0
temporarily disable test_fooUrl.js. rs=bustage-fix

Comment 72

[Jorg K (CEST = GMT+2)]

FRG, this bug is becoming very confusing, so I'll just get review for attachment 8944365 [details] [diff] [review] and then move the rest to a follow-up bug.

Please take a look at attachment 8944314 [details] [diff] [review], there are two SM hunks (last two in the review: customize.js, FeedConverter.js) which my or may not work depending on the class of URL/URI and whether the mutator works, which it doesn't for some Mailnes URLs currently. So please take note and move this issue to a SM bug.

Comment 73

[:aceman]

Comment on attachment 8944349 [details] [diff] [review]
1431913-protocol-handler.patch (v1) [landed comment #64]

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

::: mailnews/compose/src/nsSMTPProtocolHandler.js
@@ +23,5 @@
>      newURI: function (aSpec, aOriginCharset, aBaseURI) {
>        var url = Components.classes["@mozilla.org/messengercompose/smtpurl;1"]
>                            .createInstance(Components.interfaces.nsIURI);
> +      if (url instanceof Components.interfaces.nsISmtpUrl)
> +        url.init(aSpec);

I thought that by creating init() this can be merged into a single line.
If you want you can land this, but this really needs revisiting as we seem to create/initialize every URL type in a different way.

Comment 74

[:aceman]

Comment on attachment 8944373 [details] [diff] [review]
1431913-test-dod.patch

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

Interesting. Hopefully there wasn't a reason it was doen the old way, e.g. GetUrlForUri() returning different result if the '?' part is present.

Comment 75

[:aceman]

Comment on attachment 8944214 [details] [diff] [review]
1431913-js-part.patch (v2) [landed comment #35]

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

OK, but we should clean up the mentioned places in a followup.

Comment 76

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #73)
> > +      if (url instanceof Components.interfaces.nsISmtpUrl)
> > +        url.init(aSpec);
> I thought that by creating init() this can be merged into a single line.
> If you want you can land this, but this really needs revisiting as we seem
> to create/initialize every URL type in a different way.
This construct is copied from LDAP. I guess
  url.QueryInterface(Components.interfaces.nsISmtpUrl).init(aSpec)
would work, too.

(In reply to :aceman from comment #74)
> Interesting. Hopefully there wasn't a reason it was doen the old way, e.g.
> GetUrlForUri() returning different result if the '?' part is present.
You are right, looking at GetUrlForUri()
https://dxr.mozilla.org/comm-central/rev/603955ee98d37a04c231db0c5025f20b7d934a1b/mailnews/imap/src/nsImapService.cpp#275
the last thing that is appended is the message key, the query is lost.

Looking for header=quotebody
https://dxr.mozilla.org/comm-central/search?q=header%3Dquotebody&redirect=false
we see that this is only read once in nsImapUrl.cpp.

I ran the test without the "?header=quotebody" and it passed just as well. So how do you want to handle that? I think we can just remove it in a follow-up.

Other than that, I'll file two follow-up bugs now, one for SetSpecInternal() and one for the "foo" test which I think I will leave to Kent if he ever wants to fix JS Account.

Comment 77

[:aceman]

Comment on attachment 8944242 [details] [diff] [review]
1431913-url-readonly.patch (v5) [landed comment #35 without the  nsSMTPProtocolHandler.js hunk]

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

OK, as a quick fix to get a working beta after today's branching.
But this needs revisiting too, it seems Fallen's attachment 8944314 [details] [diff] [review] is more in line with what m-c did. We probably shouldn't just call SetSpecInternal from any random code.
It may be his patch doesn't work right as the needed mutators and clones for our subclasses of URI aren't implemented or working right. It seems Valentin gave some clues on IRC on what to do.

Comment 78

[Florian Quèze [:florian]]

Comment on attachment 8944365 [details] [diff] [review]
1431913-calendar+chat.patch [chat part landed in comment #89]

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

The chat/ change looks reasonable (note: I haven't tested it).

I don't have a strong opinion about whether we should add ".QueryInterface(Ci.nsIMutable).mutable = false". This uri has been mutable for years without it being a problem, so I don't think it being mutable some more until .finalize actually makes URIs immutable is a problem worth worrying about. r=me either way.

Comment 79

[:aceman]

var url = Components.classes["@mozilla.org/messengercompose/smtpurl;1"]
                    .createInstance(Components.interfaces.nsISmtpUrl).init(aSpec) ?
Anyway, I do not see an init() method is used on the other classes much so this addition may not be the best. We should clean this all up according to a schema Valentin will teach us :)

Comment 80

[Valentin Gosu [:valentin] (he/him)]

(In reply to :aceman from comment #79)
> Anyway, I do not see an init() method is used on the other classes much so
> this addition may not be the best. We should clean this all up according to
> a schema Valentin will teach us :)

There is an init method on nsIStandardURL - I filed bug 1432187 to move the init method to the mutator.
I'll upload the patches tonight, and they will be a good example for how this should look.

Comment 81

[Jorg K (CEST = GMT+2)]

Hmm, is there a misunderstanding here? Copied from LDAP, I said it multiple times: comment #47 comment #76:
https://dxr.mozilla.org/comm-central/rev/603955ee98d37a04c231db0c5025f20b7d934a1b/ldap/xpcom/src/nsLDAPProtocolHandler.js#25

I filed bug 1432191 so far but I'm too busy answering comments, so I haven't filed the more important bug now.

That said, the "new scheme" uses mutate() which is internally creating a clone. I don't see why we should do it this way, I'd much rather create the thing correctly first off.

Apparently (comment #17) NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID) can also be used to create a new thing and make it mutable for a moment, so I'd rather have something like
url = mutate("@mozilla.org/messengercompose/smtpurl;1").setSpec(aSpec).finalize();
We'll see.

Comment 82

[Jorg K (CEST = GMT+2)]

OK, apart form the outstanding calendar review, let's move the action to bug 1432191 and bug 1432204.

Comment 83

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Jorg K (GMT+1) from comment #81)
> Apparently (comment #17) NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID) can
> also be used to create a new thing and make it mutable for a moment, so I'd
> rather have something like
> url =
> mutate("@mozilla.org/messengercompose/smtpurl;1").setSpec(aSpec).finalize();
> We'll see.
My last patch allows for this, albeit not for the smtp url. Feel free to take that into account in the followup. I tried to add mutators to the subclasses of nsMsgMailNewsUrl but got stuck on some vtable C++ errors.

Comment 84

[Jorg K (CEST = GMT+2)]

Attachment: 1431913-test-dod.patch [landed comment #89]

I would remove this since it's not useful in the test.

Comment 85

[Frank-Rainer Grahl (:frg)]

Thanks Jorg. I will take care of the suite part.

Comment 86

[Jorg K (CEST = GMT+2)]

Comment on attachment 8944365 [details] [diff] [review]
1431913-calendar+chat.patch [chat part landed in comment #89]

Richard, could you check in a local build or Daily of today or later whether there is any problems with smileys in chat. If so, does the patch fix it.

Or perhaps Florian can tell us what the smileProtocolHandler.js is about and how to test this. Anyway, this uses "simple URI", to the mutator and other stuff should "just work".

Florian, thanks for the quick review, BTW.

Comment 87

[Florian Quèze [:florian]]

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

> Or perhaps Florian can tell us what the smileProtocolHandler.js is about and
> how to test this. Anyway, this uses "simple URI", to the mutator and other
> stuff should "just work".

When an emoticon eg. ;) is detected in a chat message, it's replaced in the HTML of the message with:
 <img src="smile://;)" title=";)" alt=";)" class="ib-img-smile">

Then, the smileProtocolHandler is used to convert the smile://;) url to the url of an actual image file. So yes, checking that smiley images are correctly displayed in chat messages is all it takes to verify that the patch works.

Comment 88

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+1) from comment #86)
> Comment on attachment 8944365 [details] [diff] [review]
> 1431913-calendar+chat.patch
> 
> Richard, could you check in a local build or Daily of today or later whether
> there is any problems with smileys in chat. If so, does the patch fix it.

Yes, this works again. Without patch no icon and with patch the icon is bacl again.

Comment 89

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/459b4030d2c9ee173f386e9b1bc2b1b68a8efbb8
Follow-up: remove unneeded query in test_dod.js. r=aceman

https://hg.mozilla.org/comm-central/rev/fd49c5417f7517387d0e7cc39691f4c71c8d6687
Adapt to nsIURI.spec being made readonly in chat. r=florian

Comment 90

[Jorg K (CEST = GMT+2)]

Comment on attachment 8944365 [details] [diff] [review]
1431913-calendar+chat.patch [chat part landed in comment #89]

Missed the uplift.

Comment 91

[Jorg K (CEST = GMT+2)]

Comment on attachment 8944494 [details] [diff] [review]
1431913-test-dod.patch [landed comment #89]

Missed the uplift.

Comment 92

[Martin Schröder [:mschroeder]]

(In reply to Jorg K (GMT+1) from comment #66)
> Created attachment 8944365 [details] [diff] [review]
> 1431913-calendar+chat.patch [chat part landed in comment #89]

There is a typo in the calendar part:

+            this.mCalHomeSet = Serviews.io.newURI(split2.join("/") + "/");

This should be "Services" not "Serviews"

Comment 93

[alta88]

Comment on attachment 8944214 [details] [diff] [review]
1431913-js-part.patch (v2) [landed comment #35]

Tested dnd in subscribe dialog on today's nightly and it seems to work fine.

Comment 94

[[:MakeMyDay]]

Comment on attachment 8944365 [details] [diff] [review]
1431913-calendar+chat.patch [chat part landed in comment #89]

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

Sorry for the delay, r=me for calendar with the typo fixed.

::: calendar/providers/caldav/calDavCalendar.js
@@ +464,5 @@
>                  baseUrl = baseUrl.substring(0, baseUrl.length - 2);
>              }
>              let split2 = baseUrl.split("/");
>              split2.pop();
> +            this.mCalHomeSet = Serviews.io.newURI(split2.join("/") + "/");

Typo, Serviews instead of Services

Comment 95

[Jorg K (CEST = GMT+2)]

What about the clone(), see comment #67?

I assume you tested it, right?

Comment 96

[Jorg K (CEST = GMT+2)]

Attachment: 1431913-calendar.patch [landed comment #98]

I took the liberty to remove the unneeded calUri variables and thus the unneeded clones.

Comment 97

[Jorg K (CEST = GMT+2)]

Setting the target to TB 59 here since most was landed on TB 59 and the patches that missed the branch date will be uplifted.

Comment 98

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0eaecba4fc41
Adapt to nsIURI.spec being made readonly in Calendar. r=MakeMyDay

Comment 99

[Jorg K (CEST = GMT+2)]

Uplift of the patches that missed the branch date to TB 59 (beta):
https://hg.mozilla.org/releases/comm-beta/rev/e9c732c2dc955dddcfe79a01ae2752c7e96408c1 (test dod)
https://hg.mozilla.org/releases/comm-beta/rev/531963a824fd45b6f71574a6e1a179e94a552465 (chat)
https://hg.mozilla.org/releases/comm-beta/rev/beeb0c4d43376f7ba23f3ab6b943be7fa94cba50 (calendar)