Closed Bug 678343 Opened 13 years ago Closed 13 years ago

Adapt to the new chrome worker code / Cannot parse ICS files / workerfactory undefined

Categories

(Calendar :: Internal Components, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Fallen, Assigned: Fallen)

References

Details

(Keywords: dogfood)

Attachments

(1 file)

Chromeworkers were totally rewritten on comm-central. Maybe we are lucky now and more can be done. In any case, we need to react to this change, otherwise we get:

Error: Components.classes['@mozilla.org/threads/workerfactory;1'] is undefined
Source File: resource://calendar/modules/calUtils.jsm -> file:///Users/msk/Desktop/tempmoz/comm-central/obj-x86_64-apple-darwin10.8.0/mozilla/dist/Shredder.app/Contents/MacOS/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/calendar-js/calIcsParser.js
Line: 220

This is probably just a matter of issuing |new ChromeWorker|
To make matters worse, bug 649537 has also removed xpconnect from web workers. To fix this we're going to have to move the threading inside calICSService.cpp.
Blocks: 678806
Summary: Adapt to the new chrome worker code → Adapt to the new chrome worker code / Cannot parse ICS files / workerfactory undefined
Is this change documented anywhere? Our bootstrapped extension now fails on FF8+. What is the new correct way to create a ChromeWorker if nsIWorkerFactory is no longer supported?
Gabriel, this bug is specific to Calendar, I'd suggest asking in bug 649537. I can answer your question from what I know though:

* Just use |new ChromeWorker()| instead of nsIWorkerFactory, see the tests in the patches of bug 649537.

* If you are using xpcom inside the chrome worker, you are going to have to find a different way. I believe file i/o is being allowed, but thats about it.
Thank you for the help Phillipp,
Unfortunately, Calling new ChromeWorker does not seem to work from a bootstrapped extension. I have posted over on bug 649537 for advice.
This bug makes testing Lightning on Thunderbird beta and Thunderbird nightly builds impossible. Shouldn't that make it "dogfood" and have someone assigned to it?
(In reply to Gabriel Levy from comment #3)
> Is this change documented anywhere? Our bootstrapped extension now fails on
> FF8+.

Hm, what do you mean by "bootstrapped extension"? I thought I had patched the only place that defined the global object for JS components, maybe I missed one?
https://developer.mozilla.org/en/Extensions/Bootstrapped_extensions

Our extension is built on the above model. In bootstrap.js, in the startup handler, we spawn a ChromeWorker. The previously prescribed way to create a ChromeWorker was:

var workerFactory = Components.classes["@mozilla.org/threads/workerfactory;1"].createInstance(Components.interfaces.nsIWorkerFactory);    
var worker = workerFactory.newChromeWorker("script_url.js");

as described here:

https://developer.mozilla.org/en/JavaScript_code_modules/Using_workers_in_JavaScript_code_modules

I am in a little over my head with Mozilla internals here, but I imagine that the reason is because the Worker (and ChromeWorker) classes are not defined in the context of a .jsm and apparently not in the context of bootstrap.js either. So, it was necessary to use the factory interface. If that is not possible, we'll need some alternative. Or better yet, restore the nsIWorkerFactory interface and link it up to the new code.

Thanks for looking into this, we'd like to have something that works ready and tested before FF8 ships.
Keywords: dogfood
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
I am working on this bug and will upload a patch very soon so it at least works. Unfortunately I have the feeling that moving to native runnables causes a performance decrease, but we'll see.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Flags: blocking-calendar1.0+
Attached patch Fix - v1 β€” β€” Splinter Review
This patch takes care. It got a bit larger than expected, but it really makes parsing ics files a lot faster!

I haven't yet moved all calls of parseICS to parseAsyncICS, since most of them are called from functions expected to be synchronous.
Attachment #560327 - Flags: review?(matthew.mecca)
Comment on attachment 560327 [details] [diff] [review]
Fix - v1

I'd also appreciate an extra set of eyes on the C++/XPCOM parts. Mark can you take care or maybe forward the review to someone that might do so?
Attachment #560327 - Flags: review?(mbanner)
Comment on attachment 560327 [details] [diff] [review]
Fix - v1

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

Looks good. r=mmecca

::: calendar/base/src/calIcsParser.js
@@ +141,5 @@
> +                if (Components.classes["@mozilla.org/alerts-service;1"]) {
> +                    let notifier = Components.classes["@mozilla.org/alerts-service;1"]
> +                                             .getService(Components.interfaces.nsIAlertsService);
> +                    let title = calGetString("calendar", "TimezoneErrorsAlertTitle")
> +                    let text = calGetString("calendar", "TimezoneErrorsSeeConsole");

Looks like there's a missing ';'
Attachment #560327 - Flags: review?(matthew.mecca) → review+
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/69c4e650e7b5>
-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Trunk
Backported to comm-aurora <http://hg.mozilla.org/releases/comm-aurora/rev/3c7c84d10d4b>
Target Milestone: Trunk → 1.0b8
Backported to comm-beta <http://hg.mozilla.org/releases/comm-beta/rev/cbfc2237d3f3>
Target Milestone: 1.0b8 → 1.0b7
I forgot the review comment, fixed that in a separate changeset.

I've pushed this patch already to make sure we have working nightly builds again, but as time permits I'd still appreciate a second set of eyes on the C++ parts to make sure we have no memory leaks.
Comment on attachment 560327 [details] [diff] [review]
Fix - v1

Sorry for the delay, these generally look fine to me.
Attachment #560327 - Flags: review?(mbanner) → feedback+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: