898760 - Twitter says connected but shows nothing

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Richard Marti (:Paenglab)]

After landing of bug 894367 the authentication window doesn't appear but now there is no Twitter account in Conversations list and no tweets are shown. The account says it's connected.

In console I get following:

Error: TypeError: aOnError is null
Source file: resource://gre/components/twitter.js
Line: 593
Source code:
prpl-twitter
 ----------
Error: aOnError is null
Source file: resource://gre/components/twitter.js
Line: 467

Comment 1

[Patrick Cloke [:clokep]]

Attachment: c-c fix

Sorry about this, I forgot I had also had to patch c-c in that other bug.

Comment 2

[Patrick Cloke [:clokep]]

Attachment: m-c patch

This patch truly makes all options in Http.jsm optional, i.e. it will ignore null, "" or undefined entries (this matches our previous behavior of doXHRequest). I unfortunately don't have time to make a test for this right now, but if one is wanted I'll try to get it done in the next week.

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 782906 [details] [diff] [review]
m-c patch

It would be good to get a test here as it clearly breaks things, but you can land for now anyway.

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 782906 [details] [diff] [review]
m-c patch

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

If I remember correctly using the value of an undefined property causes a warning, but just null checking it doesn't.

::: toolkit/modules/Http.jsm
@@ +34,5 @@
>    let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
>                .createInstance(Ci.nsIXMLHttpRequest);
>    xhr.mozBackgroundRequest = true; // no error dialogs
>    let hasPostData = "postData" in aOptions && aOptions.postData;
> +  xhr.open(("method" in aOptions && aOptions.method) ? aOptions.method :

I think:
("method" in aOptions && aOptions.method) ? aOptions.method :

can be simplified to:
aOptions.method ||

@@ +40,5 @@
>    xhr.channel.loadFlags = Ci.nsIChannel.LOAD_ANONYMOUS | // don't send cookies
>                            Ci.nsIChannel.LOAD_BYPASS_CACHE |
>                            Ci.nsIChannel.INHIBIT_CACHING;
>    xhr.onerror = function(aProgressEvent) {
> +    if ("onError" in aOptions && aOptions.onError) {

And this simplified to:
if (aOptions.onError)

Comment 5

[Patrick Cloke [:clokep]]

Attachment: m-c patch v2

This takes into account Florian's feedback (which actually simplifies the code quite a bit). It also attempts to add a test that I'm afraid isn't testing anything...but I can't think of a way to test that the null checks work. Any better ideas would be appreciated.

Comment 6

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5afcb60af43b

I'm not sure of c-c status right now, so I'll try to catch people during normal hours to ask and then check the c-c patch in.

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5afcb60af43b

Comment 8

[Patrick Cloke [:clokep]]

This shouldn't be resolved until the c-c part is checked in, but thanks.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/4c2345e03ee4

Comment 10

[Patrick Cloke [:clokep]]

Comment on attachment 793214 [details] [diff] [review]
m-c patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 884319
User impact if declined: Twitter will not work in TB 25
Testing completed (on m-c, etc.): TB 26 (Daily) works fine with this change
Risk to taking this patch (and alternatives if risky): I don't see much risk in taking this change (especially for m-c), for c-c it has been tested in Daily and works fine. There is some risk that this could stop FileLink or Lightning's usage of this from working properly, but it's only adding error checking.
String or IDL/UUID changes made by this patch: N/A

Comment 11

[Patrick Cloke [:clokep]]

Comment on attachment 782903 [details] [diff] [review]
c-c fix

[Approval Request Comment]
See comment 10.

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 782903 [details] [diff] [review]
c-c fix

a=Standard8

Comment 13

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-aurora/rev/a46b0b97c245

Not marking as fixed TB 25 until we get the m-c patch landed.

Comment 14

[Patrick Cloke [:clokep]]

Attachment #793214 [details] [diff] needs to be checked into mozilla-aurora.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c842880f0316