Closed Bug 1440693 Opened 3 years ago Closed 3 years ago

Port bug 1433958 and bug 1434163 to C-C: nsIURI attributes will become read-only

Categories

(Thunderbird :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 60.0

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

Details

Attachments

(15 files, 19 obsolete files)

115.91 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
1.18 KB, patch
Details | Diff | Splinter Review
1.76 KB, patch
aceman
: review+
Details | Diff | Splinter Review
786 bytes, patch
aceman
: review+
Details | Diff | Splinter Review
757 bytes, patch
aceman
: review+
Details | Diff | Splinter Review
980 bytes, patch
aceman
: review+
Details | Diff | Splinter Review
2.35 KB, patch
aceman
: review+
Details | Diff | Splinter Review
5.69 KB, patch
aceman
: review+
Details | Diff | Splinter Review
7.14 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
1.16 KB, patch
aceman
: review+
Details | Diff | Splinter Review
5.26 KB, patch
Details | Diff | Splinter Review
1.09 KB, patch
Details | Diff | Splinter Review
5.12 KB, patch
aceman
: review+
Details | Diff | Splinter Review
5.79 KB, patch
aceman
: review+
Details | Diff | Splinter Review
1.54 KB, patch
aceman
: review+
Details | Diff | Splinter Review
In bug 1434163 M-C will make nsIURI attributes read-only. Changes are C++ only, methods like SetSpecInternal() or all the other set-methods:
  SetScheme(const nsACString &input);
  SetUserPass(const nsACString &input);
  SetUsername(const nsACString &input);
  SetPassword(const nsACString &input);
  SetHostPort(const nsACString &aValue);
  SetHost(const nsACString &input);
  SetPort(int32_t port);
  SetPathQueryRef(const nsACString &input);
  SetRef(const nsACString &input);
  SetFilePath(const nsACString &input);
  SetQuery(const nsACString &input);
will become private.

To prepare for that, M-C changes all C++ and JS call sites to the new mutators in bug 1433958.

I haven't looked at TB in detail, but I believe the C++ part won't be so bad, some things will simple be switched to private functions and we notice the rest due to compile errors.

The hard part will be to find and visit all the JS call sites, of which we have a whole lot, also in calendar.

Just looking at |.port =|
https://searchfox.org/comm-central/search?q=.port+%3D&case=false&regexp=false&path=
it's hard to tell whether the port is set on an nsIURI object or some other object.

https://searchfox.org/comm-central/search?q=.pathQueryRef+%3D&case=false&regexp=false&path=
is a little clearer, but we should forget += assignments:
https://searchfox.org/comm-central/search?q=.pathQueryRef+%2B%3D&case=false&regexp=false&path=
Bug 1433958 has landed, so I'll see what breaks in TB with the latest patch from bug 1434163:
hg import https://reviewboard-hg.mozilla.org/gecko/rev/78b27ffb25e2f8c1515e393caebd82cc0f22c0c2
This should address the address book URL. More to come (of course).
Address book and LDAP.
Address book, LDAP, mailto URL and a bit of the nsMsgMailNewsUrl. More to come.
Attachment #8954353 - Attachment is obsolete: true
Attachment #8954362 - Attachment is obsolete: true
Comment on attachment 8954380 [details] [diff] [review]
1440693-nsIURI-readonly.patch - WIP (v3)

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

::: ldap/xpcom/src/nsLDAPURL.cpp
@@ +153,5 @@
>    nsCString originalSpec;
>    nsresult rv = mBaseURL->GetSpec(originalSpec);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  return NS_MutateURI(mBaseURL).SetSpec(aSpec).Finalize(mBaseURL);

How should this work? What about the code after the return? Did you mean something else instead of return?
Some more, still heaps of compile errors. I've fixed the cut/paste error spotted by Aceman in comment #5.
Attachment #8954380 - Attachment is obsolete: true
More stuff, still not compiling. I've created a nsIMsgMailNewsUrl::SetSpecInternal().
Attachment #8954427 - Attachment is obsolete: true
Comment on attachment 8954530 [details] [diff] [review]
1440693-nsIURI-readonly.patch - WIP (v5)

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

::: mailnews/base/util/nsMsgUtils.cpp
@@ +184,5 @@
>    {
>      nsCOMPtr<nsIImapUrl> imapUrl = do_CreateInstance(kImapUrlCID, &rv);
>  
>      if (NS_SUCCEEDED(rv) && imapUrl)
> +      rv = imapUrl->QueryInterface(NS_GET_IID(nsIMsgMailNewsUrl), getter_AddRefs(newUri));

Would newUri = do_QueryInterface(imapUrl, &rv) work here to automatically get the nsIMsgMailNewsUrl interface? Some cases don't work like this so I ask.

@@ +205,2 @@
>    if (*aUrl) // SetSpec can fail, for mailbox urls, but we still have a url.
> +    (void)newUri->SetSpecInternal(nsDependentCString(uri));

I think we use space after (void).

::: mailnews/compose/src/nsSmtpService.cpp
@@ +173,5 @@
>      urlSpec.AppendInt(smtpPort);
>    }
>  
> +  nsCOMPtr<nsIMsgMailNewsUrl> url;
> +  rv = NS_MutateURI(new nsMsgMailNewsUrl::Mutator()).SetSpec(urlSpec).Finalize(url);

smtpUrl is no longer needed in the code? Was it only used to get the right interface which you now specify explicitly?
Bug 1434163 is now on autoland with: https://hg.mozilla.org/integration/autoland/rev/74fc20523198.

(In reply to :aceman from comment #8)
> Would newUri = do_QueryInterface(imapUrl, &rv) work here to automatically
> get the nsIMsgMailNewsUrl interface? Some cases don't work like this so I
> ask.
Not my priority to try now.

> I think we use space after (void).
No. I have removed any space after a cast I've come across.

> smtpUrl is no longer needed in the code?
You mean the that follows:
  smtpUrl->SetSender(aSender);
  smtpUrl->SetRecipients(aRecipients);
  smtpUrl->SetRequestDSN(aRequestDSN);
  smtpUrl->SetPostMessageFile(aFilePath);
  smtpUrl->SetSenderIdentity(aSenderIdentity);
  if (aNotificationCallbacks)
    smtpUrl->SetNotificationCallbacks(aNotificationCallbacks);
  smtpUrl->SetSmtpServer(aSmtpServer);
None of these methods are in nsMsgMailNewsUrl, so surely the 'smtpUrl' is needed. Otherwise I don't understand the question.
(In reply to Jorg K (GMT+1) from comment #9)
> None of these methods are in nsMsgMailNewsUrl, so surely the 'smtpUrl' is
> needed. Otherwise I don't understand the question.

I mean that in the expression you touch smtpUrl is no longer used. If that is correct and no oversight then I have no problem.
(In reply to :aceman from comment #10)
> I mean that in the expression you touch smtpUrl is no longer used. If that
> is correct and no oversight then I have no problem.
That hunk was completely wrong, gone now.

With this patch there are a few compile errors in nsNewsFolder.cpp, nsNntpService.cpp and nsNNTPProtocol.cpp.

So the next patch will compile compeltely :-)
Assignee: nobody → jorgk
Attachment #8954530 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Enjoy ;-)

Of course this is missing the JS part, so where we go
  xx.pathQueryRef = "yy";
we will now get an error. So I expect a million test failures :-((
Attachment #8955812 - Attachment is obsolete: true
Attachment #8955820 - Flags: review?(acelists)
Looking through the patch, I found some more room for simplification (and removal of a few trailing spaces that had slipped in).
Attachment #8955820 - Attachment is obsolete: true
Attachment #8955820 - Flags: review?(acelists)
Attachment #8955823 - Flags: review?(acelists)
Added null checks in Mutator::SetSpec(). Valentin had pointed that out to me before on another occasion.
Attachment #8955823 - Attachment is obsolete: true
Attachment #8955823 - Flags: review?(acelists)
Now checking status returned from the mutator. That should get rid of compiler warnings.
Attachment #8955835 - Attachment is obsolete: true
Attachment #8955837 - Flags: review?(acelists)
Killing last warning.
Attachment #8955837 - Attachment is obsolete: true
Attachment #8955837 - Flags: review?(acelists)
Attachment #8955839 - Flags: review?(acelists)
Comment on attachment 8955839 [details] [diff] [review]
1440693-nsIURI-readonly.patch - (v11)

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

Thanks, this compiles without warnings now.
Let's see what the tests say.

::: mailnews/base/util/nsMsgMailNewsUrl.cpp
@@ +222,4 @@
>      nsCOMPtr<nsIMsgAccountManager> accountManager =
>        do_GetService(NS_MSGACCOUNTMANAGER_CONTRACTID, &rv);
> +    if (NS_FAILED(rv))
> +      return rv;

I would have liked NS_ENSURE_SUCCESS(rv, rv); here for consistency :)

::: mailnews/imap/src/nsImapService.cpp
@@ +2333,1 @@
>    } // if (NS_SUCCEEDED(rv) && imapUrl)

You have removed also this comment later on.

::: mailnews/local/src/nsPop3Service.cpp
@@ +383,5 @@
>      int32_t port;
>      server->GetPort(&port);
>      if (port == -1) port = nsIPop3URL::DEFAULT_POP3_PORT;
>  
> +    // We need to escape the username befoe calling SetUsername() because it may

before typo

@@ +406,3 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    rv = NS_MutateURI(newUri).SetUsername(escapedUsername).Finalize(newUri);

Why does it no longer need to be nsIMsgMailNewsUrl ?

::: mailnews/news/src/nsNNTPProtocol.cpp
@@ +837,5 @@
>    int32_t pos = path.FindChar('?');
>    if (pos != kNotFound) {
>      path.SetLength(pos);
> +    rv = NS_MutateURI(newUri).SetPathQueryRef(path).Finalize(newUri);
> +    NS_ENSURE_SUCCESS(rv, rv);

This function checks nothing else so maybe checking this may change behaviour.
Attachment #8955839 - Flags: review?(acelists) → review+
Comment on attachment 8955839 [details] [diff] [review]
1440693-nsIURI-readonly.patch - (v11)

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

Thanks, this compiles without warnings now.
Let's see what the tests say.

::: mailnews/base/util/nsMsgMailNewsUrl.cpp
@@ +222,4 @@
>      nsCOMPtr<nsIMsgAccountManager> accountManager =
>        do_GetService(NS_MSGACCOUNTMANAGER_CONTRACTID, &rv);
> +    if (NS_FAILED(rv))
> +      return rv;

I would have liked NS_ENSURE_SUCCESS(rv, rv); here for consistency :)

::: mailnews/imap/src/nsImapService.cpp
@@ +2333,1 @@
>    } // if (NS_SUCCEEDED(rv) && imapUrl)

You have removed also this comment later on.

::: mailnews/local/src/nsPop3Service.cpp
@@ +383,5 @@
>      int32_t port;
>      server->GetPort(&port);
>      if (port == -1) port = nsIPop3URL::DEFAULT_POP3_PORT;
>  
> +    // We need to escape the username befoe calling SetUsername() because it may

before typo

@@ +406,3 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    rv = NS_MutateURI(newUri).SetUsername(escapedUsername).Finalize(newUri);

Why does it no longer need to be nsIMsgMailNewsUrl ?

::: mailnews/news/src/nsNNTPProtocol.cpp
@@ +837,5 @@
>    int32_t pos = path.FindChar('?');
>    if (pos != kNotFound) {
>      path.SetLength(pos);
> +    rv = NS_MutateURI(newUri).SetPathQueryRef(path).Finalize(newUri);
> +    NS_ENSURE_SUCCESS(rv, rv);

This function checks nothing else so maybe checking this may change behaviour.
(In reply to :aceman from comment #18)
> Let's see what the tests say.
They will fail big time since the JS changes are missing.

> I would have liked NS_ENSURE_SUCCESS(rv, rv); here for consistency :)
Fixed.

> You have removed also this comment later on.
Fixed.

> before typo
Fixed.

> Why does it no longer need to be nsIMsgMailNewsUrl ?
I don't know why the original code QI'ed to nsIMsgMailNewsUrl since SetUsername() was available on the nsIURI interface. Maybe I'm missing something.

> This function checks nothing else so maybe checking this may change
> behaviour.
Hmm, I don't understand this comment. This is in nsNNTPProtocol::OpenCacheEntry() which check return values on all the other calls. Surely if setting the path on the URI doesn't work, you want to hear about it. However, looking at this, I noticed that the Clone() above isn't necessary any more, you since the mutator clones anyway. I'll send a new patch.
Addressed the comments and removed unnecessary Clone() calls. Try interdiff.
Attachment #8955839 - Attachment is obsolete: true
Attachment #8955861 - Flags: review+
Hmm, that gave a whole heap of compiler errors on Mac :-((
INFO -  /builds/worker/workspace/build/src/obj-thunderbird/dist/include/nsMsgMailNewsUrl.h:49:5: error: class member cannot be redeclared
INFO -      NS_DECL_NSIURI

Looks like all those declaration from NS_DECL_NSIURI:
https://searchfox.org/mozilla-central/source/__GENERATED__/dist/include/nsIURI.h#166

M-C also has the declaration as virtual:
https://hg.mozilla.org/mozilla-central/rev/74fc20523198#l15.12

So let's try that:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7175c22ccde24fbfa7ab3a452785aa30368b9db6
Keywords: leave-open
Mac almost compiled with v13, only one unused variable left which I've removed here.

Also, moved nsIMsgMailNewsUrl::SetQuery() back to the protected section after reading up on private vs. protected ;-(
Attachment #8955861 - Attachment is obsolete: true
Attachment #8955871 - Attachment is obsolete: true
Attachment #8955873 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b6af191d5eaa
Port 1434163 to C-C: nsIURI attributes are read-only now. r=aceman
This didn't fail in a local compile :-(

I'll push it once we looked at test failures.
Attachment #8955873 - Attachment description: 1440693-nsIURI-readonly.patch - (v14) → 1440693-nsIURI-readonly.patch - (v14) [landed comment #24]
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/43ead9ebb035
Follow-up: fix compile error on Windows. rs=bustage-fix DONTBUILD
Of course something like this needs to be fixed now:
https://hg.mozilla.org/comm-central/rev/253ed51068545c9f2647b88e684737f62902b323#l1.15
request.URI.query += "&type=message/rfc822";
Attachment #8955879 - Attachment description: 1440693-follow-up.patch → 1440693-follow-up.patch [landed comment #26]
We're going to be here for a while fixing tests :-( Here is the first one.
Attachment #8955890 - Flags: review?(acelists)
Attached patch 1440693-calendar.patch (obsolete) — Splinter Review
One review will do. There was only one calendar test failure in calendar/test/unit/test_webcal.js and this patch fixes that one. I don't know which change causes the test to pass now, but all changes are required anyway :-)
Attachment #8955897 - Flags: review?(philipp)
Attachment #8955897 - Flags: review?(makemyday)
Attached patch 1440693-calendar.patch (v1b) (obsolete) — Splinter Review
Oops, one more replacement, sorry about the noise.
Attachment #8955897 - Attachment is obsolete: true
Attachment #8955897 - Flags: review?(philipp)
Attachment #8955897 - Flags: review?(makemyday)
Attachment #8955898 - Flags: review?(philipp)
Attachment #8955898 - Flags: review?(makemyday)
Does uri.mutate() effectively clone the original uri and then apply further changes to the clone or does it make the uri mutable and apply any change on the original uri?
Flags: needinfo?(jorgk)
Attached patch 1440693-calendar.patch (1c) (obsolete) — Splinter Review
Typo :-(
Attachment #8955898 - Attachment is obsolete: true
Attachment #8955898 - Flags: review?(philipp)
Attachment #8955898 - Flags: review?(makemyday)
Attachment #8955920 - Flags: review?(philipp)
Attachment #8955920 - Flags: review?(makemyday)
Comment on attachment 8955920 [details] [diff] [review]
1440693-calendar.patch (1c)

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

Thanks, r=me with the comments considered.

::: calendar/providers/wcap/calWcapSession.js
@@ +565,5 @@
>                          let calId = calid[0];
>                          let calendar = cals[calId];
>                          if (calendar === null) {
>                              calendar = new calWcapCalendar(this);
> +                            let newPath = this.uri.pathQueryRef += "?calid=" + encodeURIComponent(calId);

Please remove the = here:

let newPath = this.uri.pathQueryRef + "?calid=" + encodeURIComponent(calId);

@@ +892,5 @@
>                                  if (calendar) {
>                                      calendar.m_calProps = node; // update calprops
>                                  } else {
>                                      calendar = new calWcapCalendar(this, node);
> +                                    let newPath = this.uri.pathQueryRef += "?calid=" + encodeURIComponent(calId);

same here.
Attachment #8955920 - Flags: review?(philipp)
Attachment #8955920 - Flags: review?(makemyday)
Attachment #8955920 - Flags: review+
Attachment #8955921 - Flags: review?(acelists)
Attachment #8955924 - Flags: review?(acelists)
Thanks for the review, four eyes see more than two ;-)
Sorry about the copy/paste errors.
Attachment #8955925 - Flags: review+
Attachment #8955920 - Attachment is obsolete: true
Comment on attachment 8955924 [details] [diff] [review]
1440693-query.patch [landed comment #42]

This fixes the Mozmill test test-image-display.js which, like many other tests uses open_message_from_file() in test-folder-display-helpers.js. So there is hope that other tests will start working as well.

I noticed that opening an message doesn't open from the attachment bucked when it was attached from a message attachment of another message, the thing we just fixed on bug 1343536.

So that will need another follow-up. I'm landing what I have now to see how far we got.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0bd5e22ff5bc
fix test test_nsLDAPURL.js. r=aceman
https://hg.mozilla.org/comm-central/rev/e3daa718533a
fix test test_bug366491.js. r=aceman
https://hg.mozilla.org/comm-central/rev/ba9af888ccb9
fix test test_postPluginFilters.js. r=aceman
https://hg.mozilla.org/comm-central/rev/b090e8a0634f
fix bayesian tests.js. r=aceman
https://hg.mozilla.org/comm-central/rev/4e295d352da8
fix setting of host and port in mailnews/. r=aceman
https://hg.mozilla.org/comm-central/rev/ca762e6c30ce
fix setting of query in mail/ and mailnews/. r=aceman
https://hg.mozilla.org/comm-central/rev/0dc12c51c93e
use mutators instead of attribute setters in calendar. r=MakeMyDay
Attachment #8955890 - Attachment description: 1440693-test_nsLDAPURL.js.patch → 1440693-test_nsLDAPURL.js.patch [landed comment #42]
Attachment #8955903 - Attachment description: 1440693-test_bug366491.js.patch → 1440693-test_bug366491.js.patch [landed comment #42]
Attachment #8955918 - Attachment description: 1440693-test_postPluginFilters.js.patch → 1440693-test_postPluginFilters.js.patch [landed comment #42]
Attachment #8955919 - Attachment description: 1440693-test_bayesian.patch → 1440693-test_bayesian.patch [landed comment #42]
Attachment #8955921 - Attachment description: 1440693-host-port.patch → 1440693-host-port.patch [landed comment #42]
Attachment #8955924 - Attachment description: 1440693-query.patch → 1440693-query.patch [landed comment #42]
Attachment #8955925 - Attachment description: 1440693-calendar.patch (v1d) → 1440693-calendar.patch (v1d) [landed comment #42]
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7efbdc19de35
fix setting of host (take 2, typo fix). r=me DONTBUILD
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e548615c0451
Follow-up: fix compile error on Windows (take 2). rs=bustage-fix DONTBUILD
We're now down to the following test failures:

Xpcshell:
TEST-UNEXPECTED-FAIL | comm/mailnews/news/test/unit/test_bug695309.js
TEST-UNEXPECTED-FAIL | comm/mailnews/base/test/unit/test_detachToFile.js
TEST-UNEXPECTED-FAIL | comm/mailnews/base/test/unit/test_mimemaltdetach.js
TEST-UNEXPECTED-FAIL | comm/mailnews/imap/test/unit/test_imapAttachmentSaves.js
TEST-UNEXPECTED-FAIL | comm/mailnews/news/test/unit/test_bug403242.js
TEST-UNEXPECTED-FAIL | comm/mailnews/news/test/unit/test_getNewsMessage.js
TEST-UNEXPECTED-FAIL | comm/mailnews/news/test/unit/test_internalUris.js
TEST-UNEXPECTED-FAIL | comm/mailnews/news/test/unit/test_nntpPasswordFailure.js
TEST-UNEXPECTED-TIMEOUT | comm/mailnews/news/test/unit/test_bug540288.js
TEST-UNEXPECTED-TIMEOUT | comm/mailnews/news/test/unit/test_filter.js
TEST-UNEXPECTED-TIMEOUT | comm/mailnews/news/test/unit/test_nntpProxy.js
A lot of news failures, so maybe some C++ changes weren't quite right :-(
I see two groups here: News and attachments.

Mozmill:
attachment/test-attachment.js
composition/test-forwarded-eml-actions.js
composition/test-image-display.js
  (strange, that passed locally with M-C at f4e33c42faa7 and was OK at 8cced2a46f73238da13e41bcae8f6f8014)
composition/test-multipart-related.js
composition/test-reply-addresses.js
composition/test-reply-format-flowed.js
composition/test-reply-multipart-charset.js
composition/test-reply-signature.js
composition/test-save-changes-on-quit.js
then some crash and finally:
message-header/test-phishing-bar.js

Not disastrous but certainly not ready for prime time :-(
mozmake SOLO_TEST=composition/test-image-display.js mozmill-one
passes locally on Windows for me, in the logs I see:
[Exception... "The URI is malformed"  nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)"  location: "JS frame :: chrome://messenger/content/msgMail3PaneWindow.js :: ClearMessagePane :: line 1002"  data: no]

Weird :-(
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/69cd9be90a47
Follow-up: fix compile error on Windows (take 3). rs=bustage-fix DONTBUILD
mozmake SOLO_TEST=composition/test-forwarded-eml-actions.js mozmill-one
does fail locally, with:

SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\mail\test\mozmill\composition\test-forwarded-eml-actions.js | test-forwarded-eml-actions.js::test_reply_to_attached_eml
  EXCEPTION: Timed out waiting for window open!
and
SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\mail\test\mozmill\composition\test-forwarded-eml-actions.js | test-forwarded-eml-actions.js::test_forward_attached_eml
  EXCEPTION: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIWebNavigation.currentURI]

Running the test manually I see:
XML Parsing Error: undefined entity Location: file:///C:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/bin/chrome/toolkit/content/global/netError.xhtml Line Number 313, Column 37:
        <h1 id="et_blockedByPolicy">&blockedByPolicy.title;</h1>

Weird :-(

mozmake SOLO_TEST=composition/test-multipart-related.js mozmill-one passes locally, the server log says:

[Exception... "The URI is malformed"  nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)"  location: "JS frame :: chrome://messenger/content/msgMail3PaneWindow.js :: ClearMessagePane :: line 1002"  data: no]

So it really looks like composition/test-forwarded-eml-actions.js fails and drags down the ones that follow.

So let's focus on that one first.
I fixed the XML Parsing Error in bug 1443026. With that fix the screen now reads:
The address wasn't understood
One of the following () is not a registered protocol or is not allowed in this context.
  You might need to install other software to open this address.

So that indicates a problem with a URI somewhere as expected.
OK, looking at the log of the local run for test-forwarded-eml-actions.js I get:

c:/mozilla-source/comm-central/mailnews/base/util/nsMsgMailNewsUrl.cpp, line 624
[3884, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x804B000A: file c:/mozilla-source/comm-central/mailnews/local/src/nsMailboxUrl.cpp, line 181
[3884, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x804B000A: file 

So nsMailboxUrl::CloneInternal() calls nsMsgMailNewsUrl::CloneInternal() and that fails on:
rv = ioService->NewURI(urlSpec, nullptr, nullptr, getter_AddRefs(newUri));

To be continued.
One small copy/paste error and the whole thing falls apart :-(
https://hg.mozilla.org/comm-central/rev/b6af191d5eaa#l11.59

*aUrl was set to null at the start of the function so the SetSpec() never got called. So we had URIs without a spec, nice, leading to errors like:
ASSERTION: No scheme was specified!: '!aScheme.IsEmpty()'

And surely a clone of a URI with an empty spec failed, too :-(

Let's see what works now.
Attachment #8955945 - Flags: review?(acelists)
Comment on attachment 8955945 [details] [diff] [review]
1440693-follow-up-copy-paste.patch

This patch fixes at least three Xpcshell tests:
TEST-UNEXPECTED-FAIL | comm/mailnews/base/test/unit/test_detachToFile.js
TEST-UNEXPECTED-FAIL | comm/mailnews/base/test/unit/test_mimemaltdetach.js
TEST-UNEXPECTED-FAIL | comm/mailnews/imap/test/unit/test_imapAttachmentSaves.js
That then leaves the Xpcshell news tests broken.

Sadly composition/test-forwarded-eml-actions.js now goes crazy and opens a million windows, same for attachment/test-attachment.js :-(
Impressive, with that patch in place, double-clicking an attached messages opened the message without stopping and I had to kill the parent process :-(
Better like this, not that it makes any difference :-(
Attachment #8955945 - Attachment is obsolete: true
Attachment #8955945 - Flags: review?(acelists)
Attachment #8955947 - Flags: review?(acelists)
Comment on attachment 8955890 [details] [diff] [review]
1440693-test_nsLDAPURL.js.patch [landed comment #42]

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

I do not see why m-c prefers this new style, the syntax is ugly, and the QueryInterface doubly so.
But as it fixes the tests...
Attachment #8955890 - Flags: review?(acelists) → review+
Attachment #8955903 - Flags: review?(acelists) → review+
Attachment #8955918 - Flags: review?(acelists) → review+
Attachment #8955919 - Flags: review?(acelists) → review+
Comment on attachment 8955921 [details] [diff] [review]
1440693-host-port.patch [landed comment #42]

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

::: mailnews/extensions/newsblog/content/Feed.js
@@ +35,5 @@
>    {
>      try
>      {
>        let normalizedUrl = Services.io.newURI(aUrl);
> +      let newHost = host.toLowerCase();

What is 'host' here? Did you meant normalizedUrl.host ?
(In reply to :aceman from comment #56)
> What is 'host' here? Did you meant normalizedUrl.host ?
Yep, fixed: https://hg.mozilla.org/comm-central/rev/7efbdc19de35e4453197a7707c2e6007a0d5cf0b
Comment on attachment 8955924 [details] [diff] [review]
1440693-query.patch [landed comment #42]

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

::: mailnews/base/src/nsMailNewsCommandLineHandler.js
@@ -92,5 @@
>            if (file.exists() && file.fileSize > 0) {
>              // Get the URL for this file
>              let fileURL = Services.io.newFileURI(file)
>                                    .QueryInterface(Ci.nsIFileURL);
> -            fileURL.query = "?type=application/x-message-display";

Interestign it worked with the leading question mark. It mast have been eaten automatically :)
Attachment #8955924 - Flags: review?(acelists) → review+
Attachment #8955921 - Flags: review?(acelists) → review+
Attachment #8955947 - Flags: review?(acelists) → review+
Switching off six tests related to opening message attachments. Let's see how far we get since those failures also make subsequent tests fail.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ce71121efcc9
Follow-up of initial C++ patch: Fix copy/paste error. r=aceman
https://hg.mozilla.org/comm-central/rev/887ece68ebe5
Switch off failing tests temporarily. rs=bustage-fix
Remaining test failures:
- attachment/test-attachment.js (party switched off - https://hg.mozilla.org/comm-central/rev/887ece68ebe5)
- composition/test-forwarded-eml-actions.js (party switched off - https://hg.mozilla.org/comm-central/rev/887ece68ebe5)
- message-header/test-phishing-bar.js
There are more failures on Windows like composition/test-charset-edit.js, composition/test-drafts.js, composition/test-image-display.js and more. The first two fail locally, the latter passes and seems to be a subsequent failure.

As we know, there is a problem in news, 7 failing Xpcshell tests:
TEST-UNEXPECTED-FAIL | comm/mailnews/news/test/unit/test_bug695309.js
TEST-UNEXPECTED-FAIL | comm/mailnews/news/test/unit/test_bug403242.js
TEST-UNEXPECTED-FAIL | comm/mailnews/news/test/unit/test_getNewsMessage.js
TEST-UNEXPECTED-FAIL | comm/mailnews/news/test/unit/test_internalUris.js
TEST-UNEXPECTED-FAIL | comm/mailnews/news/test/unit/test_nntpPasswordFailure.js
TEST-UNEXPECTED-TIMEOUT | comm/mailnews/news/test/unit/test_bug540288.js
TEST-UNEXPECTED-TIMEOUT | comm/mailnews/news/test/unit/test_filter.js
TEST-UNEXPECTED-TIMEOUT | comm/mailnews/news/test/unit/test_nntpProxy.js
OK, the phishing bar suffers from the same problem as the attachment tests, only the subtest using a message attachment fails:
SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\mail\test\mozmill\message-header\test-phishing-bar.js | test-phishing-bar.js::test_ignore_phishing_warning_from_eml_attachment
  EXCEPTION: Timed out waiting for notification with value maybeScam to show.

So all those failures appear to have the same common root cause which can also be observed when opening a message attachment manually.
Switch off test-phishing-bar.js::test_ignore_phishing_warning_from_eml_attachment.

Linux and Mac should have their Mozmill green now. So in total as a temporary measure, three tests are partly switched off:
- attachment/test-attachment.js
- composition/test-forwarded-eml-actions.js
- message-header/test-phishing-bar.js
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2936b9ef4ab3
Switch off failing tests temporarily (take 2). rs=bustage-fix
Here's the theory why the NNTP tests and most likely also the attachment tests fail.

nsNntpService::StreamMessage() calls ConstructNntpUrl() which registers a listener on the URL
https://dxr.mozilla.org/comm-central/rev/9804e9b9a400a24ccaba36c58ac0b4ace3a59c73/mailnews/news/src/nsNntpService.cpp#907

Later in StreamMessage() the mailnews URL object is replaced by cloning it in the SetPort() call. The clone doesn't clone the mUrlListeners member of the object, and also not the mAttachmentFileName member (!!), so by setting the port we lose the listeners. And it's downhill from there.
Looks like the theory wasn't right since this patch didn't help.
Attached patch 1440693-hack.patch (obsolete) — Splinter Review
So this makes the NNTP tests pass. I don't use the mutator and set the port directly. I'll do this properly now with a "SetPortInternal()".
It seems that we don't produce a perfect clone or that references to the "pre-clone" copy are held, for example in member variables of the NNTP protocol.

So to be on the safe side, here is our own "set port". It still mutates the underlying URL, but not the whole mailnews URL. NNTP tests pass with this. \o/

That makes me think of the remaining problems: Attachment tests and weird Mozmill failures on Windows only :-(((
Attachment #8956088 - Attachment is obsolete: true
Attachment #8956229 - Attachment is obsolete: true
Attachment #8956252 - Flags: review?(acelists)
This fixes the phishing test and I'm sure the other ones too.
Attachment #8956275 - Flags: review?(acelists)
Comment on attachment 8956252 [details] [diff] [review]
1440693-NNTP-avoid-clone.patch (v1) [landed comment #72]

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

Great work, this fixes all the xpcshell tests in mailnews/news .
Thanks
Attachment #8956252 - Flags: review?(acelists) → review+
Comment on attachment 8956275 [details] [diff] [review]
1440693-avoid-clone2.patch) [landed comment #72]

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

Thanks.
I tried composition/test-forwarded-eml-actions.js and it passes now (I re-enabled the tests).
Attachment #8956275 - Flags: review?(acelists) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a1b563e64f17
Backed out changeset 2936b9ef4ab3 to re-enable test. a=backout
https://hg.mozilla.org/comm-central/rev/d9c7de42afa1
Backed out changeset 887ece68ebe5 to re-enable tests. a=backout
https://hg.mozilla.org/comm-central/rev/b57e09463688
avoid cloning the entire URL in some cases in NNTP. r=aceman
https://hg.mozilla.org/comm-central/rev/8e0def3875f3
avoid cloning the entire URL in some cases. r=aceman
Attachment #8956252 - Attachment description: 1440693-NNTP-avoid-clone.patch (v1) → 1440693-NNTP-avoid-clone.patch (v1) [landed comment #72]
Attachment #8956275 - Attachment description: 1440693-avoid-clone2.patch → 1440693-avoid-clone2.patch) [landed comment #72]
Attachment #8955953 - Attachment description: 1440693-switch-off-tests.patch → 1440693-switch-off-tests.patch) [backed out in comment #72]
Attachment #8956013 - Attachment description: 1440693-switch-off-tests-take2.patch → 1440693-switch-off-tests-take2.patch [backed out in comment #72]
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/715910aab7df
Follow-up: fix compile errors on Windows (take 4). rs=bustage-fix
Linux and Mac Xpcshell runs are green now, but on Windows the following Mozmill test still fail, I noted local results in parenthesis:
composition/test-charset-edit.js (fails test_wrong_reply_charset and test_no_mojibake)
composition/test-drafts.js (fails many tests, including test_remove_space_stuffing_format_flowed)
composition/test-forwarded-content.js (passes)
composition/test-image-display.js (passes)
composition/test-image-insertion-dialog.js (passes, most likely the "normal" failure from bug 1246094)
composition/test-multipart-related.js (passes)
composition/test-send-format.js (passes)

This regressed here:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8cced2a46f73238da13e41bcae8f6f8014&tochange=21f7f94a2c18dc8010ac2f300a36cc4ddd
so it can only be caused by the URI changes. Weird.

I watched the "space stuffing" test and after saving the draft, it opens a different draft to check the content, so that obviously doesn't work. So there appears to be something wrong in addressing the draft.
Running composition/test-drafts.js I noticed that the first subtest test_open_draft_again() fails to delete its own draft. Disabling that subtest makes the other ones pass. Aceman, could you take a look at the mechanism to delete the draft and why it would fail in Windows only(??).

Looking at composition/test-charset-edit.js, it uses getMsgHeaders() which does streamMessage(msgUri, ...), so maybe something wrong there. But the same function is also used in test-multipart-related.js and there it doesn't fail.

Most likely those test failures are failures of the tests and not a consequence of a functional failure.
Flags: needinfo?(acelists)
(In reply to Jorg K (GMT+1) from comment #75)
> Most likely those test failures are failures of the tests and not a
> consequence of a functional failure.
Nope. I found the problem. If you open a saved draft, edit it and save it again, it doesn't supersede the previous version. So then the tests access the wrong message.

So STR:
Compose something, save draft, close window.
Edit draft, save again. You now have two instead of one.

Since superseding drafts is done via their URL somehow, there seems to be another case where we mutated too much :-(
Flags: needinfo?(acelists)
And there is plenty of noisy debug to confirm this:
[5652, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file c:/mozilla-source/comm-central/mailnews/local/src/nsMailboxService.cpp, line 645
[5652, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file c:/mozilla-source/comm-central/mailnews/local/src/nsMailboxService.cpp, line 672
[5652, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file c:/mozilla-source/comm-central/mailnews/compose/src/nsMsgCompose.cpp, line 3367
[5652, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file c:/mozilla-source/comm-central/mailnews/local/src/nsMailboxService.cpp, line 645
[5652, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file c:/mozilla-source/comm-central/mailnews/local/src/nsMailboxService.cpp, line 672
[5652, Main Thread] ###!!! ASSERTION: RemoveCurrentDraftMessage can't get msg header DB interface pointer.: 'NS_SUCCEEDED(rv)', file c:/mozilla-source/comm-central/mailnews/compose/src/nsMsgCompose.cpp, line 331
[5652, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file c:/mozilla-source/comm-central/mailnews/local/src/nsMailboxService.cpp, line 645
[5652, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file c:/mozilla-source/comm-central/mailnews/local/src/nsMailboxService.cpp, line 672
[5652, Main Thread] ###!!! ASSERTION: RemoveCurrentDraftMessage can't get msg header DB interface pointer.: 'NS_SUCCEEDED(rv)', file c:/mozilla-source/comm-central/mailnews/compose/src/nsMsgCompose.cpp, line 3865
[5652, Main Thread] ###!!! ASSERTION: The draft folder MUST be an imap folder in order to mark the msg delete!: 'imapFolder', file c:/mozilla-source/comm-central/mailnews/compose/src/nsMsgCompose.cpp, line 3935

Maybe caused by the mutate here:
https://hg.mozilla.org/comm-central/rev/b6af191d5eaa5a01f39e88ba524a2527455c9b04#l8.13
Actually I think the test failures happen also on Linux 64bit debug, but no other versions of Linux builds.

https://hg.mozilla.org/comm-central/rev/b6af191d5eaa5a01f39e88ba524a2527455c9b04#l8.13 looks interesting. That is why I think we should not mutate any of our URLs and not hunt for individual cases. On the other hand, it may not be trivial to find which variables actually hold a nsMsgMailNewsUrl (or its descendants) when many are declared as nsIURI.
Looking at the context of https://hg.mozilla.org/comm-central/rev/b6af191d5eaa5a01f39e88ba524a2527455c9b04#l8.13, that can't be it. The URL was new there and we're just tweaking it. I'm just trying to fix:
https://hg.mozilla.org/comm-central/rev/b6af191d5eaa5a01f39e88ba524a2527455c9b04#l12.32
and you can see that it goes into charset code at the end of the hunk.

Right, the patch here actually works. I'm assuming your r+ ;-)
I'll land this with the grammar "is have" error fixed.
Keywords: leave-open
Target Milestone: --- → Thunderbird 60.0
Attachment #8956420 - Flags: review?(acelists)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/894cbf4db501
avoid cloning the entire URL in some more cases. r=aceman
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Comment on attachment 8956420 [details] [diff] [review]
1440693-avoid-clone3.patch

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

Nice, thanks.

::: mailnews/compose/src/nsMsgComposeService.cpp
@@ +1334,5 @@
>    // ignore errors here - it's not fatal, and in the case of mailbox messages,
>    // we're always passing in an invalid spec...
> +  nsCOMPtr<nsIMsgMailNewsUrl> mailnewsurl = do_QueryInterface(url);
> +  if (!mailnewsurl) {
> +    NS_WARNING("Trying to run a message throught MIME which is doesn't have a nsIMsgMailNewsUrl?");

What about the typo in throught ? :)
Attachment #8956420 - Flags: review?(acelists) → review+
Grrr, I'll fix it.
You need to log in before you can comment on or make changes to this bug.