Twitter says connected but shows nothing

RESOLVED FIXED in Thunderbird 26.0

Status

Thunderbird
Instant Messaging
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Paenglab, Assigned: clokep)

Tracking

({regression})

Trunk
Thunderbird 26.0
All
Windows 7
regression

Thunderbird Tracking Flags

(thunderbird25 fixed, thunderbird26 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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
(Assignee)

Comment 1

4 years ago
Created attachment 782903 [details] [diff] [review]
c-c fix

Sorry about this, I forgot I had also had to patch c-c in that other bug.
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Attachment #782903 - Flags: review?(florian)
(Assignee)

Comment 2

4 years ago
Created attachment 782906 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Attachment #782906 - Flags: review?(dtownsend+bugmail)
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.
Attachment #782906 - Flags: review?(dtownsend+bugmail) → review+
Attachment #782903 - Flags: review?(florian) → review+
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)
(Assignee)

Comment 5

4 years ago
Created attachment 793214 [details] [diff] [review]
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.
Attachment #782906 - Attachment is obsolete: true
Attachment #793214 - Flags: review?(dtownsend+bugmail)
Attachment #793214 - Flags: review?(dtownsend+bugmail) → review+
(Assignee)

Comment 6

4 years ago
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5afcb60af43b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 26.0
(Assignee)

Comment 8

4 years ago
This shouldn't be resolved until the c-c part is checked in, but thanks.
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
https://hg.mozilla.org/comm-central/rev/4c2345e03ee4
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Comment 10

4 years ago
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
Attachment #793214 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 11

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

[Approval Request Comment]
See comment 10.
Attachment #782903 - Flags: approval-comm-aurora?
(Assignee)

Updated

4 years ago
Blocks: 884319
Comment on attachment 782903 [details] [diff] [review]
c-c fix

a=Standard8
Attachment #782903 - Flags: approval-comm-aurora? → approval-comm-aurora+
https://hg.mozilla.org/releases/comm-aurora/rev/a46b0b97c245

Not marking as fixed TB 25 until we get the m-c patch landed.
Attachment #793214 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 14

4 years ago
Attachment #793214 [details] [diff] needs to be checked into mozilla-aurora.
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-aurora/rev/c842880f0316
status-thunderbird25: --- → fixed
status-thunderbird26: --- → fixed
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.