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.

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.

Comment on attachment 8775376 [details] [diff] [review] Another potential patch. Let's stick a r? onto this one, too ;-)

We need a third one but not today :)

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.

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

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/

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.

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.

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?

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

Sorry Ian, thanks for making a start. I was looking for you on IRC but didn't see you. This is a little urgent ;-)

Alternative: Merged nsMsgMailNewsUrl::CloneInternal with nsMsgMailNewsUrl::Clone and nsMailtoUrl ::CloneInternal with nsMailtoUrl ::Clone Take your pick.

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

Comment on attachment 8775788 [details] [diff] [review] This is what Kent suggested. The other version is fine.

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.

New try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4ef30c70f89c

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).

I might be reading the test incorrectly, but is another option in the test to override cloneInternal rather than clone and cloneIgnoringRef?

Plus, does the test also need to test cloneWithNewRef?

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().

(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.

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 ;)

(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.

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).

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.

Let's fix the test in another bug, otherwise status is going to be a nightmare with the uplift pending.

OK, then we're done here.

Comment on attachment 8775888 [details] [diff] [review] Alternative solution - tests modified Suggested test changes no longer required. Kent will follow up in bug 1290541.