1493121 - Unnecessary use of `wrappedJSObject` in JsAccount's factory

Status: RESOLVED FIXED

(MailNews Core :: Backend, enhancement)

Description

[[e-mail]]

JsAccount needs to enumerate the delegate's methods in order for the delegator to know which methods are delegated. However, rather than directly accessing the delegate that it just created, it unnecessarily does this indirectly through XPCOM, necessitating the use of `wrappedJSObject` to recover the JS delegate object.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Since `jsDelegate` already exists as a variable, we don't need to retrieve it back from the delegator, and so we don't need to use the `wrappedJSObject` mechanism.

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 9010895 [details] [diff] [review]
Proposed patch

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

Thanks Neil, good to see you active again :-)

Sadly I don't know which state JsAccount is currently in. It went bust when M-C changed many things related to nsIURI, so I had to disable some tests, see bug 1447492 and bug 1475166. Since Kent is not active any more, it would be supercalifragilisticexpialidocious if you could adopt that lost child a bit.

I don't know whether your Level 3 right have lapse in the meantime. I prefer to do all landing myself so I can get the timing coordinated with M-C merges.

::: mailnews/jsaccount/modules/JSAccountUtils.jsm
@@ +118,5 @@
>        let delegateList = delegator.methodsToDelegate;
>        Object.keys(delegator).forEach(name => {log.debug("delegator has key " + name);});
>  
>        // jsMethods contains the methods that may be targets of the C++ delegation to JS.
> +      let jsMethods = Object.getPrototypeOf(jsDelegate);

Yes, similar to line 111:
delegateList = Object.getPrototypeOf(jsDelegate).delegateList;

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/940a5071df8d
Remove unnecessary use of `wrappedJSObject` in JsAccount's factory. r=jorgk

Comment 4

[[e-mail]]

Comment on attachment 9010895 [details] [diff] [review]
Proposed patch

[Approval Request Comment]
Risk to taking this patch (and alternatives if risky): No risk to Thunderbird. An extension would have to be very weirdly coded (i.e. probably already broken) in order to be affected.

Comment 5

[Jorg K (CEST = GMT+2)]

OK, but what's the benefit of having it in TB 60? Only consumer of JS Account is ExQuilla, right?

Comment 6

[Ben Bucksch (:BenB)]

We're developing a new extension, which is affected by this.

Comment 7

[Jorg K (CEST = GMT+2)]

Oh yes, I get the full picture now. And you're promising not to break ExQuilla. Having the patch on trunk doesn't prove anything since ExQuilla only works to TB 60.

Comment 8

[[e-mail]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e0e2d86fd8869f3fdd361fe869ce658d376e3b6b (do those JsAccount tests you mentioned somewhere still run on ESR?)

Comment 9

[Jorg K (CEST = GMT+2)]

(In reply to [e-mail] from comment #8)
> do those JsAccount tests you mentioned somewhere still run on ESR?
You are referring to bug 1447492 and bug 1475166, right.

Without knowing it, you've just fixed bug 1475166. That was a test failure caused by switching to clang-cl at version 63, so that test is still running on ESR and running again on all platforms now.

I switched off test_fooUrl.js in bug 1447492 in late March 2018 during the 61 cycle, so that test is also still running on ESR. That bug has a WIP, I could fix the crash trivially, but then it's downhill from there. Maybe you can take a look.

Comment 10

[Jorg K (CEST = GMT+2)]

TB 60.3 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/1615e21f1be9