Closed Bug 1418011 Opened 2 years ago Closed 2 years ago

Fallout from bug 1416343: TEST-UNEXPECTED-FAIL | mailnews/jsaccount/test/unit/test_fooUrl.js | test_nsIURI - [test_nsIURI : 63] "https://test.invalid/folder#modules" == "https://test.invalid/FOLDER#MODULES"

Categories

(MailNews Core :: Backend, defect)

defect
Not set

Tracking

(thunderbird60+)

RESOLVED FIXED
Thunderbird 60.0
Tracking Status
thunderbird60 + ---

People

(Reporter: jorgk, Assigned: rkent)

References

Details

(Keywords: regression, Whiteboard: [Thunderbird-temporary-fix])

Attachments

(2 files, 1 obsolete file)

TEST-UNEXPECTED-FAIL | mailnews/jsaccount/test/unit/test_fooUrl.js | test_nsIURI - [test_nsIURI : 63] "https://test.invalid/folder#modules" == "https://test.invalid/FOLDER#MODULES"

Looking at the test we see:

    // Test JS override of method called virtually in C++.
    // We overrode cloneInteral in JS to capitalize the path.
    url.spec = "https://test.invalid/folder#modules";
63  Assert.equal(url.cloneInternal(Ci.nsIMsgMailNewsUrl.HONOR_REF, null).spec,
                 "https://test.invalid/FOLDER#MODULES");

Since the M-C URI/URL classes have been marked "built-in", they can't be overridden in JS any more. For an explanation of "built-in", refer to bug 1060051 comment #14 (quote):
[Built-in] means the implementation would be C++ only. JS could still use it, but not re-implement.

So this test fails. The owner of this code, Kent, needs do decide how he wants to proceed here.

All I can do is disable the test for now.

Note bug 1417696:
https://hg.mozilla.org/comm-central/rev/ac5e3ca48e15496fbec777e2a74110f808b253bd
Flags: needinfo?(rkent)
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5acc341c64ac
disable failing part of test_fooUrl.js. rs=bustage-fix
Note bug 1416343 comment #12 (quote):
This is a necessary step in our plan to make nsIURI thread safe (bug 922464).
Being implemented in JS makes it impossible to be used OMT.

