Closed
Bug 1440693
Opened 7 years ago
Closed 7 years ago
Port bug 1433958 and bug 1434163 to C-C: nsIURI attributes will become read-only
Categories
(Thunderbird :: General, enhancement)
Thunderbird
General
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®exp=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®exp=false&path=
is a little clearer, but we should forget += assignments:
https://searchfox.org/comm-central/search?q=.pathQueryRef+%2B%3D&case=false®exp=false&path=
Assignee | ||
Comment 1•7 years ago
|
||
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
Assignee | ||
Comment 2•7 years ago
|
||
This should address the address book URL. More to come (of course).
Assignee | ||
Comment 3•7 years ago
|
||
Address book and LDAP.
Assignee | ||
Comment 4•7 years ago
|
||
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?
Assignee | ||
Comment 6•7 years ago
|
||
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
Assignee | ||
Comment 7•7 years ago
|
||
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?
Assignee | ||
Comment 9•7 years ago
|
||
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.
![]() |
||
Comment 10•7 years ago
|
||
(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.
Assignee | ||
Comment 11•7 years ago
|
||
(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 | ||
Comment 12•7 years ago
|
||
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)
Assignee | ||
Comment 13•7 years ago
|
||
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)
Assignee | ||
Comment 14•7 years ago
|
||
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)
Assignee | ||
Comment 15•7 years ago
|
||
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)
Assignee | ||
Comment 16•7 years ago
|
||
Killing last warning.
Attachment #8955837 -
Attachment is obsolete: true
Attachment #8955837 -
Flags: review?(acelists)
Attachment #8955839 -
Flags: review?(acelists)
![]() |
||
Comment 17•7 years ago
|
||
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 18•7 years ago
|
||
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.
Assignee | ||
Comment 19•7 years ago
|
||
(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.
Assignee | ||
Comment 20•7 years ago
|
||
Addressed the comments and removed unnecessary Clone() calls. Try interdiff.
Attachment #8955839 -
Attachment is obsolete: true
Attachment #8955861 -
Flags: review+
Assignee | ||
Comment 21•7 years ago
|
||
Assignee | ||
Comment 22•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Assignee | ||
Comment 23•7 years ago
|
||
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+
Comment 24•7 years ago
|
||
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
Assignee | ||
Comment 25•7 years ago
|
||
This didn't fail in a local compile :-(
I'll push it once we looked at test failures.
Assignee | ||
Updated•7 years ago
|
Attachment #8955873 -
Attachment description: 1440693-nsIURI-readonly.patch - (v14) → 1440693-nsIURI-readonly.patch - (v14) [landed comment #24]
Comment 26•7 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/43ead9ebb035
Follow-up: fix compile error on Windows. rs=bustage-fix DONTBUILD
Assignee | ||
Comment 27•7 years ago
|
||
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";
Assignee | ||
Updated•7 years ago
|
Attachment #8955879 -
Attachment description: 1440693-follow-up.patch → 1440693-follow-up.patch [landed comment #26]
Assignee | ||
Comment 28•7 years ago
|
||
We're going to be here for a while fixing tests :-( Here is the first one.
Attachment #8955890 -
Flags: review?(acelists)
Assignee | ||
Comment 29•7 years ago
|
||
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)
Assignee | ||
Comment 30•7 years ago
|
||
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)
Assignee | ||
Comment 31•7 years ago
|
||
Attachment #8955903 -
Flags: review?(acelists)
Comment 32•7 years ago
|
||
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)
Assignee | ||
Comment 33•7 years ago
|
||
It clones, also see here:
https://hg.mozilla.org/comm-central/rev/0eaecba4fc41#l2.12
The clone call is here:
https://searchfox.org/mozilla-central/rev/6cf5ac594a21db6df359dc49f4aa651fef59a27b/netwerk/base/nsStandardURL.cpp#2245
https://searchfox.org/mozilla-central/source/__GENERATED__/dist/include/nsIURIMutator.h#45
Flags: needinfo?(jorgk)
Assignee | ||
Comment 34•7 years ago
|
||
Attachment #8955918 -
Flags: review?(acelists)
Assignee | ||
Comment 35•7 years ago
|
||
Attachment #8955919 -
Flags: review?(acelists)
Assignee | ||
Comment 36•7 years ago
|
||
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 37•7 years ago
|
||
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+
Assignee | ||
Comment 38•7 years ago
|
||
Attachment #8955921 -
Flags: review?(acelists)
Assignee | ||
Comment 39•7 years ago
|
||
Attachment #8955924 -
Flags: review?(acelists)
Assignee | ||
Comment 40•7 years ago
|
||
Thanks for the review, four eyes see more than two ;-)
Sorry about the copy/paste errors.
Attachment #8955925 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Attachment #8955920 -
Attachment is obsolete: true
Assignee | ||
Comment 41•7 years ago
|
||
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.
Comment 42•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Attachment #8955890 -
Attachment description: 1440693-test_nsLDAPURL.js.patch → 1440693-test_nsLDAPURL.js.patch [landed comment #42]
Assignee | ||
Updated•7 years ago
|
Attachment #8955903 -
Attachment description: 1440693-test_bug366491.js.patch → 1440693-test_bug366491.js.patch [landed comment #42]
Assignee | ||
Updated•7 years ago
|
Attachment #8955918 -
Attachment description: 1440693-test_postPluginFilters.js.patch → 1440693-test_postPluginFilters.js.patch [landed comment #42]
Assignee | ||
Updated•7 years ago
|
Attachment #8955919 -
Attachment description: 1440693-test_bayesian.patch → 1440693-test_bayesian.patch [landed comment #42]
Assignee | ||
Updated•7 years ago
|
Attachment #8955921 -
Attachment description: 1440693-host-port.patch → 1440693-host-port.patch [landed comment #42]
Assignee | ||
Updated•7 years ago
|
Attachment #8955924 -
Attachment description: 1440693-query.patch → 1440693-query.patch [landed comment #42]
Assignee | ||
Updated•7 years ago
|
Attachment #8955925 -
Attachment description: 1440693-calendar.patch (v1d) → 1440693-calendar.patch (v1d) [landed comment #42]
Comment 43•7 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7efbdc19de35
fix setting of host (take 2, typo fix). r=me DONTBUILD
Comment 44•7 years ago
|
||
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
Assignee | ||
Comment 45•7 years ago
|
||
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 :-(
Assignee | ||
Comment 46•7 years ago
|
||
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 :-(
Comment 47•7 years ago
|
||
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
Assignee | ||
Comment 48•7 years ago
|
||
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.
Assignee | ||
Comment 49•7 years ago
|
||
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.
Assignee | ||
Comment 50•7 years ago
|
||
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.
Assignee | ||
Comment 51•7 years ago
|
||
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)
Assignee | ||
Comment 52•7 years ago
|
||
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 :-(
Assignee | ||
Comment 53•7 years ago
|
||
Impressive, with that patch in place, double-clicking an attached messages opened the message without stopping and I had to kill the parent process :-(
Assignee | ||
Comment 54•7 years ago
|
||
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 55•7 years ago
|
||
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 56•7 years ago
|
||
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 ?
Assignee | ||
Comment 57•7 years ago
|
||
(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 58•7 years ago
|
||
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+
Assignee | ||
Comment 59•7 years ago
|
||
Switching off six tests related to opening message attachments. Let's see how far we get since those failures also make subsequent tests fail.
Comment 60•7 years ago
|
||
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
Assignee | ||
Comment 61•7 years ago
|
||
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
Assignee | ||
Comment 62•7 years ago
|
||
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.
Assignee | ||
Comment 63•7 years ago
|
||
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
Comment 64•7 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2936b9ef4ab3
Switch off failing tests temporarily (take 2). rs=bustage-fix
Assignee | ||
Comment 65•7 years ago
|
||
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.
Assignee | ||
Comment 66•7 years ago
|
||
Looks like the theory wasn't right since this patch didn't help.
Assignee | ||
Comment 67•7 years ago
|
||
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()".
Assignee | ||
Comment 68•7 years ago
|
||
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)
Assignee | ||
Comment 69•7 years ago
|
||
This fixes the phishing test and I'm sure the other ones too.
Attachment #8956275 -
Flags: review?(acelists)
![]() |
||
Comment 70•7 years ago
|
||
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 71•7 years ago
|
||
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+
Comment 72•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Attachment #8956252 -
Attachment description: 1440693-NNTP-avoid-clone.patch (v1) → 1440693-NNTP-avoid-clone.patch (v1) [landed comment #72]
Assignee | ||
Updated•7 years ago
|
Attachment #8956275 -
Attachment description: 1440693-avoid-clone2.patch → 1440693-avoid-clone2.patch) [landed comment #72]
Assignee | ||
Updated•7 years ago
|
Attachment #8955953 -
Attachment description: 1440693-switch-off-tests.patch → 1440693-switch-off-tests.patch) [backed out in comment #72]
Assignee | ||
Updated•7 years ago
|
Attachment #8956013 -
Attachment description: 1440693-switch-off-tests-take2.patch → 1440693-switch-off-tests-take2.patch [backed out in comment #72]
Comment 73•7 years ago
|
||
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
Assignee | ||
Comment 74•7 years ago
|
||
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.
Assignee | ||
Comment 75•7 years ago
|
||
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)
Assignee | ||
Comment 76•7 years ago
|
||
(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)
Assignee | ||
Comment 77•7 years ago
|
||
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
![]() |
||
Comment 78•7 years ago
|
||
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.
Assignee | ||
Comment 79•7 years ago
|
||
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+ ;-)
Assignee | ||
Comment 80•7 years ago
|
||
I'll land this with the grammar "is have" error fixed.
Keywords: leave-open
Target Milestone: --- → Thunderbird 60.0
Assignee | ||
Updated•7 years ago
|
Attachment #8956420 -
Flags: review?(acelists)
Comment 81•7 years ago
|
||
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: 7 years ago
Resolution: --- → FIXED
![]() |
||
Comment 82•7 years ago
|
||
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+
Assignee | ||
Comment 83•7 years ago
|
||
Grrr, I'll fix it.
Comment 84•7 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/278d1f336d04
typo fix. rs=typo-fix
You need to log in
before you can comment on or make changes to this bug.
Description
•