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

RESOLVED FIXED in 1.0b7

Status

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

({dogfood})

Trunk
1.0b7
dogfood
Bug Flags:
blocking-calendar1.0 +

Details

Attachments

(1 attachment)

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
(Assignee)

Updated

7 years ago
Duplicate of this bug: 678806
(Assignee)

Updated

7 years ago
Summary: Adapt to the new chrome worker code → Adapt to the new chrome worker code / Cannot parse ICS files / workerfactory undefined

Comment 3

7 years ago
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.

Comment 5

7 years ago
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.

Comment 6

7 years ago
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?

Comment 8

7 years ago
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.

Updated

7 years ago
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+
Created attachment 560327 [details] [diff] [review]
Fix - v1

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
Last Resolved: 7 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.