1447492 - Fix fallout from bug 1447272 - mailnews/jsaccount/test/unit/test_fooUrl.js is crashing

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1447272 +++

mailnews/jsaccount/test/unit/test_fooUrl.js is crashing

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1447492-disable-test.patch

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5bad3a176144
temporarily disable crashing test test_fooUrl.js. rs=bustage-fix

Comment 3

[Jorg K (CEST = GMT+2)]

TB 60 is unaffected since the broke after the branch to TB 61 on 2018-03-12.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1447492-fix-test_fooUrl.patch

This fixes the crash, but the test then fails on line 31 and 67, both
  let fooUrl = url.getInterface(Ci.msgIFooUrl);

Comment 5

[Jorg K (CEST = GMT+2)]

Oh, the test fails with
ERROR NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIInterfaceRequestor.getInterface]

Comment 6

[Jorg K (CEST = GMT+2)]

Hello Valentin,

to allow other mail protocols to be added by add-ons, TB has a component JS Account which "plays" a lot with URLs since we address many things in the system with our own URL types. For example Kent's add-on ExQuilla to allow TB to connect to MS Exchange servers creates a scheme "ews" (Exchange Web Services).

The JS Account component has a few tests, and one of them, test_fooUrl started crashing after bug 1447272 which was the mailnews port of bug 1442239. Finding the reason of the crash wasn't hard, but now the test fails here:
https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/jsaccount/test/unit/test_fooUrl.js#65
(and further up) on |let fooUrl = url.getInterface(Ci.msgIFooUrl);|

That "foo url" is implemented here:
https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/jsaccount/test/unit/resources/testJaFooUrlComponent.js#57

So the question is: Can URLs still be implemented in JS? If so, any hints why the test fails now?

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1447492-fix-test_fooUrl.patch

Comment 8

[Valentin Gosu [:valentin] (he/him)]

(In reply to Jorg K (GMT+2) from comment #6)
> So the question is: Can URLs still be implemented in JS? If so, any hints
> why the test fails now?

We made the nsIURI interfaces non-scriptable, but you could write a JS wrapper for the URIs - it seems JaBaseUrl is just that, but I haven't looked at it too closely to figure out if there's a problem there.
A major caveat is that the JS wrapper shouldn't be passed into any Gecko interface (don't navigate to it, or anything), as that would pass a reference to it into Gecko, and it may end up being used off-main-thread, which would lead to a crash.

I don't see fooURL adding any relevant functionality to URLs. Any extra attributes that are currently saved on the URL can easily be held in a hashtable<URL, values>.
So, I really don't recommend implementing URLs in JS. One way or another it will lead to bugs.

Comment 9

[Wayne Mery (:wsmwk)]

Ben, is there a blocker to doing this?

Comment 10

[Ben Bucksch (:BenB)]

Nope, I just forgot about it.

Comment 11

[[e-mail]]

Comment on attachment 9011118 [details] [diff] [review]
1447492-fix-test_fooUrl.patch

>-      Assert.ok(urlQI);
>+      // Since the URL wasn't properly initialised, that is, it has no spec
>+      // the following will crash. The underlying nsMsgMailNewsUrl
>+      // has no m_baseURL yet and hence GetSpec() triggered by the
>+      // Assert(urlQI) will crash. So use this instead:
>+      Assert.ok(urlQI != null);
>     }
>     for (let iface of extraInterfaces) {
>       let fooUrl = url.getInterface(iface);
>       Assert.ok(fooUrl instanceof iface);
>       Assert.ok(fooUrl.QueryInterface(iface));
For me, with the change above also applied at this line, the test then passes locally.

The issue you mention in comment #5 might have been bug 1511359?

Comment 12

[[e-mail]]

Attachment: Added parallel fix and reenabled test

I also kicked off a Try build: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=e7ff7dfa23343bb5585ec0d1c5c11ade230e04d4

Comment 13

[Geoff Lankow (:darktrojan)]

Comment on attachment 9210025 [details] [diff] [review]
Added parallel fix and reenabled test

Seems reasonable to me. You need to fix the comment though, you've got Assert(urlQI) not Assert.ok(urlQI).

Comment 14

[[e-mail]]

Attachment: With fixed comment

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1d6b537eb803
Fix crash in test_fooUrl.js. r=neil, darktrojan