For now, you can probably revert this change in Thunderbird code. As we change
a lot of the code to use the new URI API, keeping up with the changes could be
a bit more difficult. But if you are sure the JS implemented URIs are only used
on the main thread, you can create a new C++ wrapper that forwards calls to the
JS implementation.
This also fixes and re-enables test_fooUrl.js making bug 1432191 obsolete for now. That test does a lot less, since we can no longer override nsIURI, nsIURL, or nsIMsgMailNewsURL. Missing features caused by that disabled override are added where needed in a new msgIJaUrl.
Flags: needinfo?(rkent)
Attachment #8953666 - Flags: review?(jorgk)
Kent, please take note of bug 1440693. I haven't looked at all, but discontinuing setters might affect this area.
Duplicate of this bug: 1432191
(In reply to Jorg K (GMT+1) from comment #5)
> Kent, please take note of bug 1440693. I haven't looked at all, but
> discontinuing setters might affect this area.

Yes it will affect it, but for TB 60 and JsAccount the answer is going to be the same approach - add setters to msgIJaURI that will replace the missing setters.

The missing setters need to exist as non-XPCOM items in C++, but we can still create setters on another interface that does not derive from nsIURI. Perhaps we can figure out how to get mutators setup so that we can do this properly in the Gecko way, but I still think that might be best viewed as technical debt rather than something we need to do from the gitgo.

But that assumes that, at TB 60, there are not dramatic changes in the handling of nsIURI objects in the backend that will cause us issues when POP3 and similar methods run a URI.

A POP3 url is ultimately run using NS_NewInputStreamPump() then pump->AsyncRead(... url), so we are passing our URL into the innards of Gecko to run it on a separate thread. That's where we will have potential problems.

JsAccount though does not typically work that way, at least with ExQuilla. Protocol objects use  XMLHttpRequest, which takes a string url rather than an nsIURI. So it is much more forgiving of changes in nsIURI. You still need to pass around that nsIURI through the protocol, but it is not used directly in the non-main thread - unlike other mail protocols.
(In reply to Kent James (:rkent) from comment #7)
> The missing setters need to exist as non-XPCOM items in C++, but we can
> still create setters on another interface that does not derive from nsIURI.
> Perhaps we can figure out how to get mutators setup so that we can do this
> properly in the Gecko way, but I still think that might be best viewed as
> technical debt rather than something we need to do from the gitgo.
Kent, could you check whether we can set up the mutators the "Gecko way"? The code is already there
https://dxr.mozilla.org/comm-central/search?q=%3A%3AMutate&redirect=false
for example:
https://dxr.mozilla.org/comm-central/rev/5aeb32aaa92b73257d9b47673a85306814834bf9/ldap/xpcom/src/nsLDAPURL.h#43
https://dxr.mozilla.org/comm-central/rev/5aeb32aaa92b73257d9b47673a85306814834bf9/ldap/xpcom/src/nsLDAPURL.cpp#771
and it even seems to work:
https://dxr.mozilla.org/comm-central/rev/5aeb32aaa92b73257d9b47673a85306814834bf9/ldap/xpcom/tests/unit/test_nsLDAPURL.js#190
https://dxr.mozilla.org/comm-central/rev/5aeb32aaa92b73257d9b47673a85306814834bf9/ldap/xpcom/tests/unit/test_nsLDAPURL.js#261

Valentin mentioned that the mutator for the derived classes of nsIMsgMailNewsUrl, was missing, in this case the SMTP URL, so I already tried to fix this in attachment 8944332 [details] [diff] [review] (adding the nsSmtpUrl::Mutate() code, see file nsSmtpUrl.cpp). We could add those to the derived classes "wholesale" if necessary without causing problems.

I'm still not sure whether the mutators of nsIMsgMailNewsUrl and derived classes, which are relying on Clone()
https://searchfox.org/mozilla-central/source/__GENERATED__/dist/include/nsIURIMutator.h#44
will work given our discussion in bug 1432204 comment #31, the (quote)
  "Doing mutate() on an nsMsgMailNewsUrl appeared to cause this error: NS_ERROR_MALFORMED_URL".
But most likely that was caused trying to mutate/clone a URL before it was set up fully.
Flags: needinfo?(rkent)
Comment on attachment 8953666 [details] [diff] [review]
Add missing features to a new idl msgIJaUrl

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

More URL magic ;-)

::: mailnews/jsaccount/public/msgIJaUrl.idl
@@ +15,5 @@
> +interface msgIJaUrl : nsISupports
> +{
> +  /**
> +   * Set the url type, which will be checked by nsIMsgMailNewsUrl::IsUrlType. See
> +   * IsUrlType for possible values

Nit: URL upper case as below, full stop missing.

::: mailnews/jsaccount/src/JaUrl.h
@@ +45,5 @@
>    // nsIMsgMailNewsUrl overrides
>    NS_IMETHOD GetFolder(nsIMsgFolder **aFolder) override;
>    NS_IMETHOD SetFolder(nsIMsgFolder *aFolder) override;
> +  NS_IMETHOD IsUrlType(uint32_t type, bool *isType) override;
> +  NS_IMETHOD GetServer(nsIMsgIncomingServer **aIncomingServer);

`override` missing? Can you please enlighten me why these two overrides are necessary now when they weren't necessary before. What am I missing?
Attachment #8953666 - Flags: review?(jorgk) → review+
Flags: needinfo?(rkent)
(In reply to Jorg K (GMT+1) from comment #9)
> Comment on attachment 8953666 [details] [diff] [review]
> Nit: URL upper case as below, full stop missing.

Fixed in my local copy.

> 
> ::: mailnews/jsaccount/src/JaUrl.h
> @@ +45,5 @@
> >    // nsIMsgMailNewsUrl overrides
> >    NS_IMETHOD GetFolder(nsIMsgFolder **aFolder) override;
> >    NS_IMETHOD SetFolder(nsIMsgFolder *aFolder) override;
> > +  NS_IMETHOD IsUrlType(uint32_t type, bool *isType) override;
> > +  NS_IMETHOD GetServer(nsIMsgIncomingServer **aIncomingServer);
> 
> `override` missing?

Fixed in my local copy.

> Can you please enlighten me why these two overrides are
> necessary now when they weren't necessary before. What am I missing?

IsUrlType: The base class nsMsgMailNewsUrl.cpp has for this class:

  //base class doesn't know about any specific types
  NS_ENSURE_ARG(isType);
  *isType = false;

So the base class does not support types, and assumes that derived classes will support them. Previously we could override IsUrlType in JS and do that, but since it is a member of the now non-overridable nsIMsgMailNewsUrl, we can no longer do that. So we need an alternative way of supporting types, which is to add a setter and let the C++ support types in JaUrl.cpp

GetServer: Again, previously we could override GetServer in JS, but no longer. The base class GetServer is unusable because 1) it uses NS_MutateURI which will not work with XPCOM polymorphism (or so I think), plus the base class relies on if ("pop") and if ("imap") constructions, which really have no business being in the base class in the first place, and obviously we cannot insert our own if ("foo") into the C++.
OK.

nsMsgMailNewsUrl::GetServer() uses
  rv = NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID).SetSpec(urlstr).Finalize(url);
which is the brave new world for setting up a standard URL with a given spec. So that functionality hasn't changed.

All good ;-)
My hg checkin is no longer working, could you please check this in Jorg? Nits fixed.
Attachment #8953666 - Attachment is obsolete: true
Flags: needinfo?(jorgk)
Attachment #8954873 - Flags: review+
Flags: needinfo?(jorgk)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c2d11ad71eba
fix URL in JsAccount to set nsIURI things without JS overrides. r=jorgk
Status: NEW → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 60.0
You need to log in before you can comment on or make changes to this bug.