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

RESOLVED FIXED in Thunderbird 60.0

Status

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

Trunk
Thunderbird 60.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(15 attachments, 19 obsolete attachments)

115.91 KB, patch
jorgk
: 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
: 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
Assignee

Description

Last year
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=
Assignee

Comment 1

Last year
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

Last year
This should address the address book URL. More to come (of course).
Assignee

Comment 3

Last year
Address book and LDAP.
Assignee

Comment 4

Last year
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 5

Last year
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

Last year
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

Last year
More stuff, still not compiling. I've created a nsIMsgMailNewsUrl::SetSpecInternal().
Attachment #8954427 - Attachment is obsolete: true

Comment 8

Last year
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

Last year
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

Last year
(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

Last year
(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
Assignee

Comment 12

Last year
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

Last year
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

Last year
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

Last year
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

Last year
Killing last warning.
Attachment #8955837 - Attachment is obsolete: true
Attachment #8955837 - Flags: review?(acelists)
Attachment #8955839 - Flags: review?(acelists)

Comment 17

Last year
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

Last year
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

Last year
(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

Last year
Addressed the comments and removed unnecessary Clone() calls. Try interdiff.
Attachment #8955839 - Attachment is obsolete: true
Attachment #8955861 - Flags: review+
Assignee

Comment 22

Last year
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

Last year
Keywords: leave-open
Assignee

Comment 23

Last year
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

Last year
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

Last year
This didn't fail in a local compile :-(

I'll push it once we looked at test failures.
Assignee

Updated

Last year
Attachment #8955873 - Attachment description: 1440693-nsIURI-readonly.patch - (v14) → 1440693-nsIURI-readonly.patch - (v14) [landed comment #24]

Comment 26

Last year
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

Last year
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

Last year
Attachment #8955879 - Attachment description: 1440693-follow-up.patch → 1440693-follow-up.patch [landed comment #26]
Assignee

Comment 28

Last year
We're going to be here for a while fixing tests :-( Here is the first one.
Attachment #8955890 - Flags: review?(acelists)
Assignee

Comment 29

Last year
Posted 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)
Assignee

Comment 30

Last year
Posted 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)

Comment 32

Last year
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 36

Last year
Posted 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 37

Last year
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

Last year
Attachment #8955921 - Flags: review?(acelists)
Assignee

Comment 39

Last year
Attachment #8955924 - Flags: review?(acelists)
Assignee

Comment 40

Last year
Thanks for the review, four eyes see more than two ;-)
Sorry about the copy/paste errors.
Attachment #8955925 - Flags: review+
Assignee

Updated

Last year
Attachment #8955920 - Attachment is obsolete: true
Assignee

Comment 41

Last year
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

Last year
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

Last year
Attachment #8955890 - Attachment description: 1440693-test_nsLDAPURL.js.patch → 1440693-test_nsLDAPURL.js.patch [landed comment #42]
Assignee

Updated

Last year
Attachment #8955903 - Attachment description: 1440693-test_bug366491.js.patch → 1440693-test_bug366491.js.patch [landed comment #42]
Assignee

Updated

Last year
Attachment #8955918 - Attachment description: 1440693-test_postPluginFilters.js.patch → 1440693-test_postPluginFilters.js.patch [landed comment #42]
Assignee

Updated

Last year
Attachment #8955919 - Attachment description: 1440693-test_bayesian.patch → 1440693-test_bayesian.patch [landed comment #42]
Assignee

Updated

Last year
Attachment #8955921 - Attachment description: 1440693-host-port.patch → 1440693-host-port.patch [landed comment #42]
Assignee

Updated

Last year
Attachment #8955924 - Attachment description: 1440693-query.patch → 1440693-query.patch [landed comment #42]
Assignee

Updated

Last year
Attachment #8955925 - Attachment description: 1440693-calendar.patch (v1d) → 1440693-calendar.patch (v1d) [landed comment #42]

Comment 43

Last year
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

Last year
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

Last year
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

Last year
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

Last year
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

Last year
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

Last year
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

Last year
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

Last year
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

Last year
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

Last year
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

Last year
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

Last year
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+

Updated

Last year
Attachment #8955903 - Flags: review?(acelists) → review+

Updated

Last year
Attachment #8955918 - Flags: review?(acelists) → review+

Updated

Last year
Attachment #8955919 - Flags: review?(acelists) → review+

Comment 56

Last year
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

Last year
(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

Last year
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+

Updated

Last year
Attachment #8955921 - Flags: review?(acelists) → review+

Updated

Last year
Attachment #8955947 - Flags: review?(acelists) → review+
Assignee

Comment 59

Last year
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

Last year
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

Last year
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

Last year
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

Last year
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

Last year
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

Last year
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

Last year
Looks like the theory wasn't right since this patch didn't help.
Assignee

Comment 67

Last year
Posted 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()".
Assignee

Comment 68

Last year
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

Last year
This fixes the phishing test and I'm sure the other ones too.
Attachment #8956275 - Flags: review?(acelists)

Comment 70

Last year
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

Last year
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

Last year
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

Last year
Attachment #8956252 - Attachment description: 1440693-NNTP-avoid-clone.patch (v1) → 1440693-NNTP-avoid-clone.patch (v1) [landed comment #72]
Assignee

Updated

Last year
Attachment #8956275 - Attachment description: 1440693-avoid-clone2.patch → 1440693-avoid-clone2.patch) [landed comment #72]
Assignee

Updated

Last year
Attachment #8955953 - Attachment description: 1440693-switch-off-tests.patch → 1440693-switch-off-tests.patch) [backed out in comment #72]
Assignee

Updated

Last year
Attachment #8956013 - Attachment description: 1440693-switch-off-tests-take2.patch → 1440693-switch-off-tests-take2.patch [backed out in comment #72]

Comment 73

Last year
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

Last year
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

Last year
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

Last year
(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

Last year
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

Last year
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

Last year
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

Last year
I'll land this with the grammar "is have" error fixed.
Keywords: leave-open
Target Milestone: --- → Thunderbird 60.0
Assignee

Updated

Last year
Attachment #8956420 - Flags: review?(acelists)

Comment 81

Last year
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: Last year
Resolution: --- → FIXED

Comment 82

Last year
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

Last year
Grrr, I'll fix it.
You need to log in before you can comment on or make changes to this bug.