Closed Bug 898760 Opened 11 years ago Closed 11 years ago

Twitter says connected but shows nothing

Categories

(Thunderbird :: Instant Messaging, defect)

All
Windows 7
defect
Not set
normal

Tracking

(thunderbird25 fixed, thunderbird26 fixed)

RESOLVED FIXED
Thunderbird 26.0
Tracking Status
thunderbird25 --- fixed
thunderbird26 --- fixed

People

(Reporter: Paenglab, Assigned: clokep)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

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
Attached patch c-c fixSplinter Review
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)
Attached patch m-c patch (obsolete) — Splinter Review
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.
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)
Attached patch m-c patch v2Splinter Review
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+
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
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 26.0
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
Closed: 11 years ago11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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?
Comment on attachment 782903 [details] [diff] [review]
c-c fix

[Approval Request Comment]
See comment 10.
Attachment #782903 - Flags: approval-comm-aurora?
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+
Attachment #793214 [details] [diff] needs to be checked into mozilla-aurora.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.