Closed Bug 1431913 Opened 2 years ago Closed 2 years ago

Port bug 1431204 to mailnews: Make nsIURI.spec readonly

Categories

(Thunderbird :: General, defect)

defect
Not set

Tracking

(thunderbird59 fixed)

RESOLVED FIXED
Thunderbird 59.0
Tracking Status
thunderbird59 --- fixed

People

(Reporter: Paenglab, Assigned: jorgk)

References

Details

Attachments

(11 files, 16 obsolete files)

13.59 KB, patch
alta88
: review+
aceman
: review+
Details | Diff | Splinter Review
48.65 KB, patch
aceman
: review+
Details | Diff | Splinter Review
97.17 KB, patch
Details | Diff | Splinter Review
1.82 KB, patch
aceman
: review+
Details | Diff | Splinter Review
5.37 KB, patch
Details | Diff | Splinter Review
2.53 KB, patch
aceman
: review+
Details | Diff | Splinter Review
4.38 KB, patch
MakeMyDay
: review+
florian
: review+
Details | Diff | Splinter Review
1.34 KB, patch
aceman
: review+
Details | Diff | Splinter Review
614 bytes, patch
Details | Diff | Splinter Review
1.32 KB, patch
aceman
: review+
Details | Diff | Splinter Review
3.29 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
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.
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++.
Attached patch Bug1431913.patch (obsolete) β€” β€” Splinter Review
Oops, wrong patch
Attachment #8944152 - Attachment is obsolete: true
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
Valentin, do we also need to change JS code? I haven't looked but we may use url.spec = ... in JS.
Flags: needinfo?(valentin.gosu)
JS code needs to replace
uri.spec = "something"
with
uri = uri.mutate().setSpec("something").finalize()
Flags: needinfo?(valentin.gosu)
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++.
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.
Assignee: nobody → jorgk
Attachment #8944153 - Attachment is obsolete: true
Status: NEW → ASSIGNED
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.
Using SetSpecInternal() now, to be continued.
Attachment #8944174 - Attachment is obsolete: true
Attached patch 1431913-url-readonly.patch (v3) (obsolete) β€” β€” Splinter Review
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.
Attachment #8944182 - Attachment is obsolete: true
Make that: I wonder why his didn't compile.
Attached patch 1431913-url-readonly.patch (v4) (obsolete) β€” β€” Splinter Review
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 ;-)
Attachment #8944186 - Attachment is obsolete: true
Attachment #8944193 - Flags: review?(acelists)
Attached patch 1431913-url-readonly.patch (v4b) (obsolete) β€” β€” Splinter Review
Oops, leftover from a previous experiment.
Attachment #8944193 - Attachment is obsolete: true
Attachment #8944193 - Flags: review?(acelists)
Attachment #8944194 - Flags: review?(acelists)
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);
Attachment #8944194 - Flags: feedback?(valentin.gosu)
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 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.
Attachment #8944194 - Flags: feedback?(valentin.gosu) → feedback+
(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.
Attached patch 1431913-js-part.patch (obsolete) β€” β€” Splinter Review
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.
Attached patch Philipp's fix - v1 (obsolete) β€” β€” Splinter Review
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);
Attachment #8944210 - Flags: feedback?(valentin.gosu)
Attached patch 1431913-js-part.patch (obsolete) β€” β€” Splinter Review
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.
I think this is right. This doesn't cover test_fooUrl.js since that is very complicated stuff that needs a close look.
Attachment #8944213 - Attachment is obsolete: true
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.
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.
Attachment #8944205 - Attachment is obsolete: true
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().
Attached patch 1431913-url-readonly.patch (v4c) (obsolete) β€” β€” Splinter Review
Didn't compile one Mac:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f2c5a52c4a72a8e6fb2fc12f51021ab31be55928
Attachment #8944194 - Attachment is obsolete: true
Attachment #8944194 - Flags: review?(acelists)
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.
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
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.
Attachment #8944229 - Attachment is obsolete: true
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.
Attachment #8944214 - Flags: review?(alta88)
> 
> 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
Attachment #8944210 - Attachment is obsolete: true
Attachment #8944210 - Flags: feedback?(valentin.gosu)
Attachment #8944228 - Flags: feedback?(valentin.gosu)
(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.
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 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?
Attachment #8944228 - Flags: feedback?(valentin.gosu)
Pushed by mozilla@jorgk.com:
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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
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.
Attached patch Philipp's fix - v1b - rebased (obsolete) β€” β€” Splinter Review
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.
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Attachment #8944214 - Attachment description: 1431913-js-part.patch (v2) → 1431913-js-part.patch (v2) [landed comment #35]
Attachment #8944242 - Attachment description: 1431913-url-readonly.patch (v5) → 1431913-url-readonly.patch (v5) [landed comment #35]
Attachment #8944242 - Flags: review?(acelists)
Attachment #8944214 - Flags: review?(acelists)
Attachment #8944242 - Attachment description: 1431913-url-readonly.patch (v5) [landed comment #35] → 1431913-url-readonly.patch (v5) [landed comment #35 without the nsSMTPProtocolHandler.js hunk]
Attachment #8944242 - Attachment is obsolete: true
Attachment #8944242 - Flags: review?(acelists)
Attachment #8944242 - Attachment is obsolete: false
Attachment #8944242 - Flags: review?(acelists)
(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?
Attachment #8944228 - Attachment is obsolete: true
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.
Attachment #8944304 - Flags: feedback?(valentin.gosu)
Attachment #8944304 - Flags: feedback?(philipp)
(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.
(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 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.
(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 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.
(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 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); ?
(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.
(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 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.
Attachment #8944304 - Flags: feedback?(philipp) → feedback+
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.
Attachment #8944303 - Attachment is obsolete: true
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.
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.
(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 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
Attachment #8944304 - Flags: feedback?(valentin.gosu) → feedback+
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.
Attached patch 1431913-js-follow-up.patch (obsolete) β€” β€” Splinter Review
LDAP follow-up.
Oops, I forgot to take out the 'instanceof' :-(
Attachment #8944320 - Attachment is obsolete: true
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}]"
Attachment #8944304 - Attachment is obsolete: true
Attachment #8944328 - Flags: feedback?(valentin.gosu)
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.
Attachment #8944328 - Flags: feedback?(valentin.gosu)
This is what I've got at 2 AM :-( - It crashes running test_smtpURL.js.
Attachment #8944328 - Attachment is obsolete: true
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.
Attachment #8944324 - Flags: review+
(In reply to :aceman from comment #61)
> This looks better, I just wonder why we need the Qi.
See comment #41.
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.
Pushed by mozilla@jorgk.com:
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
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.
Attachment #8944324 - Attachment description: 1431913-js-follow-up.patch (v1b) → 1431913-js-follow-up.patch (v1b) [landed comment #64]
Attachment #8944349 - Attachment description: 1431913-protocol-handler.patch (v1) → 1431913-protocol-handler.patch (v1) [landed comment #64]
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.
Attachment #8944365 - Flags: review?(makemyday)
Attachment #8944365 - Flags: review?(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]:
-----------------------------------------------------------------

::: 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.
Attachment #8944314 - Attachment description: Philipp's fix - v2 → Philipp's fix - v2 - parts will be moved to another bug.
Attachment #8944332 - Attachment description: 1431913-smtp.patch - WIP (v3) - NOT WORKING → 1431913-smtp.patch - WIP (v3) - NOT WORKING - parts will be moved to another bug.
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.
Attachment #8944349 - Flags: review?(acelists)
Attached patch 1431913-test-dod.patch β€” β€” Splinter Review
That was easy. I'll land this now since the branch will happen any minute.
Attachment #8944373 - Flags: review?(acelists)
Attached patch 1431913-test-foourl.patch β€” β€” Splinter Review
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.
Pushed by mozilla@jorgk.com:
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
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.
Flags: needinfo?(frgrahl)
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.
Attachment #8944349 - Flags: review?(acelists) → review+
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.
Attachment #8944373 - Flags: review?(acelists) → review+
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.
Attachment #8944214 - Flags: review?(acelists) → review+
(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 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.
Attachment #8944242 - Flags: review?(acelists) → review+
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.
Attachment #8944365 - Flags: review?(florian) → review+
Blocks: 1432191
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 :)
(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.
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.
Blocks: 1432204
OK, apart form the outstanding calendar review, let's move the action to bug 1432191 and bug 1432204.
(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.
I would remove this since it's not useful in the test.
Attachment #8944494 - Flags: review?(acelists)
Thanks Jorg. I will take care of the suite part.
Flags: needinfo?(frgrahl)
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.
Flags: needinfo?(richard.marti)
Flags: needinfo?(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.
Flags: needinfo?(florian)
Attachment #8944494 - Flags: review?(acelists) → review+
(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.
Flags: needinfo?(richard.marti)
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
Attachment #8944494 - Attachment description: 1431913-test-dod.patch → 1431913-test-dod.patch [landed comment #89]
Attachment #8944365 - Attachment description: 1431913-calendar+chat.patch → 1431913-calendar+chat.patch [chat part landed in comment #89]
Comment on attachment 8944365 [details] [diff] [review]
1431913-calendar+chat.patch [chat part landed in comment #89]

Missed the uplift.
Attachment #8944365 - Flags: approval-comm-beta+
Comment on attachment 8944494 [details] [diff] [review]
1431913-test-dod.patch [landed comment #89]

Missed the uplift.
Attachment #8944494 - Flags: approval-comm-beta+
(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 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.
Attachment #8944214 - Flags: review?(alta88) → review+
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
Attachment #8944365 - Flags: review?(makemyday) → review+
What about the clone(), see comment #67?

I assume you tested it, right?
Flags: needinfo?(makemyday)
I took the liberty to remove the unneeded calUri variables and thus the unneeded clones.
Flags: needinfo?(makemyday)
Attachment #8945225 - Flags: review+
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.
Keywords: leave-open
Target Milestone: --- → Thunderbird 59.0
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0eaecba4fc41
Adapt to nsIURI.spec being made readonly in Calendar. r=MakeMyDay
Status: ASSIGNED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Attachment #8945225 - Attachment description: 1431913-calendar.patch → 1431913-calendar.patch [landed comment #98]
Duplicate of this bug: 1433801
You need to log in before you can comment on or make changes to this bug.