Open Bug 1447492 Opened 2 years ago Updated 5 months ago

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

Categories

(MailNews Core :: Backend, defect)

defect
Not set

Tracking

(thunderbird_esr60 unaffected, thunderbird60 unaffected, thunderbird61 affected, thunderbird62 affected)

ASSIGNED
Tracking Status
thunderbird_esr60 --- unaffected
thunderbird60 --- unaffected
thunderbird61 --- affected
thunderbird62 --- affected

People

(Reporter: jorgk-bmo, Assigned: BenB)

References

Details

(Keywords: leave-open, Whiteboard: [Thunderbird-testfailure: X all][Thunderbird-disabled-test])

Attachments

(2 files, 1 obsolete file)

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

mailnews/jsaccount/test/unit/test_fooUrl.js is crashing
Flags: needinfo?(rkent)
Keywords: leave-open
Whiteboard: [Thunderbird-testfailure: X all][Thunderbird-disabled-test]
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5bad3a176144
temporarily disable crashing test test_fooUrl.js. rs=bustage-fix
Assignee: nobody → rkent
Status: NEW → ASSIGNED
TB 60 is unaffected since the broke after the branch to TB 61 on 2018-03-12.
Attached patch 1447492-fix-test_fooUrl.patch (obsolete) — Splinter Review
This fixes the crash, but the test then fails on line 31 and 67, both
  let fooUrl = url.getInterface(Ci.msgIFooUrl);
Oh, the test fails with
ERROR NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIInterfaceRequestor.getInterface]
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?
Flags: needinfo?(valentin.gosu)
(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.
Flags: needinfo?(valentin.gosu)
Assignee: rkent → ben.bucksch
Flags: needinfo?(rkent)
You need to log in before you can comment on or make changes to this bug.