Closed Bug 901329 Opened 11 years ago Closed 11 years ago

Thunderbird doesn't start when using master password and Google CalDAV OAuth2 authentication

Categories

(Calendar :: Provider: CalDAV, defect)

Lightning 2.6
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ssitter, Assigned: Fallen)

References

Details

(Keywords: hang)

Attachments

(1 file, 2 obsolete files)

Follow up for Bug 853236 Comment 10:

1. setup a master password
2. connect to Google Calendar using the new CalDAV endpoint 
   https://apidata.googleusercontent.com/caldav/v2/{calid}/events
3. fill in the OAuth prompt and allow Lightning to access Google Calendar
4. restart Thunderbird

Actual error:
Thunderbird will not startup anymore, the master password prompt will appear partially but is not operable and Thunderbird uses 100 % CPU cycles.
Google will will shut down CalDAV via http authentication in September 2013. If many user switch to CalDAV via OAuth authentication and Lightning 2.6 they might be affected by this bug.
I see this is an issue that needs fixing, but I won't have time to take care before September 5th. Do you think you could spend a little time to debug this? I would be delighted!
Confirming on Ubuntu.

The master password dialog appears empty, but interestingly you can still type the password and press enter, and everything loads normally.

The master password is prompted when we call into Services.logins.findLogins() here:
http://mxr.mozilla.org/comm-central/source/calendar/base/modules/calAuthUtils.jsm#123
OS: Windows 7 → All
Hardware: x86_64 → All
I think we call the master passwords dialog blocking (which we have to), which might not play well with that dialog. Maybe we can force the master password retrieval before the dialog opens?
Attached patch Workaround v1 (obsolete) β€” β€” Splinter Review
It seems like the master password dialog is having trouble because it's being loaded too early, maybe because the app window isn't loaded yet?

I'm not sure if this is the best way to go, but adding in a delay before setupAuthentication seems to get it working right, but I've only had time to test it briefly.
Attachment #800769 - Flags: feedback?(philipp)
Comment on attachment 800769 [details] [diff] [review]
Workaround v1


>-            this.setupAuthentication(aChangeLogListener);
>+            let self = this;
>+            function doSetupAuthentication() self.setupAuthentication(aChangeLogListener);
>+            setTimeout(doSetupAuthentication, 1);

You can also do setTimeout(this.setupAuthentication.bind(this, aChangeLogListener), 0);


It sounds reasonable and from a brief look I don't think it will interfere. We just need to make sure this doesn't increase the number of master password prompts. f+, but I'd appreciate if Stefan could give this patch a test.
Attachment #800769 - Flags: feedback?(philipp) → feedback?(ssitter)
With a second calendar added using the v2 API I get 2 master password prompts with or without this patch applied. Without the patch applied the original issue went away, I suspect the initialization code on the additonal calendar provided enough of a delay on its own to avoid the problem.
I have worked around the probem activating the StartupMaster plugin (asks for master password at startup). This is on TB Windows v24 with Lightning v2.6.
I've been toying around with this a bit. One easy solution is to follow what StartupMaster does and load the master password prompt early, i.e remove the isSunbird condition in calCalendarManager. This only postpones the problem though: if you cancel the first dialog, you run into the same situation. 

I tried using msgAsyncPrompter, but this doesn't really work, as it expects the onPromptStart function to be synchronous. Using the thread manager to spin the event loop didn't work.

Using a timeout is rather random, values 0, 1 or 100 don't work for me on mac. I'm not sure how to solve this correctly unfortunately.
Attached patch Fix - v2 (obsolete) β€” β€” Splinter Review
Ok, I've found a solution for this that sticks and also fixes a few more issues. First of all this fixes bug 921744 by using the async prompter from Thunderbird. Next up, when the browser window for authentication is just closed we used to not do anything about it. Then I'm geting rid of an extra error message in the console when the master password dialog is cancelled.

The fix for this bug is actually to wait until the main window is loaded. This makes the master password dialog show up correctly. I'm aware this can cause problems if there is no main window, but I'm not sure how to better fix this. As mitigation, maybe it makes sense to add a maximum timeout? Doesn't make me feel much better though.
Assignee: nobody → philipp
Attachment #800769 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #800769 - Flags: feedback?(ssitter)
Attachment #812566 - Flags: review?(matthew.mecca)
Attached patch Fix - v3 β€” β€” Splinter Review
Oops, missing qrefresh. Here it is.
Attachment #812566 - Attachment is obsolete: true
Attachment #812566 - Flags: review?(matthew.mecca)
Attachment #812567 - Flags: review?(matthew.mecca)
Comment on attachment 812567 [details] [diff] [review]
Fix - v3

Looks good. r=mmecca
Attachment #812567 - Flags: review?(matthew.mecca) → review+
Attachment #812567 - Flags: approval-calendar-release+
Attachment #812567 - Flags: approval-calendar-beta+
Attachment #812567 - Flags: approval-calendar-aurora+
Please push to all branches: c-c, c-a, c-b and comm-esr24.
Keywords: checkin-needed
Target Milestone: --- → 2.6.1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: