Port |Bug 1280584 - About protocol handler totally ignores baseURI and doesn't work for relative (#foo) URIs| to comm-central

RESOLVED FIXED in Thunderbird 50.0

Status

MailNews Core
Networking
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: Ian Neal, Assigned: Jorg K (GMT+2))

Tracking

Trunk
Thunderbird 50.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

11 months ago
See bug 1280584 comment #28 for how it's done:

The definition of nsIURI has changed. You should add the following code:

For URLs that may have a ref:
NS_IMETHODIMP
nsMailtoUrl::CloneWithNewRef(const nsACString& newRef, nsIURI **_newURI)
{
  nsresult rv = Clone(_newURI);
  if (NS_FAILED(rv)) return rv;
  return (*_newURI)->SetRef(newRef);
}
For URLs that can't have a ref:
NS_IMETHODIMP
nsMailtoUrl::CloneWithNewRef(const nsACString& newRef, nsIURI **_newURI)
{
  return Clone(_newURI);
}

Now all we need to figure out is which case we have. Altogether there were four unresolved symbols (bug 1280584 comment #25), so we need to do this for:

nsLDAPURL::CloneWithNewRef in nsLDAPURL.cpp
nsAddbookUrl::CloneWithNewRef in nsAddbookUrl.cpp
nsMsgMailNewsUrl::CloneWithNewRef in nsMsgMailNewsUrl.cpp
nsMailtoUrl::CloneWithNewRef in nsSmtpUrl.cpp

Maybe Kent knows which case we need.
Flags: needinfo?(rkent)
(Reporter)

Comment 2

11 months ago
Created attachment 8775371 [details] [diff] [review]
Potential patch
Attachment #8775371 - Flags: review?(rkent)
Created attachment 8775376 [details] [diff] [review]
Another potential patch.

Looking at the urls I would say no ref but this is just educated guesswork and don't ask me about my c++ knowledge. 

I think the orginal clone method can be used and doesn't need to be implemented linke in IanNs patch. At least Seamonkey compiles with it and doesn't crash when opening mail or the address book.
(Assignee)

Comment 4

11 months ago
Comment on attachment 8775376 [details] [diff] [review]
Another potential patch.

Let's stick a r? onto this one, too ;-)
Attachment #8775376 - Flags: review?(rkent)
We need a third one but not today :)
(Reporter)

Comment 6

11 months ago
As all files have SetRef/GetRef, from the comments in the m-c patch that suggests all files do support refs.
I took the same approach as the m-c patch and "cloned" the CloneIgnoringRef method and tweaked it to create CloneWithNewRef method.
(Reporter)

Comment 7

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e51d8037ff5e
(Reporter)

Comment 8

11 months ago
Once completed, builds and logs will be available at:
https://archive.mozilla.org/pub/thunderbird/try-builds/iann_cvs@blueyonder.co.uk-e51d8037ff5e6bb53378502d00767f49825a7e52/
(Assignee)

Comment 9

11 months ago
Ian was working very very late, so by accident he submitted "try: -b -do -p all -u all -t none".
It must be "-b do", so nothing actually happened. I shipped off another one:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=14844fca2017

I don't see why debug vs. opt would make any difference here, so I just requested opt builds.

Results here:
https://archive.mozilla.org/pub/thunderbird/try-builds/mozilla@jorgk.com-14844fca2017d8940b1b2ca0278d07efbf30cbd4/
(Assignee)

Comment 10

11 months ago
Comment on attachment 8775376 [details] [diff] [review]
Another potential patch.

Looking further into it, Ian's approach seems correct, so IMHO no review for this simple patch. Sorry for the interference.
Attachment #8775376 - Flags: review?(rkent)
(Assignee)

Comment 11

11 months ago
Comment on attachment 8775371 [details] [diff] [review]
Potential patch

Apart from the today's expected failures the try run looks good.

Ian's approach to "clone" CloneIgnoringRef() makes sense to me.

So I'd give this r+ (r=jorgk), but of course we can wait for Kent's opinion.
Flags: needinfo?(rkent)
(Assignee)

Updated

11 months ago
Attachment #8775371 - Flags: review+

Comment 12

11 months ago
Comment on attachment 8775371 [details] [diff] [review]
Potential patch

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

It's always a tough call when we try to fix things, should we allow further deterioration of existing code or design that has issues, or try to move it toward being a little better? Do we put lipstick on a pig so to speak, or do we go ahead and break more windows because there are already broken windows in the neighborhood (the theory which so-called Broken Windows policing is fighting).

I'm encouraging us here to move just a little bit forward instead of a little backwards with some simple changes. I do appreciate the effort in keeping the tree open!

