624192 - Move Threading to Chrome Worker

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Since bug 608142 its no longer possible to dispatch objects across threads the way we have been doing it now. The upcoming patch will fix this by using chrome workers, which (since yesterday) can access threadsafe XPCOM objects.

This is just a first step to get trunk working again, I hope we can move more things to that worker in the future.

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v1

Comment 2

[Martin Schröder [:mschroeder]]

Comment on attachment 502294 [details] [diff] [review]
Fix - v1

First question and nits, otherwise looks good so far but haven't tested it functionally:

The removed function from calUtils.jsm is the only user of getWorkerThread(), so it can also be removed.

>diff -r 2b4b40c2f1d1 calendar/base/src/calIcsParser.js
>--- a/calendar/base/src/calIcsParser.js	Sat Jan 08 20:50:04 2011 +0100
>+++ b/calendar/base/src/calIcsParser.js	Sat Jan 08 23:31:24 2011 +0100
[...]
>@@ -221,70 +220,65 @@
[...]
>+            worker.onerror = {
>+                handleEvent: function(error) {
>+                    cal.ERRRO("Error Parsing ICS: " + error.message);
>+                    aAsyncParsing.onParsingComplete(Components.results.NS_ERROR_FAILURE, this_);
>+                }
>+            };

Typo: cal.ERROR(...)

[...]

>     parseFromStream: function ip_parseFromStream(aStream, aTzProvider, aAsyncParsing) {
[...]
>+        let stringData = unicodeConverter.convertFromByteArray(octetArray, octetArray.length);
>+
>+        this.parseString(stringData, aTzProvider, aAsyncParsing);
>     },

The reading of the string from the stream seems to be no longer on an extra thread/in the worker after this patch. Is this your intention?

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to comment #2)
> The reading of the string from the stream seems to be no longer on an extra
> thread/in the worker after this patch. Is this your intention?

Yes and no. The unicode converter is not threadsafe, so we can't access it from a worker. This is something we'll have to fix another time.

Thanks for catching the other errors!

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/9c767af63b3a>
-> FIXED

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to comment #2)
> Comment on attachment 502294 [details] [diff] [review]
> Fix - v1
> 
> First question and nits, otherwise looks good so far but haven't tested it
> functionally:
Oops, I missed the fact that you didn't actually say r+ ;-) Go ahead and do anything you still wanted to do and let me know if it fails.

Comment 6

[Martin Schröder [:mschroeder]]

Comment on attachment 502294 [details] [diff] [review]
Fix - v1

Patch was checked in long ago... so finally setting r+ (without testing because I have some build problems on my Mac like our tinderboxen).