::: ldap/xpcom/src/nsLDAPURL.cpp
@@ +684,5 @@
>  
>  NS_IMETHODIMP
> +nsLDAPURL::CloneWithNewRef(const nsACString& newRef, nsIURI** result)
> +{
> +  return mBaseURL->CloneWithNewRef(newRef, result);

This does not return an nsLDAPURL which is certainly not what I would expect from a clone procedure. Yes you are copying from ::CloneIgnoringRef but that is a footgun as well.

Rather than do an incorrect implementation, if this is never used the correct approach is NS_ERROR_NOT_IMPLEMENTED but I would much prefer a real implementation. Probably the best approach would be to take the current code for nsLDAPURL::Clone and make it a private CloneInternal, and call it from each of the standard methods ::Clone(), ::CloneIgnoringRef(), and ::CloneWithNewRef() with appropriate parameters.

::: mailnews/addrbook/src/nsAddbookUrl.cpp
@@ +245,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +nsAddbookUrl::CloneWithNewRef(const nsACString& newRef, nsIURI** _retval)

You are duplicating the code from ::Clone() here. Could you instead add a private ::CloneInternal and call it from each of the NS_IMETHODIMP ::clone variants?
Attachment #8775371 - Flags: review?(rkent) → review-
(Reporter)

Updated

11 months ago
Duplicate of this bug: 1290194
(Reporter)

Updated

11 months ago
Assignee: nobody → iann_bugzilla
Status: NEW → ASSIGNED
>> Looking further into it, Ian's approach seems correct, so IMHO no review for this simple
>> patch. Sorry for the interference.

No problem on my side.

>> As all files have SetRef/GetRef, from the comments in the m-c patch that suggests 
>> all files do support refs.

Didn't have much time today. I only did a quick check in dxr. It seems the SetRef methods might be only used in the mailnews urls. Would it be worth to look further into this and remove some of them?

FRG
(Assignee)

Comment 15

11 months ago
Created attachment 8775788 [details] [diff] [review]
This is what Kent suggested.

Sorry Ian, thanks for making a start. I was looking for you on IRC but didn't see you. This is a little urgent ;-)
Assignee: iann_bugzilla → mozilla
Attachment #8775371 - Attachment is obsolete: true
Attachment #8775376 - Attachment is obsolete: true
Attachment #8775788 - Flags: review?(rkent)
(Assignee)

Comment 16

11 months ago
Created attachment 8775814 [details] [diff] [review]
Alternative solution

Alternative:
Merged
nsMsgMailNewsUrl::CloneInternal with nsMsgMailNewsUrl::Clone and
nsMailtoUrl     ::CloneInternal with nsMailtoUrl     ::Clone

Take your pick.
Attachment #8775814 - Flags: review?(rkent)

Comment 17

11 months ago
Comment on attachment 8775814 [details] [diff] [review]
Alternative solution

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

I have a couple of small changes, but I don't need to see it again. r+=me

::: mailnews/addrbook/src/nsAddbookUrl.cpp
@@ +234,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>    clone->ParseUrl();
>    clone.forget(_retval);
>    return NS_OK;
> +}	

Nit: trailing space

::: mailnews/base/util/nsMsgMailNewsUrl.cpp
@@ +539,5 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>      msgMailNewsUrl->SetMsgWindow(msgWindow);
>    }
>  
> +  if (aRefHandlingMode == eReplaceRef || aRefHandlingMode == eIgnoreRef) {

This is not really correct. You are assuming that anyone with eIgnoreRef is going to set the ref to blank. But there is no real reason to assume that. You should set ref to blank if eIgnoreRef
Attachment #8775814 - Flags: review?(rkent) → review+

Comment 18

11 months ago
Comment on attachment 8775788 [details] [diff] [review]
This is what Kent suggested.

The other version is fine.
Attachment #8775788 - Flags: review?(rkent) → review-
(Assignee)

Comment 19

11 months ago
The trailing space was already there. Only BMO shows it as changed.
I'll fix the other issue.
Can you do me a favour? Check (and perhaps explain) the ref-counting business.
There are a few "forget"s and there is a NS_ADDREF(*aResult = clone);
Is this correct? I left it as it was.
(Assignee)

Comment 20

11 months ago
New try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4ef30c70f89c
(Assignee)

Comment 21

11 months ago
Created attachment 8775888 [details] [diff] [review]
Alternative solution - tests modified

Sadly this failed:
mozilla/mach xpcshell-test mailnews/jsaccount/test/unit/test_fooUrl.js

You're assuming something about the internals of nsMsgMailNewsUrl, namely that nsMsgMailNewsUrl::CloneIgnoringRef calls Clone() which is not longer the case.

I fixed the test by overriding both, but then the "tricky" bit doesn't work any more since it relies on that behaviour.

Please advise how you'd like it or correct the patch as you deem fit.

(In this version I also changed the names of the return values to make them consistent and moved one block of code, so the interdiff might look unexpected).
Attachment #8775788 - Attachment is obsolete: true
Attachment #8775814 - Attachment is obsolete: true
Flags: needinfo?(rkent)
Attachment #8775888 - Flags: review?(rkent)
(Reporter)

Comment 22

11 months ago
I might be reading the test incorrectly, but is another option in the test to override cloneInternal rather than clone and cloneIgnoringRef?
Flags: needinfo?(mozilla)
(Reporter)

Comment 23

11 months ago
Plus, does the test also need to test cloneWithNewRef?
(Assignee)

Comment 24

11 months ago
Comment on attachment 8775888 [details] [diff] [review]
Alternative solution - tests modified

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

::: mailnews/jsaccount/test/unit/test_fooUrl.js
@@ -74,5 @@
>      Assert.equal(cppDelegator.cloneIgnoringRef().spec, "https://test.invalid/FOLDER");
>  
>      // The cppBase object will not have the overrides.
>      let cppBase = url.QueryInterface(Ci.msgIOverride).cppBase.QueryInterface(Ci.nsIURI); 
>      Assert.equal(cppBase.clone().spec, "https://test.invalid/folder#modules");

Note: result is lower case, since the base clone() is not overridden.

@@ -78,5 @@
>      Assert.equal(cppBase.clone().spec, "https://test.invalid/folder#modules");
> -
> -    // But then it gets tricky. We can call cloneIgnoringRef in the C++ base
> -    // but it will use the virtual clone which is overridden.
> -    Assert.equal(cppBase.cloneIgnoringRef().spec, "https://test.invalid/FOLDER");

I've been looking at it for a while now. Why did that ever work? The base cloneIgnoringRef() will use something from the superclass? How does the base class even know of the superclass.

Anyway, this is a comment on a removed test since it doesn't work any more when cloneIgnoringRef() does *not* call clone().
(Assignee)

Comment 25

11 months ago
(In reply to Ian Neal from comment #22)
> I might be reading the test incorrectly, but is another option in the test
> to override cloneInternal rather than clone and cloneIgnoringRef?
CloneInternal is not exposed to be overridden, as I see it.

(In reply to Ian Neal from comment #23)
> Plus, does the test also need to test cloneWithNewRef?
If Kent wants. So far it looks like he just wasn't to test that he can override C++ with JS.

I'd wait for Kent who is working today. He has r?, NI and a personal mail, so he will see it and he knows it's urgent.
Flags: needinfo?(mozilla)

Comment 26

11 months ago
Comment on attachment 8775888 [details] [diff] [review]
Alternative solution - tests modified

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

The original bug has a comment that says "This was... more work than I anticipated.". Looks like this is true for c-c too ;)
(Assignee)

Comment 27

11 months ago
(In reply to aleth [:aleth] from comment #26)
> Looks like this is true for c-c too ;)
That's because Kent wanted to do a little clean-up, see comment #12: "Put lipstick on a pig" and not "break windows". Ian's patch initial worked, but the clean-up took a little longer. And to add insult to injury, in the end one of Kent's tricky tests:
https://dxr.mozilla.org/comm-central/source/mailnews/jsaccount/test/unit/test_fooUrl.js#80
failed because due to the clean-up (which got r+) one of the (illegal?) assumptions of the test isn't true any more. I expect that to be resolved in the next few hours when Kent looks at it.
(Assignee)

Comment 28

11 months ago
To fix the current bustage, the reviewed solution got landed. That does *not* a fix for the text which now fails, see comment #21. Test failure is in
mozilla/mach xpcshell-test mailnews/jsaccount/test/unit/test_fooUrl.js

Kent promised to look into the test failure.

Landed:
https://hg.mozilla.org/comm-central/rev/ee02c089bb8e
(I'll attach the patch again in the next comment).
Flags: needinfo?(rkent)
Keywords: leave-open
Target Milestone: --- → Thunderbird 50.0
(Assignee)

Comment 29

11 months ago
Created attachment 8776097 [details] [diff] [review]
Alternative solution - landed [comment #28]

Carrying forward Kent's r+:

Addressed review issues.

In this version I also changed the names of the return values to make them consistent and moved one block of code to reduce differences.
Attachment #8776097 - Flags: review+

Updated

11 months ago
Blocks: 1290541

Comment 30

11 months ago
Let's fix the test in another bug, otherwise status is going to be a nightmare with the uplift pending.
(Assignee)

Comment 31

11 months ago
OK, then we're done here.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
(Assignee)

Updated

11 months ago
Keywords: leave-open
(Assignee)

Comment 32

11 months ago
Comment on attachment 8775888 [details] [diff] [review]
Alternative solution - tests modified

Suggested test changes no longer required. Kent will follow up in bug 1290541.
Attachment #8775888 - Attachment is obsolete: true
Attachment #8775888 - Flags: review?(rkent)
You need to log in before you can comment on or make changes to this bug.